fix: update OME-Zarr unit handling to match spec#907
fix: update OME-Zarr unit handling to match spec#907omdowley wants to merge 3 commits intogoogle:masterfrom
Conversation
As discussed in google#780, the OME-Zarr v0.5 spec specifies that the units SHOULD be one of those that are currently allowed, not MUST. This instead treats unsupported units as unitless and warns.
|
I'm wondering if this should be visible in the UI as either a temporary status message or ideally as a message exactly where the error message currently shows but still allow creating the layer |
|
Yes, I'd agree, we just need to add a mechanism for data sources to propagate warnings --- probably by passing in a |
The OME Zarr spec specifies the lists of units from which valid units SHOULD come separately for `time` and `space` axes. This enforces that (with a warning).
This definitely makes a lot more sense than what I initially did.
I've done this -- it ended up a bit messy because by default the warnings were removed pretty immediately by the refCounted disposer, so I had to persist them. Again there may be a neater way to do that, it just felt like it'd require bigger changes, open to feedback I've also added logic checking the axis type and unit type make sense, and warning if there is either an axis type mismatch or if the axis type isn't one defined in the spec (both of these are SHOULD items in the spec) |
| outputSpace: volume.modelSpace, | ||
| transform: multiscaleInfo.baseTransform, | ||
| }; | ||
| const warnings = Array.from(localMessages, (m) => m.message); |
There was a problem hiding this comment.
Can we simplify this to Array.from(localMessages) to avoid having to recreate the message when adding it to options.messages. In that case i would rename warnings to messages.
Also, I think we should change Message from a class to an interface.
| .then((source: DataSourceWithRedirectInfo) => { | ||
| if (refCounted.wasDisposed) return; | ||
| this.messages.clearMessages(); | ||
| refCounted.registerDisposer(this.messages.addChild(dataSourceMessages)); |
There was a problem hiding this comment.
I think we could move refCounted.registerDisposer(this.messages.addChild(dataSourceMessages)); to right after the creation of the message list. Change the error message in the catch handler to add to dataSourceMessages. Then remove the calls to clearMessages in both handlers as the disposer now is now in charge of that. All messages are added to a child instead of the parent MessageList.
3 participants
As discussed in #780, the relevant part of the OME Zarr v0.5 spec specifies that the units SHOULD be one of those that are currently allowed, not that they MUST be (which is what is currently enforced). This instead of erroring treats unsupported units as unitless and warns.
It seems possible to me that there are smarter things to do than this, but thought I'd get the ball rolling with it, because it feels like the absolute minimal change such that we're actually meeting the spec.