Closed tcompa closed 11 months ago
The coverage rate went from 89.89%
to 90.2%
:arrow_up:
The branch rate is 83%
.
98.19%
of new lines are covered.
Great. Let's adapt wording to make clear we're just loading metadata, not image data
Coverage report
The coverage rate went from
89.8%
to89.93%
⬆️ The branch rate is83%
.
96.89%
of new lines are covered. Diff Coverage details (click to unfold)
For some unclear reason this comment is not up-to-date, and the coverage of the new lib_ngff.py
module is at 98% (only three lines are missing, and they are the exception handling in load_NgffWellMeta
).
For some unclear reason this comment is not up-to-date, and the coverage of the new
lib_ngff.py
module is at 98% (only three lines are missing, and they are the exception handling inload_NgffWellMeta
).
The coverage comment is not working as expected, but I checked by hand that there are no visible regressions.
This PR is essentially ready on my side. Its contents is briefly summarized as:
num_levels
and coarsening_xy
parameters from NGFF objects, rather than from metadata
task input .get_axes_names
, extract_zyx_pixel_sizes
and get_acquisition_paths
), that are now methods/properties of the Pydantic models..zattrs
files.Relevant issues closed by this PR:
It is quite a large PR, but at least a high-level review would be very useful - cc @jluethi.
I added a few minor comments on the NGFF pydantic models and some questions on whether some lines should now be moved to a different part of the task
These should all be covered now.
I didn't review the test changes.
That's OK. I'm mostly unit-testing all the new models, and then updated existing tests that were relying on the old helper functions.
All seems good, and we have new issues in-place for parts that we postponed (e.g. #540 or #150). Merging.
close #351 close #501 close #532 close #533
metadata
.lib_ngff.py
lib_zattrs_utils.py
lib_ngff.py
CHANGELOG.md
lib_channels.py
- ref #540The main goal is to extract some ome-ngff-related metadata directly from an ome-zarr, rather than from the
metadata
input parameter (ref #351). This is a clear goal, that can be achieved in multiple ways. The simplest one is to have several helper functions that go and construct a given parameter (e.g.num_levels
) based on the attributes of an OME-Zarr group.With the current PR, I'm proposing a different approach that brings in broader changes. I'm introducing Pydantic models that encode (part of) OME-NGFF specs, and then implementing additional logic as part of these models (via properties or methods), e.g. to extract
num_levels
. Instead of writing the Pydantic models myself, I generated them automatically from the JSON Schema of 0.4 NGFF (via https://github.com/koxudaxi/datamodel-code-generator) and then tweaked them a bit - resulting e.g. in aNgffImage
model - see also note below on effort duplication.If we pursue this route, then most of the functions in
lib_zattrs_utils.py
will become methods of the appropriate NGFF object (e.g.NgffImage
orNgffWell
). The example I already included in the preliminary commits of this PR is that alsoextract_zyx_pixel_sizes
can be re-written in a simpler way as a method ofNgffImage
. Example:Scope of this change
To be clear: this new
lib_image.py
module (to be renamed intolib_ngff.py
as soon as we also include Well or something else) may become an important one, since it will also contribute to defining our compliance/support w.r.t OME-NGFF. Some aspects:multiscale
perimage
). This information would be more centralized if a set of validators were in place for each Well/Image/Plate model...lib_ngff.py
modules would also be one way to simplify support for multiple versions in the future (e.g. by usingNgffImageV04
orNgffImageV05
based on the version stored in the zarr attributes).This PR may also affect:
420 (see also logic introduced in https://github.com/fractal-analytics-platform/fractal-tasks-core/pull/403, for the
extract_zyx_pixel_sizes
function)150
501
A note on effort duplication
The same procedure (transorming at least a subset of the NGFF specs into Python models) is already done elsewhere:
If/when one of these efforts grows and/or becomes the officially sanctioned Pydantic version of OME-NGFF (or something we want to rely on, for whatever reason), then we should consider using their models instead of our "custom" ones. The transition would take place by defining a
FractalNgffImage
class that inherits from the "official" one and adds additional methods/properties that are needed for fractal-tasks-core tasks (e.g it would expose anum_levels
property).Neglecting the different attribute names, the change could be as smooth as in: