Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Axes v0.4 #93

Merged
merged 16 commits into from Jan 31, 2022
Merged

Axes v0.4 #93

merged 16 commits into from Jan 31, 2022

Conversation

will-moore
Copy link
Member

@will-moore will-moore commented Oct 28, 2021

Starting to implement Axes and Transformations (see ome/ngff#57).
cc @constantinpape

This requires https://pypi.org/project/ome-zarr/0.3a1/ for mask writing and new HCS metadata (row/column Indexes).

To test:

$ omero zarr export Image:6001240     # /6001240.zarr/.zattrs  should have new axes (see below)
$ omero zarr masks Image:6001240      # /6001240.zarr/labels/0/.zattrs should also have new axes

$ omero zarr export Plate:1234           # 1234.zarr/.zattrs each 'well' should have rowIndex and columnIndex

Some data exported with this PR can be viewed with vizarr:
https://deploy-preview-139--vizarr.netlify.app/?source=https://minio-dev.openmicroscopy.org/idr/v0.4/2022-01-05/idr0062/6001240.zarr (with PR at hms-dbmi/vizarr#139)

Plate data: https://minio-dev.openmicroscopy.org/idr/v0.4/2022-01-24/plates/idr0004/1752.zarr

Exported NGFF includes:

    "axes": [
                {
                    "name": "c",
                    "type": "channel"
                },
                {
                    "name": "z",
                    "type": "space",
                    "units": "micrometer"
                },
                {
                    "name": "y",
                    "type": "space",
                    "units": "micrometer"
                },
                {
                    "name": "x",
                    "type": "space",
                    "units": "micrometer"
                }
            ]

and each dataset has transformations. E.g. czyx image is scaled along axis z, y, x or [1, 2, 3]:
NB: this format is still being discussed at ome/ngff#57 (review)

                  "transformations": [
                        {
                            "axisIndices": [
                                1,
                                2,
                                3
                            ],
                            "scale": [
                                0.5002025531914894,
                                0.3603981534640209,
                                0.3603981534640209
                            ],
                            "type": "scale"
                        }
                    ]

@constantinpape
Copy link

For me installation of cfunits via conda install -c conda-forge cfunits went smooth and without any issues. What system did you try this on? Maybe we can raise this issue with the conda-forge maintainers of cfunits / uduints2.

@will-moore
Copy link
Member Author

Hah! OK, it worked this time (on an aging Mac 10.14).
But I still don't know if I want to make this a dependency. I might try to validate all the units that we get from OMERO for length (and if not valid then map to a valid unit).

It seems that a lot of things are valid (see below), so if the spec allows all of these, then it becomes very hard to read OME-NGFF data without having cfunits installed. If the spec were more restrictive, then we might not have to make cfunits a dependency of napari-ome-zarr, which would be nice. I'll add a comment to the spec PR...

>>> Units("micrometer").isvalid
True
>>> Units("micrometre").isvalid
True
>>> Units("micrometres").isvalid
True
>>> Units("micrometress").isvalid
False
>>> Units("micrometres")
<Units: micrometres>
>>> Units("micrometre")
<Units: micrometre>
>>> Units("nm")
<Units: nm>
>>> Units("nm").isvalid
True
>>> Units("nanom").isvalid
True
>>> Units("nanome").isvalid
True
>>> Units("nanomet").isvalid
False
>>> Units("nanometer").isvalid
True
>>> Units("nanometre").isvalid
True
>>> Units("nanometre").formatted()
'1e-09 m'
>>> Units("nanome").formatted()
'1.602176487e-31 C'
>>> Units("nanome").units
'nanome'
>>> Units("nanom").units
'nanom'
>>> Units("nano").units
'nano'
>>> Units("nano").formatted()
>>> Units("nanom").formatted()
'1e-09 m'
>>> Units("nanometer").formatted()
'1e-09 m'
>>> Units("nanometre").formatted()
'1e-09 m'
>>> Units("nanom").formatted()
'1e-09 m'

@will-moore will-moore marked this pull request as ready for review January 6, 2022 14:53
@will-moore
Copy link
Member Author

Plate data exported today with current state of this branch and https://pypi.org/project/ome-zarr/0.3a1/
https://minio-dev.openmicroscopy.org/idr/v0.4/2022-01-24/plates/idr0004/1752.zarr
Screenshot 2022-01-24 at 11 49 10

Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few API comments. I have not carried out any functional testing yet

src/omero_zarr/__init__.py Outdated Show resolved Hide resolved
src/omero_zarr/masks.py Outdated Show resolved Hide resolved
src/omero_zarr/raw_pixels.py Outdated Show resolved Hide resolved
src/omero_zarr/raw_pixels.py Show resolved Hide resolved
src/omero_zarr/util.py Outdated Show resolved Hide resolved
@@ -91,6 +91,12 @@ def planeGen() -> np.ndarray:
cache_file_name_func=get_cache_filename,
)

