Add API for iterating over variants

[tomwhite]

Fixes #112

While writing this I noticed a few inconsistencies with variant_chunk_iter:

• variant_chunk_iter takes a root Zarr group, whereas variant_iter in this PR takes a filename. Any preferences?
• We should rename variant_regions and variant_targets (in variant_chunk_iter) to just regions and targets (as I've done for variant_iter in this PR).
• variant_chunk_iter takes a samples_selection array, whereas variant_iter in this PR takes a bcftools-style samples string. That's probably OK as sometimes we want to parse samples before iterating (for query and view commands for example). We could easily support both samples and samples_selection (mutually exclusive) in both functions if needed.

[jeromekelleher]

variant_chunk_iter takes a root Zarr group, whereas variant_iter in this PR takes a filename. Any preferences?

I think the filename is the correct choice here as this is going to be part of the external API, and I'd rather not have an explicit API dependency on Zarr (at least initially).

We should rename variant_regions and variant_targets (in variant_chunk_iter) to just regions and targets (as I've done for variant_iter in this PR).

Agreed

variant_chunk_iter takes a samples_selection array, whereas variant_iter in this PR takes a bcftools-style samples string. That's probably OK as sometimes we want to parse samples before iterating (for query and view commands for example). We could easily support both samples and samples_selection (mutually exclusive) in both functions if needed.

This bit I don't like - why not just a list of strings? I don't think the extra bcftools sample selection syntax is particularly powerful, and it's easy to replicate with some extra API functions if we need to.

[tomwhite]

We should rename variant_regions and variant_targets (in variant_chunk_iter) to just regions and targets

I've now done this. I've also added some tests for the edge case where fields=[], which I've interpreted as returning an empty iterator (based on Pandas read_csv behaviour).

This bit I don't like - why not just a list of strings?

I was about to change to a list of strings, but there is actually an extra complication due to the ^ exclude syntax (see https://github.com/sgkit-dev/vcztools/blob/main/vcztools/samples.py#L10-L48). A plain string is consistent with having a string for regions - i.e. both are just bcftools-style.

[percyfal]

LGTM. As concerns the samples being formatted as a string or array, it seems the functionality for parsing the samples string is already in place (samples.parse_samples), so presumably the samples-as-string syntax is already in effect elsewhere? Should both expressions be exposed to the user or should the API only expose one for simplicity?

[tomwhite]

As concerns the samples being formatted as a string or array, it seems the functionality for parsing the samples string is already in place (samples.parse_samples), so presumably the samples-as-string syntax is already in effect elsewhere?

Yes, it's used in the CLI where you can pass a bcftools-style string such as -s ^NA00003,NA00002,NA00003

Should both expressions be exposed to the user or should the API only expose one for simplicity?

I'm suggesting we start with the single string one, but could add more later if needed (e.g. a samples_selection NumPy indexing array).

[jeromekelleher]

I don't like the way that this implies we have to do things like

variant_iter(url, samples=",".join(list_of_samples))

this is an anti-pattern IMO, and I don't think we should be entirely constrained by the CLI syntax when designing the Python API.

How about we use parameter type checking to decide the appropriate action? So, for samples if it is a single string, we treat it as a bcftools-like input with the full syntax. Otherwise, we treat it as a list of sample_id strings, as you would expect in a Python API. Similarly for regions and targets.

[tomwhite]

So, for samples if it is a single string, we treat it as a bcftools-like input with the full syntax. Otherwise, we treat it as a list of sample_id strings, as you would expect in a Python API. Similarly for regions and targets.

Sounds good - I've implemented this in the latest commit.