Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #515 +/- ##
==========================================
+ Coverage 86.05% 86.58% +0.53%
==========================================
Files 14 15 +1
Lines 1857 2073 +216
==========================================
+ Hits 1598 1795 +197
- Misses 259 278 +19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@jo-mueller Thanks for that - This looks like a nice approach to separate the metadata creation and manipulation from the writing to zarr. Comparing APIs:
Various comments, questions. I realise some of this is just not implemented yet... And I haven't tried the code (which might answer some of these)...
|
|
@will-moore thanks for the breakdown. I was aware of some of these points (not all) but decided to send it anyway to not go too far in the wrong direction in case there were strong objections to the approach. Actually, what you wrote is an excellent to-do list :) |
|
@will-moore I think this is taking a bit more shape towards how I'd expect it. But before this can continue, I think there is value in discussing first the current duplicity in functionality between these two functions:
There's a lot of overlap between these two functions which I think can be condensed so we'd have a single place where
Anyway, I just tried the |
|
This pull request has been mentioned on Image.sc Forum. There might be relevant details there: https://forum.image.sc/t/separate-tiles-to-ome-zarr/109071/55 |
|
@jo-mueller Could you update the description to reflect where this is heading now? Are we planning to keep all the existing write methods (any API changes)? |
79fdff7 to
8e1ffdf
Compare
fix: pass name to multiscales class feat: add reading/writing fix: do not use v06 yet test: add testcases for class-based writer [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci refactor: write scales to zarr group scale<idx> add image class [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci remove unused ABC fix multiscale creation [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci use dask utils for rescaling [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci more general input coercion ensure `scaleX` naming for levels rely on refactoring changes for pyramid creation & write raise validation error of ndims != data shape [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci docstring pass storage options on to writer function [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci move down group check improve metadata handling change multiscale path formatting split into two different classes compute shapes off actual shapes rather than ideal factors [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci use ome-zarr-models-py v06 [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci add version to export args correct naming in docstring code style remove dead code [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci add docstrings [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci use CoordinateSystem and Axis from ozmp and pop empty fields pass name to multiscales class correct arg passage Create scene.py add support for passing additional transformations through class interface add reading/writing [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci remove class conversion methods add fallback for scales remove v06 objects [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci Delete scene.py fix: make precommit happy [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci fix: typesafety fix: no self-conversion test: add testcases for class-based writer [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci refactor: write scales to zarr group scale<idx> [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci refactor: write scales to zarr group scale<idx> pass name to multiscales class correct arg passage Create scene.py add support for passing additional transformations through class interface add reading/writing [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci remove class conversion methods add fallback for scales remove v06 objects [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci Delete scene.py fix: make precommit happy [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci fix: typesafety fix: no self-conversion test: add testcases for class-based writer [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci refactor: write scales to zarr group scale<idx> [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci
2b38492 to
c67569d
Compare
|
It would be nice to support writing of "omero" metadata. I think that's covered by ome-zarr-models-py too. |
for more information, see https://pre-commit.ci
…/ome-zarr-py into introduce-image-class
Agree.
THAT is a good question I'm not entirely sure of myself. I guess if we want to go this way further, we would ultimately deprecate def write_image(args, kwargs):
image = NGffImage(args, kwargs)
multiscales = NgffMultiscales(image, ....)
multiscales.to_ome_zarr(....)which would at least reduce the amount of code to maintain and make sure that everything we do on the class-based API side is covered well by the already existing tests. What's missing hereThe only thing that makes tests fail here currently is this one: ome-zarr-models/ome-zarr-models-py#398. Locally, all tests are passing. Also, note that this branch has been rebased on #544, so that'll have to go in first, too. |
…/ome-zarr-py into introduce-image-class
for more information, see https://pre-commit.ci
…/ome-zarr-py into introduce-image-class
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
[pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci
[pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci chore: remove unused variable
[pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…/ome-zarr-py into introduce-image-class
Hi @will-moore ,
this is an implementation of my idea for a class-based, user-facing API to writing and reading.
Key features
NgffImageandNgffMultiscales(similar to the implementation over at ngff-zarr), that serve as primary entrypoints to the writing.NgffImageaccepts the data to be written as an array and coerces it to dask internally. It requires thedims(i.e.,"tczyx") to be passed at instantiation. It also accepts kwargs for pixel sizes (scale), axes units (axes_units) and the name of the image (name) which are later serialized in the ome-zarr metadata.The
NgffMultiscalesthen constructs a pyramid using the already existing methods (_build_pyramid) that were implemented in deprecate scaler class #516.Metadataclass from there for simpke serialization and de-serialization in the write/read process. Primarily, the coordinate transformation classes and theMultiscalesmetadata classes are used.Importantly, all metadata is internally coerced to
ozmp.v05.Multiscales. Only on writing the metadata class is converted to whatever ome-zarr version is desired.NgffMultiscales.to_ome_zarr()method. This method makes use of the already existing writing API from Streamline writing #531 (_write_pyramid_to_zarr). It then converts the metadata to the chosen version and uses pydantic'sobject.model_dump()to create the metadata dictionary. Importantly, the version conversion is only implemented in implement version converters ome-zarr-models/ome-zarr-models-py#398, so this is currently blocked by that.NgffMultiscalesclass also has an attachedfrom_ome_zarr(...)classmethod. The argument is simply the path/group of the ome-zarr image. The function then reads the metadata and the multiscales as dask arrays and returns an instance ofNgffMultiscales. The version is automatically detected and again coerced toozmp.v05.Multiscalesinternally.NgffMultiscalesand passing them as a single image or as adict(str, NgffMultiscales)to theto_ome_zarrwriter function. I have yet to implement the same functionality for the reading.All in all, I think especially the
to_ome_zarrandfrom_ome__zarrmethods are super convenient. I have written a follow-up implementation of thescenemetadata from 0.6 and making use of the same API there makes a lot of sense. We could think of similar entrypoints to writing HCS layouts.Further considerations
_build_pyramidandwrite_pyramid_to_zarr- I see no harm in keeping the currently existing API around if it works for people. Maybe we'd have to update thewrite_imagefunction so that it would also accept an instance ofNgffImage, otherwise it may be confusing? I'm not so decided on the reading though.TODOs: