-
Notifications
You must be signed in to change notification settings - Fork 47
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
Specify NGFF 0.6-dev in output data #849
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #849 +/- ##
==========================================
+ Coverage 91.95% 91.96% +0.01%
==========================================
Files 47 47
Lines 7293 7304 +11
==========================================
+ Hits 6706 6717 +11
Misses 587 587
|
pass | ||
@property | ||
def version(self) -> str: | ||
return "0.6-dev" |
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.
cc @bogovicj in case defining a specific pre-release version (i.e. dev1 or beta1) would make detecting differences in these upcoming versions more reliable.
See also ongoing conversation around 0.5.2
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.
Actually, going back and looking at the diff output in the description, I'd suggest we not use 0.6.* unless this also implements the Zarr v3 work of 0.5.
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 a good point. Any suggestion on the string to use? Maybe something more verbose like 0.4-dev-transformations
?
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.
Or 0.4-dev-spatialdata
.
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.
- 👍 for starting with
0.4-
- 👍 for having "spatialdata" or similar in it
- you might think about whether or not you have a version field from SD itself
- I defer to you if you want "dev" in it or not (I don't think the validator, e.g., has special behavior for that. @will-moore?)
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.
Thanks @joshmoore.
you might think about whether or not you have a version field from SD itself
The SpatialData is versioned here:spatialdata/src/spatialdata/_io/format.py
Line 233 in a7e9e99
SpatialDataContainerFormats = {
@will-moore I think your "no" answers to "(I don't think the validator, e.g., has special behavior for that. @will-moore?)". Do you have any preference for the 0.4-
string?
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.
So it'll need a little patching to parse the "0.4" out of a longer string if it's going to validate those.
Except this version explicitly does not validate, so perhaps it's enough that it's just not the known "0.4"?
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 can handle 0.4-*
anything is fine.
I tried looking at some spatialdata data in the validator with...
$ ome_zarr view mouse_liver_data.zarr
...looking at the /labels/segmentation_mask/
the validator is complaining that /multiscales/0/coordinateTransformations/0
doesn't have scales
or translation
objects.
Instead you have "input":{"axes":[...]}, "output": {"axes":[...]}, "type": "identity"}
which is heading towards the CoordinateSystems RFC-5 https://ngff.openmicroscopy.org/rfc/5/index.html but not quite the same.
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.
Yes, I will adjust the coordinate transformations metadata together with @melonora in ome-zarr-models-py
.
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 agree, I'd go then with 0.4-dev-spatialdata
and merge this PR.
This PR monkeypatches
ome_zarr.format
to make it possible to use the read/write APIs ofome-zarr-py
, while clarifying which OME-Zarr data version we are building on top.Precisely, we implement the NGFF transformations specification, which, as of today, it is not merged to NGFF main yet. The timeline is to have the specification included in NGFF v0.6, hence from this PR we will write
0.6-dev
instead of0.4
(which was kept for compatibility withome-zarr-py
).All the tests pass, the code for reproducing the documentation and paper notebooks and the data conversions from
spatialdata-sandbox
are currently running.Practically, the PR will not change APIs or usage of the function of the
spatialdata
package and will only modify, on-disk, the version string for images and labels, as shown here (in this example I used Seqfish v2 data).CC @joshmoore