Fix loading multi-source zarrs where one source is missing a "C" dimension #364
Conversation
frasercl
requested review from
ShrimpCryptid and
interim17
and removed request for
a team
| let channelCount = 0; | ||
| for (const s of sources) { | ||
| s.channelOffset = channelCount; | ||
| channelCount += s.omeroMetadata?.channels.length ?? s.scaleLevels[0].shape[s.axesTCZYX[1]]; |
There was a problem hiding this comment.
So just to check my understanding, this was failing before because s.axesTCZYX[1] evaluated to -1, which then caused the statement to effectively become:
channelCount += undefined ?? undefinedIs that right? and now in that failure case we instead increment by 1
There was a problem hiding this comment.
Yup! And adding undefined to channelCount is what eventually produced the NaNs in chunk request URLs.
(Vaguely interesting: the NaNs only appeared if the image missing the channel dimension came before the image with it. Configurations with the URLs the opposite way around fell victim to the completely separate bug I fixed further down this file.)
src/loaders/OmeZarrLoader.ts
Outdated
| const level = src.scaleLevels[scaleLevel]; | ||
| const chunkDimsUnordered = level.shape.map((dim, idx) => Math.ceil(dim / level.chunks[idx])); | ||
| return this.orderByTCZYX(chunkDimsUnordered, 1); | ||
| return this.orderByTCZYX(chunkDimsUnordered, 1, idx); |
There was a problem hiding this comment.
What does adding idx to this line do?
There was a problem hiding this comment.
This specifies which source's dimension ordering we want to use. This used to just default to using the topology of the first source for everything. This was bad.
idx is also unnecessarily terse. I've renamed it sourceIndex.
ShrimpCryptid
left a comment
ShrimpCryptid
left a comment
There was a problem hiding this comment.
Thanks for answering my questions! LGTM
Review time: tiny (5min)
Resolves #359: fixes multiple bugs with loading multi-source zarr images where some sources are missing a channel (C) dimension.
This is useful to scientists with a multi-channel "original" image and a generated dataset written to a different zarr. E.g. if the original zarr has two channels and the scientist generates an additional segmentation layer in a different zarr, Vol-E can show it as a single three-channel image. But often, that single-channel segmentation image will be generated without an explicit "channels" dimension entirely (TZYX rather than TCZYX).
This PR fixes a couple spots where
OMEZarrLoadercurrently doesn't account for this case.