Properly treat blank ancestral alleles

[hyanwong]

There was a bug in the previous code: since "" is the fill-value for alleles, if an ancestral allele was "", it wasn't treated as missing.

This PR fixes that bug, but also fixes #962: the string "" "N" is treated as meaning "deliberately set the ancestral allele as unknown". This is OK because "" is not a valid VCZ allele string. Therefore if the only ancestral alleles that don't match are "", then it's confusing to issue a warning, because setting unknown alleles is a deliberate user choice.

[codecov]

Codecov Report

Attention: Patch coverage is 97.50000% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.15%. Comparing base (1d04fb8) to head (15705f9).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
tsinfer/formats.py	97.43%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #963      +/-   ##
==========================================
+ Coverage   93.13%   93.15%   +0.01%     
==========================================
  Files          18       18              
  Lines        6354     6368      +14     
  Branches     1136     1142       +6     
==========================================
+ Hits         5918     5932      +14     
  Misses        296      296              
  Partials      140      140              

Flag	Coverage Δ
C	93.15% <97.50%> (+0.01%)	⬆️
python	95.50% <97.50%> (+0.01%)	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[benjeffery]

LGTM, thanks @hyanwong

I've thought about it more and think that unknown should be explicitly N or another character as that is usually the case in FASTA etc. "" would probably only get into the ancestral allele as a mistake right?

[jeromekelleher]

I don't think we should assign any meaning to the empty string - as @benjeffery "N" is a much better signal for unknown from the FASTA.

[hyanwong]

The problem with "N" is surely that it could match "N" in the actual VCF? It would require extra hardcoded logic not to match "N" against existing alleles. It seemed a bit neater to me to pick something that explicitly isn't allowed as an allele in a VCF file.

I guess it's fine hardcoding a specific string like N as long as it's part of the VCF spec and unlikely to change? I'm idly wondering why N rather than e.g. . or n or -, or whatever all the other conventions are? I'm no VCF expert, so happy to defer to people who are.

Note that inference will find a different ancestral allele from any what we set here.

[hyanwong]

Just looking at how to do this. What alleles strings do we want to treat as "unknown ancestral state"? I assume "N", "n" and "". The VCF spec says nothing about "." in the REF and ALT fields (it's only in the GT field).

If we want "N" and "n" to be the "deliberately unknown" state, we would skip the warning if these were the only non-matching alleles.

[hyanwong]

I changed the behaviour to using (and documenting) N as the standard unknown ancestral allele. I also wrote the docstring from VariantData, so that this behaviour is described. It would be good to merge this, so that I can start adding documentation for the other changes such as contig_id that I've been making.

[hyanwong]

I think this is as you wanted now @jeromekelleher (N or n are the "unknown" ancestral state, with warnings for anything else): it's a pretty simple change. Do you want to quickly review?

[jeromekelleher]

LGTM, just needs a bit of simplification

[jeromekelleher]

We shouldn't treat empty string as legal input here as it has a defined meaning in VCZ

[hyanwong]

Now done. See comment below about changing the name to "ancestral_state", to match what we used previously.

[hyanwong]

As discussed with @jeromekelleher: we warn on unknown ancestral states, unless they are all "N" or "n", in which case we merely output info.

Note that after discussion with @benjeffery, this also changes the parameter name to "ancestral_state" (to match tskit, and so that it is clearer that it differs from the index (which is known as the "ancestral allele"). It also fixes #927. I have tested it on a 200Mb 1000 genomes VCF, and the check does not noticeably increase the construction time of the VariantData object (which in this case was ~0.7 secs)

[hyanwong]

Also note that I duplicated the sites() constructor so that in the VCF Zarr case, it trims the allele list so that it doesn't include the padding values. This can then be used when checking the ancestral allele states (and is fast)

[jeromekelleher]

Nice!