Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.38%. Comparing base (1aa0233) to head (d5dd3f4).
Report is 1 commits behind head on main.

@@            Coverage Diff             @@
##             main     #964      +/-   ##
==========================================
+ Coverage   93.31%   93.38%   +0.07%     
==========================================
  Files          18       18              
  Lines        6460     6475      +15     
  Branches     1096     1101       +5     
==========================================
+ Hits         6028     6047      +19     
+ Misses        294      291       -3     
+ Partials      138      137       -1     

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

Also fixes #763 I think

Thanks @hyanwong - I've been some thinking on API design here and am thinking a simpler solution is to error out if there is more than one contig in the used variants.
As for contig_length, I don't think this is the same conceptually as tskit sequence_length? As in tskit positions have to be less than sequence length, whereas there is no such requirement in vcf zarr, so we risk causing odd (to the user) errors downstream if we use contig_length. So how about we check if contig_length is greater than the positions and error early if it isn't, and allow the user to override with a sequence_length argument. If both are missing use the largest position+1.

Sorry, brief reply while away, but this came from user feedback in tsinfer: people want to be able to take a vcf with multiple chromosomes in it (which is the common input format), select a chromosome, and run inference without too much extra thought (e.g. they are probably not the original generators of the vcf, and may not want to have to parse the vcf themselves). I assume this would include getting the seq len from the vcf if that is specified? But whatever makes it least error-prone for a naive user to run tsinfer on an externally provided vcf file, I guess?

I think checking contig length if present and erroring out early if it is problematic seems fine?

people want to be able to take a vcf with multiple chromosomes in it (which is the common input format), select a chromosome, and run inference without too much extra thought

I think this scheme enables that - you bio2zarr then write a contig-specific mask, which is a one-liner we can put in the tutorial. I don't want to add things like contig_id to the API when they overlap and interact with existing functionality (masking).

Thanks @hyanwong - I've been some thinking on API design here and am thinking a simpler solution is to error out if there is more than one contig in the used variants. As for contig_length, I don't think this is the same conceptually as tskit sequence_length? As in tskit positions have to be less than sequence length, whereas there is no such requirement in vcf zarr, so we risk causing odd (to the user) errors downstream if we use contig_length. So how about we check if contig_length is greater than the positions and error early if it isn't, and allow the user to override with a sequence_length argument. If both are missing use the largest position+1.

Having the option to override the sequence_length is a feature that I'm looking for in a project that i'm doing right now. I've been using tsinfer to create tree sequences from windowed chunks of a VCF file that I will analyze downstream in a ML pipeline.

How does this look @hyanwong

LGTM. Fine to merge, I reckon.

(p.s. #765 suggests that the error message which currently reads

"Sites belong to multiple contigs ({contig_names}). "
"Please restrict sites to one contig using the sites_mask argument."

Should have a code snippet to show users how to do this?

Should have a code snippet to show users how to do this?

Good idea, added

@jeromekelleher This is now ready for your review.