- Notifications
You must be signed in to change notification settings - Fork61
bioformats2raw metadata support#174
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecovbot commentedMar 2, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@## master #174 +/- ##==========================================- Coverage 84.79% 83.90% -0.89%========================================== Files 13 14 +1 Lines 1473 1591 +118 ==========================================+ Hits 1249 1335 +86- Misses 224 256 +32
☔ View full report in Codecov by Sentry. |
Pointing ome_zarr at a non-root node will now walk-up thehierarchy until the root node is discovered.
This allows the definition of node specs outside ofthis repository. Primary driver is for experimentingwith specs which depend on ome_types, etc. These mayeventually be pulled into the mainline.
Seehttps://github.com/ome/ome-zarr-metadata/releases/tag/0.1.0 for an example of an entrypoint. After creating a fake .zgroup under the output of Notice:
|
| found.append(Well(self)) | ||
| self.specs.append(found[-1]) | ||
| for key, value in entrypoints.get_group_named("ome_zarr.spec").items(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What does this do? I see no file named"ome_zarr.spec" and I've not usedentrypoints before. I don't get much idea fromhttps://github.com/takluyver/entrypoints as to what it does, only that "This package is in maintenance-only mode. New code should use theimportlib.metadata module".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What does this do?
It's essentially a namespace for the particular entrypoint. It's used at
https://github.com/ome/ome-zarr-metadata/blob/main/setup.cfg#L81
For an Image in a Plate (12 Wells A-C, 1-4, all wells with labels), without this PR I get: and with this PR I get all the sibling Without this PR,
With this PR, I get everything as for 'info' above: All the |
joshmoore commentedApr 13, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@will-moore, you approve of getting the siblings (assuming napari can be fixed)? If so, I'll look why it's only for the one row. Also, where do you get the labels for this plate? Is this the one you had a script for? |
@will-moore, I've reverted the upwards parsing. It seemed like a good strategy but there are currently too many edge cases. I don't have labels on plates for testing at the moment, but I think with the current state along withome/ome-zarr-metadata@08e12f7#diff-0bb17e0ecb4ac83835ee3800a1af71a12f644b0ce782c623ba97f8917916250eR54 all the following should be true:
The only other change I can think of is if you pass a group that previously did nothing, it will likely try to load the contents. |
In discussing today with@dgault,@sbesson,@jburel and@melissalinkert, there was a case made for at least adding the flag ( |
To improve the codecov results, seehttps://github.com/zarr-developers/numcodecs/pull/300/files#diff-bc37cd9860eec1facdc18a47798e8a1a2c0ef5dabd999deee049de4a48a5d35fR1 for an option of in-repo testing of entrypoints. |
@joshmoore To help address the "don't have labels on plates for testing", I createdhttps://gist.github.com/will-moore/0f4cb6b1fdd60a255fcbb956a54a645e which adds labels to a plate (currently assumes images axes are I don't know if I'm missing something, maybe not using |
fd64b1d to9ad2e5aComparesee a quick use of this functionality: |
Migrated the bf2raw implementation fromhttps://github.com/ome/ome-zarr-metadata : |
Initially prepared as a plugin inhttps://github.com/ome/ome-zarr-metadatathis is being moved to the main repository since it is a part of themain spec.
| _logger.info("found %s", series) | ||
| subnode = node.add(series) | ||
| if subnode: | ||
| subnode.metadata["ome-xml:index"] = idx |
joshmooreSep 15, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is probably the biggest lingering question I have since this basically becomes public API e.g. used innapari-ome-zarr. Thoughts welcome. cc:@will-moore@sbessonet al.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I wonder where this public API is documented?
This new module should be added tohttps://github.com/ome/ome-zarr-py/blob/master/docs/source/api.rst but I think these changes in parsing also need to be described elsewhere, maybe athttps://ome-zarr.readthedocs.io/en/stable/python.html#reading-ome-ngff-images
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Big 👍 but first do you think what's there is usable, understandable, extensible, etc.?
| subnode = node.add(series) | ||
| if subnode: | ||
| subnode.metadata["ome-xml:index"] = idx | ||
| subnode.metadata["ome-xml:image"] = image |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If you add the following here, then you don't needome/napari-ome-zarr#47
subnode.metadata["name"] = image.nameThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
But then plugins would/could overwrite the main metadata, right? Which means it becomes a question of the order that the plugins run in. Would:
subnode.plugins["bioformats2raw"].metadata["name"]be better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As probably transpired from#140, I am also unclear on the contract and expectation for thenode.metadata field. My impression was that this API has been largely driven by the napari use-case.Reading this, it looks like another goal is to introduce some form of metadata consolidation so that a consumer does not have to go up and down the hierarchy to assemble pieces of metadata, is that correct?
If so, I agree what is missing is some form of namespace to differentiate the metadata injected at different levels in the hierarchy. Taking advantage of the fact this is a Python dictonary, another proposal would be to group all the metadata under a top-level key e.g.subnode.metadata["bioformats2raw"] = {"index": idx, "image": image", "name", image.name}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
.Reading this, it looks like another goal is to introduce some form of metadata consolidation so that a consumer does not have to go up and down the hierarchy to assemble pieces of metadata, is that correct?
Maybe. But now that we have the json-schema, I could also see just attaching objects at the right node level to prevent re-reading the file. That's essentially what would happen if the ome-typesOME object becomes part of the bioformats2raw plugin's public API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In my mind I might expect something like Josh's original idea orsubnode.metadata["ome.xml"] = {"index": idx, "image": image", "name", image.name} since this is metadata coming from theome.xml?
The reason I likesubnode.metadata["name"] = image.name is that any consumer doesn't have to know aboutbioformats2raw or xml etc to have the correct image name.
But if it's justnapari-ome-zarr and we already haveome/napari-ome-zarr#47 then 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
is that any consumer doesn't have to know about bioformats2raw or xml etc to have the correct image name.
This possibly gets back to the question of whether or not this bf2raw parsing is core or not. If it is, then you're probably right that we should just encode the rules for what "wins" directly. If this is a plugin, though, then how would we handle misbehavior in the plugins?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What is a plugin here? If it's a plugin, then it's not installed by default and you get to choose if you want to add it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This possibly gets back to the question of whether or not this bf2raw parsing is core or not
Trying to answer this, this is where I would draw the line:
- if a specification is published inhttps://ngff.openmicroscopy.org/, I consider it as
core(even iftransitional) and my expectation is that this library should support it - a contrario, any specificationnot defined in the upstream specification should be seen as an extension and should be handled by some plugin/third-party mechanism. This might drive the discussions on the extensibility of this library which is a good thing.
Should we also discuss the name of the module itself? |
We added an |
Other than the string |
Ah - yes, too late to change the |
imagesc-bot commentedSep 28, 2022
This pull request has been mentioned onImage.sc Forum. There might be relevant details there: https://forum.image.sc/t/intermission-ome-ngff-0-4-1-bioformats2raw-0-5-0-et-al/72214/1 |
imagesc-bot commentedAug 30, 2023
This pull request has been mentioned onImage.sc Forum. There might be relevant details there: https://forum.image.sc/t/saving-volumetric-data-with-voxel-size-colormap-annotations/85537/24 |
imagesc-bot commentedOct 4, 2024
This pull request has been mentioned onImage.sc Forum. There might be relevant details there: https://forum.image.sc/t/constructing-zarr-for-napari-ome-zarr/65398/4 |
Uh oh!
There was an error while loading.Please reload this page.
Part of the investigation of metadata inome/ngff#104. This "implicit" group is the cheapest form of collection imaginable.
Currently, onlygroups within the given group (and not arrays or explicit files) will be further parsed.