-
Notifications
You must be signed in to change notification settings - Fork 8
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
Multi-channel OME-Zarr loading #98
Conversation
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.
Hey @tibuch,
I opened the PR so we can discuss along the code. I noticed 2 things, first is trivial in one of the conditions, but the second seems more important, since it looks to me like the dimensions passed to the view setup changed (and the new code makes more sense to me, but since it worked with the old one I am not sure if the change is correct.)
src/main/java/org/embl/mobie/io/ome/zarr/loaders/N5OMEZarrImageLoader.java
Show resolved
Hide resolved
src/main/java/org/embl/mobie/io/ome/zarr/loaders/N5OMEZarrImageLoader.java
Show resolved
Hide resolved
src/main/java/org/embl/mobie/io/ome/zarr/loaders/N5OMEZarrImageLoader.java
Show resolved
Hide resolved
scale[scalesFirstIndexBackward - 1], | ||
scale[scalesFirstIndexBackward - 2] }; | ||
if (zarrAxes.hasChannels()) { | ||
if (zarrAxes.is3DWithChannels() || zarrAxes.is2D()) { |
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 don't think we need the zarrAxes.is2D
here, since this will always have zarrAxes.hasChannels(): false
OmeZarrMultiscales multiscale = setupToMultiscale.get(setupId); | ||
|
||
|
||
FinalDimensions dimensions = getFinalDimensions3D(setupId); |
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 am a bit confused by the code we had here before; it looks like beforehand we always just put the dimensions from the zarr directly, which might be 2 - 5D. Now we always put a 3D shape. The new code seems more reasonable to me, but it has worked in the old version, so I am not sure if this change is correct.
cc @KateMoreva
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.
But what if we have zarrAxes.hasChannels() == false?
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.
Yeah, that's another good point, I think this is missing from above.
@KateMoreva for zarrAxes.hasChannels == false
we have the same behaviour as before, see also my comment above.
But what confuses me is that for zarrAxes.hasChannels == true
, we now have a different behaviour (before we had the full dimensions, now we only have the spatial ones.)
OmeZarrMultiscales multiscale = setupToMultiscale.get(setupId); | ||
|
||
|
||
FinalDimensions dimensions = getFinalDimensions3D(setupId); |
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.
Yeah, that's another good point, I think this is missing from above.
@KateMoreva for zarrAxes.hasChannels == false
we have the same behaviour as before, see also my comment above.
But what confuses me is that for zarrAxes.hasChannels == true
, we now have a different behaviour (before we had the full dimensions, now we only have the spatial ones.)
private FinalDimensions getFinalDimensions3D( int setupId ) { | ||
final DatasetAttributes attributes = setupToAttributes.get(setupId); | ||
if ( zarrAxes != null) { | ||
if ( zarrAxes.hasChannels() ) { |
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 zarrAxes.hasChannels() == false
we will not go into this branch, won't hit any of the returns and just fall back to the 'normal' return in line 442 (so the same behaviour as before)
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 @KateMoreva
@KateMoreva do we already have a |
I checked out this PR and also here I am getting:
for https://s3.embl.de/i2k-2020/ngff-example-data/v0.4/czyx.ome.zarr |
@constantinpape @tibuch
Is that correct? |
Trying this with https://s3.embl.de/i2k-2020/spatial-transcriptomics-example/pos42/images/ome-zarr/MMStack_Pos42.ome.zarr
What data type should that be? |
@constantinpape which data type should the image above have (i.e. when you open it in python)? |
uint16 |
TBH, the error does not sounds like it is related to the multi-channel... |
Here's a version with just one channel: |
As expected, same error. |
I created #107 about this. |
Ok, turns out there were issues with the data; after fixing them (see #107 for details) everything works as expected on MoBIE-beta now: |
Could we close this PR? |
Sure, that also works. (Though in general it's a good idea to learn how to work with forks, it's not really more difficult ;)).
Here's the diff between the two branches: fmi-faim/mobie-io@develop...mobie:mobie-io:multi-channel-fix. So I think we can safely close this PR and continue working from your |
No description provided.