ZarrSource support Stores/path

funkelab / gunpowder

A library to facilitate machine learning on multi-dimensional images.

https://funkelab.github.io/gunpowder/

MIT License

78 stars 56 forks source link

ZarrSource support Stores/path #186

Closed mzouink closed 1 year ago

mzouink commented 1 year ago

Fixing https://github.com/funkey/gunpowder/issues/184 Pre-Merge Checklist:

[ ] if this PR adds a feature: request merge into the latest development branch (vX.Y-dev)
[ ] if this PR fixes a bug: request merge into the latest patch branch (patch-X.Y.Z)
[ ] PR branch name is short and describes the feature/bug addressed in this PR
[ ] commit messages are consistent
[ ] changes reviewed by another contributor
[ ] tests cover changes
[ ] all tests pass

pattonw commented 1 year ago

I like the idea of using stores as a more general interface for the Zarr read/write nodes, but I'm not sure if I like the new interface with a mandatory filename and optional store. So I guess if I had a store my_zarr_store I wanted to use it would be something like ZarrSource(filename="placeholder_filename", datasets=..., store=my_zarr_store)?

I think it would be nicer to mimic the open interface where store is a "Optional[Union[BaseStore, MutableMapping, str]]" and we get just rename the filename arg.

Seems like you might have what you want if you just remove the ensure_string helper function around filename.

mzouink commented 1 year ago

I agree with you @pattonw, i just didn't want to brake the existant work and tutorials i will try to do it in a better way but without breaking things

mzouink commented 1 year ago

@pattonw can you please check it

pattonw commented 1 year ago

I think that looks good. could you add some quick tests for reading/writing to different store types. and rebase to target branch v1.3-dev since this is a breaking change

pattonw commented 1 year ago

merged into v1.3-dev