Add a --zarr-format CLI option#483
Conversation
jeromekelleher
left a comment
jeromekelleher
left a comment
There was a problem hiding this comment.
LGTM - I think we should stabilise on this as a long term interface now.
| zarr_format_kwargs = dict( | ||
| zarr_format=self.zarr_format or zarr_utils.ZARR_FORMAT | ||
| ) | ||
| root = zarr.open(store=self.path, mode="a", **zarr_format_kwargs) |
There was a problem hiding this comment.
Let's make a zarr_utils.open_zarr function which takes zarr_format as an argument to hide these details.
There was a problem hiding this comment.
Let's change this to
DEFAULT_ZARR_FORMAT = int(os.environ.get("BIO2ZARR_DEFAULT_ZARR_FORMAT", "2"))
and change the comment. This feels like a reasonable thing to do long-term so we can get rid of the caveat.
| "--zarr-format", | ||
| type=click.Choice([2, 3]), | ||
| default=None, | ||
| help="Zarr format version of output", |
There was a problem hiding this comment.
Document that this is 2 by default, but this behaviour can be changed by using the BIO2ZARR_DEFAULT_ZARR_FORMAT env var.
May as well do it in one go? Should be a straightforward change, and better to know now if there are any hidden complications. |
Remove unnecessary monkeypatching Rename ZARR_FORMAT to DEFAULT_ZARR_FORMAT and similarly for the env var Add zarr_format to more signatures Add open_zarr_append convenience function
Add plink zarr_format test Add vcf zarr_format test
tomwhite
force-pushed
the
zarr-format-cli-option
branch
from
e447ed9 to
c903d26
Compare
|
Thanks for the suggestions @jeromekelleher. I've implemented all of them and added some tests to check the |
jeromekelleher
left a comment
jeromekelleher
left a comment
There was a problem hiding this comment.
LGTM, couple of minor suggestions.
| def __init__(self, source_type, path, zarr_format=None): | ||
| self.source_type = source_type | ||
| self.path = pathlib.Path(path) | ||
| self.zarr_format = zarr_format or zarr_utils.DEFAULT_ZARR_FORMAT |
There was a problem hiding this comment.
| self.zarr_format = zarr_format or zarr_utils.DEFAULT_ZARR_FORMAT | |
| self.zarr_format = zarr_format |
Let's keep that decision about what "None" means localised
|
|
||
| def open_zarr_append(store, zarr_format): | ||
| """Open a Zarr store for append using the given Zarr format""" | ||
| zarr_format_kwargs = dict(zarr_format=zarr_format or DEFAULT_ZARR_FORMAT) |
There was a problem hiding this comment.
I'd be happier if this was explicit, so that things like zarr_format=[] or zarr_format=False don't silently get turned into 2:
if zarr_format is None:
zarr_format = DEFAULT_ZARR_FORMAT
2 participants
First draft of #482. Still needs a test for the new option. Also not sure whether to do other formats (plink etc) here or in follow ups.