Validate axes types v0.4 #124

will-moore · 2021-10-28T16:05:55Z

This builds on top of #123 to add validation of new axes dicts, based on the axes types.

Based on spec at ome/ngff#57 (not merged yet).

There is a fair bit of validation code here cc @glyg

Breaking changes:

reader axes now returns a list of dicts instead of list of str
OME-Zarr data is written as v0.4

But the writing API isn't breaking. Anything that was valid for v0.3 is also valid for v0.4, since we automatically add type info for tczyx dimensions.

codecov · 2021-10-28T16:15:11Z

Codecov Report

Merging #124 (d9a44f7) into master (2ed4426) will increase coverage by 1.04%.
The diff coverage is 96.36%.

@@            Coverage Diff             @@
##           master     #124      +/-   ##
==========================================
+ Coverage   80.49%   81.53%   +1.04%     
==========================================
  Files          11       12       +1     
  Lines        1174     1251      +77     
==========================================
+ Hits          945     1020      +75     
- Misses        229      231       +2

Impacted Files	Coverage Δ
ome_zarr/format.py	`93.97% <90.90%> (-0.55%)`	⬇️
ome_zarr/axes.py	`95.52% <95.52%> (ø)`
ome_zarr/reader.py	`84.18% <100.00%> (+0.52%)`	⬆️
ome_zarr/writer.py	`93.91% <100.00%> (+0.47%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ed4426...d9a44f7. Read the comment docs.

glyg · 2021-10-28T16:44:13Z

Hi @will-moore sorry I'm not more responsive - I'm out of work till Tuesday, will dig in all that then!

will-moore · 2021-10-28T20:46:43Z

@glyg No problem. No hurry - just pinging you in case it helps avoid any duplicate effort.

ome_zarr/writer.py

ome_zarr/reader.py

ome_zarr/axes.py

sbesson · 2022-01-10T15:43:32Z

ome_zarr/reader.py

+            axes = multiscales[0].get("axes")
+            fmt = format_from_version(version)
+            # Raises ValueError if not valid
+            validate_axes(None, axes, fmt)


Since we are not consuming the return values of validate_axes, the goal here is "only" to validate an axes but not modify it ?

Ah - good point. Looking at this again, I realise that validate_axes(None, axes, fmt) is really designed for writing. So, what's returned will be valid, even if what it's passed isn't valid. E.g. it will convert "tczyx" to an axis array, and will allow v0.3 to have None if 2D or 5D.
What we really want here is Axes(axes, fmt).validate()

sbesson · 2022-01-10T15:50:43Z

ome_zarr/reader.py

-            axes = tuple(multiscales[0].get("axes", ["t", "c", "z", "y", "x"]))
-            if len(set(axes) - axes_values) > 0:
-                raise RuntimeError(f"Invalid axes names: {set(axes) - axes_values}")
+            axes = multiscales[0].get("axes")


What's the main implication of not setting a default value here?
For 0.1/0.2 data, this means, the node.metadata["axes"] might be None as opposed to ["t", "c", "z", "y", "x"] previously i.e. we are preserving the value stored in the metadata? Is there an impact on clients relying on node.metadata["axes"]?

sbesson · 2022-01-10T15:52:07Z

ome_zarr/writer.py

-    ndim: int, axes: Union[str, List[str]] = None, fmt: Format = CurrentFormat()
-) -> Union[None, List[str]]:
-    """Returns validated list of axes names or raise exception if invalid"""
+def validate_axes(


At least I find the following lines

axes_obj = Axes(axes) axes_obj.validate(fmt)

clarifies a lot of the logic happening here. It brings the question of whether additional logic should be moved to the constructor. Said otherwise, what is the added value of calling the validate_axes API vs the two-liner:

axes = Axes(axes, fmt=fmt, ndim=ddim) axes.validate()

What's the outcome of #124 (comment). Should the name of the method be updated to reflect this is a writer/constructor rather than a validator? Since this API is moved to be a public API, this is increasingly important. Alternatively, we can keep it prefixed with _ for now.

ome_zarr/reader.py

ome_zarr/axes.py

…_v0.4

sbesson

A few additional comments mostly revolving around API names and behaviors. But I think the bulk of the work looks good

As discussed this morning, we will need to get this consumed by downstream clients e.g. omero-cli-zarr and/or napari-ome-zarr. To respect the fact the 0.4 specification is not yet published, I don't think this can land into a public release of ome-zarr-py. Instead, can we try to make the minimal decision to target a 0.3 pre-release?

ome_zarr/writer.py

sbesson · 2022-01-13T10:21:07Z

ome_zarr/writer.py

-    ndim: int, axes: Union[str, List[str]] = None, fmt: Format = CurrentFormat()
-) -> Union[None, List[str]]:
-    """Returns validated list of axes names or raise exception if invalid"""
+def validate_axes(


What's the outcome of #124 (comment). Should the name of the method be updated to reflect this is a writer/constructor rather than a validator? Since this API is moved to be a public API, this is increasingly important. Alternatively, we can keep it prefixed with _ for now.

ome_zarr/reader.py

joshmoore

Generally looks good. One minor detail question below and one higher-level here: Does anyone have a feeling for when we might want to start adding submodules, e.g. are we approaching a "util" or "internals" with his PR?

joshmoore · 2022-01-13T16:08:59Z

ome_zarr/axes.py

+            axes_names.append(axis["name"])
+        return axes_names
+
+    def _validate_03(self) -> None:


I need to read more here to get my head around the new class, but my immediate reaction to methods that are version specific is that I'd hope they could live in the Format object.

Thanks for the hindsight. At some point, I was also wondering whether Axes03 Axes04 should be introduced to toggle between behaviors. It would be interesting to see how to make use the Format API to handle this as this can be re-used in the future.

I'm trying to think this through... It's possible for everything to become version specific if it changes in a future version, and we wouldn't want the API to change when methods move to a Format object.
Everything in the Axes class is version specific, (e.g. _validate_axes_types will soon become more liberal). So do we move those internal methods to Format objects when they change?
This will mean that eventually all the code for everything will migrate Format objects.
Are the methods of Format objects (e.g. validate_axes()) expected to be part of the external API?
What's the use-case or benefit of all version-specific code being in Format objects?

Trying to think of another example with the 0.4 HCS changes to the wells element discussed in #157, I could certainly imagine migrating _validate_well_images(wells) by fmt.validate_well_images(wells) or similar and let FormatV04 vs FormatV{01,02,03} override the methods as appropriate.

sbesson · 2022-01-18T08:25:58Z

As discussed yesterday, merging to move forward towards a pre-release of ome-zarr-py 0.3.0 and we can capture some of the outstanding comments as issues.

will-moore force-pushed the validate_axes_types_v0.4 branch from 6052098 to 478a4e7 Compare December 17, 2021 22:55

will-moore added 4 commits December 17, 2021 22:58

Add new current version 0.4

9250136

writer validates axes for 0.4 version

6557b26

reader handles axes as None, List of str or List of dict

19a320c

Add tests

a2b3b3e

will-moore force-pushed the validate_axes_types_v0.4 branch from 478a4e7 to a2b3b3e Compare December 17, 2021 22:58

will-moore added 4 commits January 5, 2022 10:17

Fix logging of axes names

02342b6

Don't reassign axes - fix mypy

939295c

Check for axes is None - fix mypy

789b95b

Fix logging of axes names for v0.4 and v0.3

4097ece

will-moore mentioned this pull request Jan 5, 2022

Axes v0.4 ome/omero-cli-zarr#93

Merged

sbesson reviewed Jan 6, 2022

View reviewed changes

ome_zarr/writer.py Outdated Show resolved Hide resolved

ome_zarr/writer.py Outdated Show resolved Hide resolved

ome_zarr/reader.py Outdated Show resolved Hide resolved

will-moore added 2 commits January 6, 2022 17:04

Use validate_axes() in reader.py

bd16dfd

Move axes logic to new Axes class

9f75ae3

sbesson reviewed Jan 10, 2022

View reviewed changes

will-moore added 3 commits January 12, 2022 10:05

Fix various points from Seb

18608d9

Merge remote-tracking branch 'origin/master' into validate_axes_types…

a580788

…_v0.4

Rename get_axes() -> to_list()

7147bbd

sbesson reviewed Jan 13, 2022

View reviewed changes

joshmoore reviewed Jan 13, 2022

View reviewed changes

will-moore added 3 commits January 14, 2022 09:58

Rename writer.validate_axes() to _get_valid_axes()

8e37f31

Axes() constructor also calls self.validate()

3037b33

Handle transformations in reader and writer. Add test

5607786

sbesson mentioned this pull request Jan 14, 2022

Fix remaining assumptions on 5D dimensions #148

Merged

reader node.metadata['axes'] in latest format

d9a44f7

sbesson mentioned this pull request Jan 17, 2022

Add support for passing wells as List[dict] in write_plate_metadata #157

Merged

sbesson merged commit 73aee67 into ome:master Jan 18, 2022

will-moore mentioned this pull request Jan 19, 2022

v0.4 axes transforms ome/napari-ome-zarr#31

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate axes types v0.4 #124

Validate axes types v0.4 #124

will-moore commented Oct 28, 2021 •

edited

Loading

codecov bot commented Oct 28, 2021 •

edited

Loading

glyg commented Oct 28, 2021

will-moore commented Oct 28, 2021

sbesson Jan 10, 2022

will-moore Jan 12, 2022

sbesson Jan 10, 2022

sbesson Jan 10, 2022

sbesson Jan 13, 2022

sbesson left a comment

sbesson Jan 13, 2022

joshmoore left a comment

joshmoore Jan 13, 2022

sbesson Jan 13, 2022

will-moore Jan 14, 2022

sbesson Jan 14, 2022

sbesson commented Jan 18, 2022

Validate axes types v0.4 #124

Validate axes types v0.4 #124

Conversation

will-moore commented Oct 28, 2021 • edited Loading

codecov bot commented Oct 28, 2021 • edited Loading

Codecov Report

glyg commented Oct 28, 2021

will-moore commented Oct 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbesson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshmoore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbesson commented Jan 18, 2022

will-moore commented Oct 28, 2021 •

edited

Loading

codecov bot commented Oct 28, 2021 •

edited

Loading