Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

pyramid checking seems to be breaking lazy loading #699

Closed
tlambert03 opened this issue Nov 15, 2019 · 6 comments 路 Fixed by #740 路 May be fixed by #737
Closed

pyramid checking seems to be breaking lazy loading #699

tlambert03 opened this issue Nov 15, 2019 · 6 comments 路 Fixed by #740 路 May be fixed by #737
Milestone

Comments

@tlambert03
Copy link
Member

@tlambert03 tlambert03 commented Nov 15, 2019

馃悰 Bug

I was just trying to familiarize myself with napari's handling of zarr datasets using one of my big light sheet datasets... but it would hang indefinitely each time I added it.

I went back to the example in zarr_nD_image.py, but there too, as i increase the size of the zarr store, napari starts to hang upon napari.view_image(data)... regardless of whether I provide the contrast_limits, rgb or is_pyramid=False arguments. Digging a bit more, it seems that, as of #556, it is impossible to avoid the callchain from Image.__init__() -> get_pyramid_and_rgb() -> is_pyramid() ... and that is_pyramid() call takes forever on a big zarr store.

Is this a regression or is zarr_nD_image.py now invoking napari.view_image incorrectly?

@jni

This comment has been minimized.

Copy link
Contributor

@jni jni commented Nov 15, 2019

@tlambert03 this is a known issue, in that @sofroniewn and I did know about it, but it appears I neglected to create an issue for it! 馃槵 Maybe it was on slack, or a comment on a different issue. Sorry about that!

Anyway, the problem is this line:

size = np.array([np.prod(d.shape) for d in data])

[... for d in data] actually instantiates each d when data is a zarr array.

The workaround is to load the zarr file not with zarr.open, but with da.from_zarr.

The fix is to change that expression to be lazier. Additionally, is_pyramid=False should probably skip the check, but I'm not sure...

@tlambert03

This comment has been minimized.

Copy link
Member Author

@tlambert03 tlambert03 commented Nov 15, 2019

Ok thanks

@sofroniewn

This comment has been minimized.

Copy link
Contributor

@sofroniewn sofroniewn commented Nov 16, 2019

yeah @tlambert03 this is not great ... any suggestions / improvements would be appreciated. I think it's pretty natural for people to do what you did with zarr instead of da.from_zarr so we should do better than we do right now

@sofroniewn sofroniewn added this to the 0.2.0 milestone Nov 16, 2019
@tlambert03

This comment has been minimized.

Copy link
Member Author

@tlambert03 tlambert03 commented Nov 16, 2019

I need to get much more familiar with the pyramid handling (and other general array-handling) parts of napari before I'd be able to make any sort of useful suggestions. But in the short term, would it make sense to change the zarr example to use da.from_zarr?

with napari.gui_qt():
data = zarr.zeros((102_0, 200, 210), chunks=(100, 200, 210))
data[53_0:53_1, 100:110, 110:120] = 1
print(data.shape)
# For big data, we should specify the contrast_limits range, or napari will try
# to find the min and max of the full image.
viewer = napari.view_image(data, contrast_limits=[0, 1], rgb=False)

I also need to look back through #556 to understand why the if pyramid: check was removed in get_pyramid_and_rgb. If the user explicitly passes pyramid=False, why does is_pyramid still need to be called in this line?

@sofroniewn

This comment has been minimized.

Copy link
Contributor

@sofroniewn sofroniewn commented Nov 17, 2019

I guess the calling of is_pyramid was more of an input check, because it leads to this error if things don't match -

raise ValueError(
but maybe that's a bad idea

Agree changing the zarr example to use da.from_zarr is a good idea.

@constantinpape

This comment has been minimized.

Copy link

@constantinpape constantinpape commented Nov 25, 2019

I ran into this issue today as well and this also breaks lazy loading for other lazy input sources like h5py or z5py datasets.
Maybe leave out this check if is_pyramid is not None? This might cause some downstream errors for inputs that are not consistent with the passed pyramid flag though. Or just perform the check for tuple and list inputs? This would cause issues if someone passes a pyramid format as np.object though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can鈥檛 perform that action at this time.