fasta rework

[soypat]

So this is NOT intended as a performance boost change. This is above all an API rework for ease of use and clarity.

I have implemented a fasta.Parser type. This type is intended to be the base to parse all fasta data and replace all other implementations.

Benefits of Parser.ParseNext() when compared to XConcurrent functions:

• Provides the bare minimum API signature to parse a single fasta genome
• It is a simple and easy to understand API.
• No ambiguities on API use.
• Takes a io.Reader for user liberty to use with any API or stream.
• User has more control over how many fasta genomes they want to read
• (EDIT) Also errors! ParseNext can return a very useful error with the line number on which parsing stopped

goos: linux
goarch: amd64
pkg: github.com/TimothyStiles/poly/io/fasta
cpu: Intel(R) Core(TM) i5-8265U CPU @ 1.60GHz
BenchmarkFastaLegacy-8   	      55	  19968425 ns/op	17326897 B/op	  158406 allocs/op
BenchmarkParser-8        	     118	   9643441 ns/op	16795447 B/op	   58150 allocs/op

[gitpod-io]

[TimothyStiles]

@soypat the benchmark needs some fixing but this is looking really good! Is your plan to eventually deprecate the legacy fasta parsers as well?

[soypat]

@TimothyStiles I'd really like to see those "Concurrent" parsers go. Maybe Read can be renamed as ReadFile, but that'd be gravy. Parse is OK as shorthand for creating a new parser and reading all.

[soypat]

@TimothyStiles So I have changed the following things:

• I initialize uniprot 1MB fasta as a raw format string in TestMain. This will avoid some noisiness of file manipulation in benchmarks
• I have added an argument to NewParser: maxLineSize, in bytes. This is to give users some freedom of how much heap they want to use for the parser.
• Added error handling during benchmarks
• Better error handling for the non-EOF case during parsing

[TimothyStiles]

@soypat recent additions are looking great. Having a shorthand like Parse would be excellent.

Reset, ParseAll and ParseN need to be tested and it looks there are a couple of corner cases in ParseNext that also need to be tested.

[soypat]

@TimothyStiles Alright, looks much better. Fixed various bugs and test coverage is up to 95%

[TimothyStiles]

success case doesn't seem to be covered by tests?

[TimothyStiles]

@soypat I just rewrote fasta.Parse as a wrapper around fasta.ParseAll and had a couple of tests fail.

ExampleWrite fails and doesn't return anything when trying to read what was written out. Couple of things are happening here.

1. New parser is stripping the * character which denotes a stop codon. Since it's technically not a amino acid it doesn't have a letter but is important to denote.
2. It looks like there's a fatal difference between what the parser can read and what the writer outputs.

Been super busy the past several days but will be free to work on this together any time this week after tonight!

[TimothyStiles]

we should probably be more permissive and allow EOF-ended Fasta as valid?

[TimothyStiles]

@soypat my latest changes have this entire suite of tests failing on off-by-one errors where there's always one more fasta than what is expected. Any idea what's going on?

[soypat]

I'll take a deep dive look today! Sorry, was in the zone with another project yesterday

[soypat]

I'm hesitant to say EOF ended fastas are OK. This means you can't use io.LimitReader to read fastas at all using poly API since its use will return incomplete fastas without warning. The reason this is important to me is because io.LimitReader is a commonly used foundation for writing software resilient to adversarial user input.

Though I'd also like users who want to parse EOF ended fastas to be able to use poly. This is possible since we can return a valid fasta alongside an io.EOF error when the fasta is immediately terminated by an EOF. This will allow users who need this functionality to build their own parsers who treat the EOF case specially.

Note: ParseNext now (correctly?) parses data/empty.fasta, returning a single fasta.

[soypat]

Maybe we can add a boolean flag to the ParseN, ParseAll, ParseBytes: acceptEOFTerminatedFastas?

[TimothyStiles]

Maybe we can add a boolean flag to the ParseN, ParseAll, ParseBytes: acceptEOFTerminatedFastas?

This feels a little clunky but may be the way to do it. Should this variadic or explicit?

[TimothyStiles]

Note: ParseNext now (correctly?) parses data/empty.fasta, returning a single fasta.

Just made empty.fasta truly empty. It's meant to test the corner cases I just commented on.

[soypat]

This feels a little clunky but may be the way to do it. Should this variadic or explicit?

Maybe we can have parser options? Something like this https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis? Or maybe that's too much- just a struct with fields?

[soypat]

I removed empty.fasta since it can be represented as a const string