Finalise alignment export

[jeromekelleher]

Add support for outputting alignments in FASTA and nexus formats, and fixes #1893.

[codecov]

Codecov Report

Merging #1894 (cc0a9e7) into main (6f3f942) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1894      +/-   ##
==========================================
+ Coverage   93.15%   93.16%   +0.01%     
==========================================
  Files          27       27              
  Lines       25061    25099      +38     
  Branches     1104     1109       +5     
==========================================
+ Hits        23345    23383      +38     
  Misses       1682     1682              
  Partials       34       34              

Flag	Coverage Δ
c-tests	92.21% <ø> (ø)
lwt-tests	89.14% <ø> (ø)
python-c-tests	68.30% <18.91%> (-0.14%)	⬇️
python-tests	98.76% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
python/tskit/cli.py	95.93% <ø> (ø)
python/tskit/text_formats.py	100.00% <100.00%> (ø)
python/tskit/trees.py	97.79% <100.00%> (-0.01%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f3f942...cc0a9e7. Read the comment docs.

[jeromekelleher]

This is ready for a look, can you have a look please @hyanwong @benjeffery?

[jeromekelleher]

Bumping this - this is a major new piece of functionality so it would be good to get some eyes on it.

[hyanwong]

This all looks fine to me, modulo some minor comments (especially about how we detect missing data).

One more general point is that we should perhaps think about how in the future we would deal with indels, so that we don't paint ourselves into a corner now. However , it's hard to imagine how to align sequences with indels, unless the reference allele is the longest one (e.g. ATC) and any mutations are the same length or shorter. There would also be a weird interaction between e.g. a site at position 10 with ancestral_state "ATC" and a site at position 11 with ancestral state "G". Nevertheless, will will have to deal with indels at some point, so it might be worth thinking about how other libraries do it.

[hyanwong]

Note that I think this also closes #353 doesn't it? Unless we want to leave an issue open to deal with indels?

[jeromekelleher]

Note that I think this also closes #353 doesn't it? Unless we want to leave an issue open to deal with indels?

Yep - it's in the commit messages

[benjeffery]

LGTM, % @hyanwong's questions.

[jeromekelleher]

Thanks for the comments @hyanwong - I've made another pass. We're not going to get this perfect for missing data on this pass, but it'll be good enough to cover 99% of use cases. I'm fairly sure it's currently conservative, so that we may throw an error where not strictly necessary, but we'll never output something that's actually wrong.

[jeromekelleher]

Re indels, I agree we'll have to tackle this at some point. The only thing we'll need to worry about here I think in terms of forward compatibility is whether we insist that the reference sequence is equal to the sequence length when storing the reference data (#146). I don't think it affects the code here.

[hyanwong]

Re indels, I agree we'll have to tackle this at some point. The only thing we'll need to worry about here I think in terms of forward compatibility is whether we insist that the reference sequence is equal to the sequence length when storing the reference data (#146). I don't think it affects the code here.

Right. I was wondering how people store multiple sequences in a FASTA file if there are indels? Do they just allow each sequence to be a different length, and to hell with the alignment & reference sequence? Or do they pad out all the deletions with "-", and potentially have more characters in each sequence than in the reference? It might be helpful to talk to someone who knows about this sort of thing, although I agree it doesn't seem like it should affect this PR.

[jeromekelleher]

OK, merging this! Now the big question is whether we go ahead and release 0.4.0 like this or if we implement reference sequences first...