Be more permissive with version 0.5, allowing support for spatial-data ome zarrs

[pennycuda]

Hello! I opened another PR #587 a little over a week ago to support zipped SpatialData zarr stores. After some conversation with @will-moore and @jo-mueller about the image class overhaul, I decided to wait on Jo's PR to be merged and redo my work after.

Jo's PR was merged yesterday, so I am excited to move forward with my changes. This will reintroduce SpatialData support to ome-zarr-py by adding handling to accept the 0.5-dev-spatialdata ome version in SpatialData metadata.

I tested my changes by using SpatialData's read_zarr() method on zipped and unzipped SpatialData zarr stores. The version of read_zarr() that I used is a combination of changes from the soon to be merged PR scverse/spatialdata#1107 and my branch of that PR that accepts ZipStores and works in the new ome-zarr-py image class changes.

Penny Cuda, she/her, HuBMAP Consortium

[jo-mueller]

So...first thought is that I'm not sure whether I'd like to see the version written by the spatial data projectspecial-cased over here.

From what I see the added code

• adds parsing for the coordinateaTransformations` field - I'm on my phone rn so I can't tell whether that's new (will check later)
• Parses some additional metadata into a spatialdata_transforms field? That I find more problematic because the metadata will essentially be unvalidated on both read and write

A way forward I could imagine:

• Replace the if version == "0.5:" further up with if "0.5" in version:. This will make ome-zarr-py accept a version field such as the 0.5-dev-spatialdata
• Remove reading in spatialdata-transforms because that's something an implementation that uses ome-zarr-py can always do outside the from_one_zarr method after reading.

[pennycuda]

Thank you for the feedback @jo-mueller ! I had the same thoughts about the spatialdata version right after I submitted this PR. 🤦🏻‍♀️ I will take in your feedback and push another commit soon. I'm at a conference this week, so I might be delayed in my responses and commits.

[jo-mueller]

Is that conference ELMI, by any chance? ^^

[pennycuda]

No actually ! Today is HuBMAP Consortium's final meeting, and the next two days are Spatial Biology - The New Frontier. Both in Rockville, MD.

[jo-mueller]

Ok. would have been funny :)

[LucaMarconato]

Thanks @pennycuda for the push. I'll write a few notes on the roadmap, on the meaning of the string 0.5-dev-spatialdata (and 0.4-dev-spatialdata), and how to proceed.

spatialdata was created when OME-Zarr 0.4 was out, while needing the support for coordinate transformations (what today we call RFC-5). This was a few years back, and RFC-5 was supposed to be part of the specs within a few months, so we already started building on top of what should have been the 0.5 format (i.e. 0.4 + RFC-5).

Since 0.5 was requiring more time, we started using the string 0.4-dev-spatialdata. The effort for RFC-5 was turning out to be larger than anticipated: when OME-Zarr 0.5 was released it still could not support RFC-5. From our side, we added support for 0.5 in spatialdata, but still kept using RFC-5 (therefore bumping 0.4-dev-spatialdata to 0.5-dev-spatialdata).

Things were moving slowly, but were moving. And from the last year code and specs have started moving much faster! Thanks to @jo-mueller and collaborators the spec is now close to include RFC-5 into OME-Zarr 0.6. The plan from the spatialdata side is to switch to 0.6 as soon as possible. This will cleanup things on the file format and code, and make interoperability easier.

Until then (or to support earlier dataset), what is possible (this is what we implement), is that we read and write the OME-Zarr store as if it was 0.5, and then we manually add the transformations part. RFC-5 has evolved since our implementation, so there are some differences on how the coordinate systems are encoded in 0.5-spatialdata-dev and the current RFC-5. I therefore endorse the suggredstion from Johannes to keep the avoid having project specific (i.e. the string spatialdata) in a diff, but allow proceeding with 0.5 parsing on specialized versions strings that include 0.5. The rest of the parsing logic ideally would live outside the ome-zarr-py.

Some few useful pointers.

• Here you can find a schema for 0.5-dev-spatialdata from @Bisaloo https://github.com/Huber-group-EMBL/SpatialData.validate/tree/main/inst/extdata
• We recently expanded the spatialdata dev team and we are going to share a public roadmap soon.
• The top items of the roadmap are:
  • Create a new repo to document the details of 0.5-dev-spatialdata and future versions. This repo will be entirely about describing the format, no code. Now source of truth for the spatialdata spec is in format of Python version classes, and therefore difficult to read: https://github.com/scverse/spatialdata/blob/d024c0591bb925c81642702db781f113102b9ece/src/spatialdata/_io/format.py.
  • Implement 0.6 support in spatialdata (this is actually what we are working on now) in preparation to the official OME-Zarr 0.6 release, which is imminent.

@pennycuda can you please share the timeline of HuBMAP, hopefully they are aligned and happy to support from the spatialdata side.

[pennycuda]

Hi @LucaMarconato thank you so much for your very helpful response! I am going to work on some updates and push them today hopefully for OME folks to review. I agree that I should handle the spatialdata transformation incompatibilities all on the spatialdata side rather than in ome-zarr-py.

As far as HuBMAP's timeline, we have already begun to zip our SpatialData outputs, so we would like to move as quickly as possible. We would like our outputs to be easily read by users without them having to unzip the stores themselves. We will be processing data through the end of the year if not longer. I am happy to provide any support I can!

cc @mruffalo

[jo-mueller]

Maybe another 2cts from my end: An even better way to check the version would be to do version.startswith("0.x")

[pennycuda]

@jo-mueller good call! I took yours and @LucaMarconato suggestions and pushed a new commit. :) All of the special spatialdata handling is now over in my spatialdata branch.

[jo-mueller]

Hi @pennycuda sorry for the late reply here. Just a few minor changes to add before merging. I think we'll want to make clear in some comments that this sot of loose version checking will only be allowed for 0.4/0.5 kind of metadata. For the other versions (0.1-0.3, >=0.6) I think the stricter version checking is the more sound option.

In the future, I think it will be better if the spatialdata-specific metadata were to be stored in a separate namespace (outside of the ome key?). But my hope is that ome-zarr 0.6 reconciles the differences between the two worlds sufficiently :)

cc @kevinyamauchi

[codecov]

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 86.46%. Comparing base (de247e5) to head (bf45fbd).
⚠️ Report is 55 commits behind head on master.

Files with missing lines	Patch %	Lines
ome_zarr/classes/image.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #594      +/-   ##
==========================================
- Coverage   86.65%   86.46%   -0.19%     
==========================================
  Files          16       16              
  Lines        2316     2365      +49     
==========================================
+ Hits         2007     2045      +38     
- Misses        309      320      +11     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[pennycuda]

Got it @jo-mueller ! Thank you

[jo-mueller]

@pennycuda just two typos then pre-commit will stop complaining and we're done here 👍

[pennycuda]

all done @jo-mueller !! Thanks so much

[jo-mueller]

@pennycuda one more rebase on the master branch, then tests will pass and I'll merge!

[jo-mueller]

@will-moore can you rebase this? I feel like it would be nice to get this in before #605 , which contains some similar changes.

[will-moore]

@jo-mueller I don't think I have the ability to push to this branch/PR. I can only open my own PR (less ideal).

[will-moore]

@jo-mueller I pushed this to #619 with origin/master merged-in (no merge conflicts), if that's useful?
Since that is green etc, we could take that as approval for this PR, and merge this??