metadata = marshal_axes(image, len(paths))

write_multiscales_metadata(parent, paths, **metadata)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I see this API (introduced in ome/ome-zarr-py#124) in action, I am slightly confused. I think this is primarily because transformations is generated from the knowledge of paths above and then later re-zipped with paths when writing the metadata.

An alternative workflow would be to support datasets either as lists of string or dictionaries in write_multiscales_metadata.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps slightly related to my thoughts in ome/ome-zarr-py#161 (review)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since datasets would include paths, we would just have write_multiscales_metadata(group, datasets) and remove support for paths? Or if datasets is a list of strings, treat it as paths (wouldn't break the API, but could be more confusing).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar questions and concerns around the usability came up in ome/ome-zarr-py#157 when adding support for plate.wells as list of dictionaries alongside list of strings (also corresponding to individual well paths).

At least, I find List[Union[str, dict]] to be a fairly good compromise that allows to handle both the simple use case where a minimal valid spec is generated from a list of paths and the extensible scenario where the caller wants to store additional metadata (and not necessarily specified by the OME-NGFF spec) to the datasets element.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also wonder about the API for reading the transformations in ome-zarr-py. These are currently a 2D list in the node.metadata (not in the form of datasets) since paths are not returned by the reader:

            paths = [d["path"] for d in datasets]
            self.datasets: List[str] = paths
            transformations = [d.get("transformations") for d in datasets]
            if any(trans is not None for trans in transformations):
                node.metadata["transformations"] = transformations

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sbesson
Copy link
Member

sbesson commented Jan 25, 2022

In addition to the various comments above, following this morning's discussion, I have been thinking of ways to exercise the translation transformation that is being introduced in ome/ngff#57.

In the OMERO -> OME-NGFF workflow, a potential candidate might be the WellSample.PositionX and WellSample.PositionY i.e. within the context of a well specification have the individual multiscales images registering the X/Y position relative to some well origin so that a client would be able to display a well grid using the absolute positions.

Unless this is something straightforward to implement, I would definitely consider this outside the scope of this PR and limit the transformations support to scale for now. The primary reason for bringing it up is that it might something to keep in mind alongside the API questions raised in #93 (comment). Since this translation metadata would be not be available from the ImageWrapper itself and would either need to be passed to add_image or be added via a follow-up call which would update the metadata.

@sbesson
Copy link
Member

sbesson commented Jan 25, 2022

Trying to use the latest version of this PR, I get

(zarr) [sbesson@pilot-zarr1-dev ~]$ omero zarr export Plate:9512 --help

Error loading: /home/sbesson/miniconda3/envs/zarr/lib/python3.9/site-packages/omero/plugins/zarr.py
Traceback (most recent call last):
  File "/home/sbesson/miniconda3/envs/zarr/lib/python3.9/site-packages/omero/cli.py", line 1690, in loadpath
    execfile(str(pathobj), loc)
  File "/home/sbesson/miniconda3/envs/zarr/lib/python3.9/site-packages/past/builtins/misc.py", line 87, in execfile
    exec_(code, myglobals, mylocals)
  File "/home/sbesson/miniconda3/envs/zarr/lib/python3.9/site-packages/omero/plugins/zarr.py", line 1, in <module>
    from omero_zarr.cli import HELP, ZarrControl
  File "/home/sbesson/miniconda3/envs/zarr/lib/python3.9/site-packages/omero_zarr/cli.py", line 15, in <module>
    from .raw_pixels import (
  File "/home/sbesson/miniconda3/envs/zarr/lib/python3.9/site-packages/omero_zarr/raw_pixels.py", line 10, in <module>
    from ome_zarr.writer import (
ImportError: cannot import name 'write_multiscales_metadata' from 'ome_zarr.writer' (/home/sbesson/miniconda3/envs/zarr/lib/python3.9/site-packages/ome_zarr/writer.py)

Should the ome-zarr-py dependency be upgraded similarly to ome/napari-ome-zarr@dc4f0bd?

@will-moore
Copy link
Member Author

@sbesson Fixed ome-zarr requirement.

@will-moore
Copy link
Member Author

will-moore commented Jan 25, 2022

That last commit de7703a uses PR: ome/ome-zarr-py#162

@will-moore
Copy link
Member Author

@joshmoore @sbesson I guess we probably don't want that last commit if this PR goes into a pre-release of omero-cli-zarr that works with ome-zarr>=0.3a1?
So, I can revert that and then we can make those changes in a follow-up PR?
I also have a commit locally that uses "scale": {"x": 0.5, "y":0.5: "z": 0.3} format and I guess that should go into a later PR too (so that the pre-releases are all aligned on the axisIndices format?

@sbesson
Copy link
Member

sbesson commented Jan 26, 2022

Yes 👍 for getting a pre-release of omero-cli-zarr with ome-zarr-py 0.3.0a that matches the current proposal and focus on the new writing API and new variant for scale as a follow-up set of pre-releases.

@will-moore
Copy link
Member Author

Reverted de7703a

@will-moore will-moore mentioned this pull request Jan 26, 2022
Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The most important concerns above have been addressed and new PRs have been opened upstream to introduce new metadata writing API. No objection from my side to getting this merged and releases as 0.3.0a1. @joshmoore is there anything from your side that should be captured as an issue and addressed in the next development segment?

@will-moore
Copy link
Member Author

As discussed today, a screenshot illustrating the use of different names for Z/T axes and how it looks in Vizarr:
cc @joshmoore @sbesson @constantinpape

Screenshot 2022-01-26 at 22 39 39

@constantinpape
Copy link

Thanks @will-moore. Is this the dataset from https://minio-dev.openmicroscopy.org/idr/v0.4/2022-01-05/idr0062/6001240.zarr?
I can open that one with Fiji/MoBIE now, but I am not sure if it's quite correct yet:
image

@will-moore
Copy link
Member Author

Looking at https://deploy-preview-139--vizarr.netlify.app/?source=https://minio-dev.openmicroscopy.org/idr/v0.4/2022-01-05/idr0062/6001240.zarr (use Firefox - bugs on Chrome just now).
The .zattrs has:

"datasets": [
                {
                    "path": "0",
                    "transformations": [
                        {
                            "axisIndices": [
                                1,
                                2,
                                3
                            ],
                            "scale": [
                                0.5002025531914894,
                                0.3603981534640209,
                                0.3603981534640209
                            ],
                            "type": "scale"
                        }
                    ]
                },

which should mean it applies to both channels? It looks like your example has different transforms applied to the 2 channels?

@sbesson
Copy link
Member

sbesson commented Jan 31, 2022

As discussed this morning, merging and tagging this as 0.3.0a1 alongside all the other Python libraries pre-releases created ahead of the NGFF community call.
Ongoing work is happening in #103 to support the latest proposed changes to the 0.4 specification and the conversation on how to validate transformations started above can happen there.

@sbesson sbesson merged commit ab56eca into ome:master Jan 31, 2022
@sbesson sbesson added this to Done in OME-NGFF 0.4 Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants