Add test coverage for async slicing of labels #5325
Conversation
|
This is my first time working with Labels. Now that I've dipped my toe in the water a bit and attempted an implementation similar to Images, I realize that much of this code (the vast majority) could be removed by allowing In fact, SO MUCH CODE would be removed that I'm starting to question if I've done this right. |
|
Given discussion on this topic on zulip, I'll be making some major reductions to this PR. Stand by. |
| @@ -83,6 +83,7 @@ class LockableData: | |||
| def __init__(self, data: LayerDataProtocol): | |||
| self.data = data | |||
| self.lock = RLock() | |||
| self.ndim = data.ndim | |||
There was a problem hiding this comment.
Required to avoid ValueError: Input data should be an array-like object, or a sequence of arrays of decreasing size. Got arrays of single shape: (10, 15) inside of layer.guess_multiscale()
There was a problem hiding this comment.
Normally, passing multiscale=False in the layer initializer should be enough to avoid that error. But I'm not against adding this if needed.
There was a problem hiding this comment.
If this is needed, then maybe just make this a derived property that comes from self.data.ndim?
There was a problem hiding this comment.
because of the nonlinear if checking, even setting multiscale=False doesn't avoid this message. I can change it to a property derived from self.data.ndim though.
|
@andy-sweet / @perlman is this in line with what you were thinking? |
Codecov Report
@@ Coverage Diff @@
## main #5325 +/- ##
=======================================
Coverage 91.67% 91.67%
=======================================
Files 582 582
Lines 51073 51087 +14
=======================================
+ Hits 46819 46836 +17
+ Misses 4254 4251 -3
|
|
Looks good so far, but as discussed, I think it makes sense to put this one on hold until we have merged an initial integration PR. The PR is already draft, so no need to change anything. Could be interesting to combine this with a PR that enables async slicing/rendering for some of the labels layer specific functionality (e.g. painting). |
andy-sweet
changed the title
|
@brisvag : I picked this up while doing some PR grooming. It's very small now and just adds a little more test coverage for async slicing. Would appreciate a review! |
| @property | ||
| def ndim(self) -> int: | ||
| # LayerDataProtocol does not have ndim, but this should be equivalent. | ||
| return len(self.data.shape) |
There was a problem hiding this comment.
I guess this protocol is only really used by images and labels? Otherwise this does not really hold true for layers that accept tuples like surface.
There was a problem hiding this comment.
Yes. This change was needed because Labels also needs ndim in the test case here, so (at most) LayerDataProtocol only supports Image right now.
I like the intent of it and it's often useful for prototyping and testing, but would be great to give it a more thought and care. Outside of this PR though ;)
andy-sweet
added
the
ready to merge
* main: (26 commits) Fix some typing in napari.components (napari#6203) Use class name for object that does not have qt name (napari#6222) test: [Automatic] Constraints upgrades: `hypothesis`, `magicgui`, `psygnal`, `tensorstore`, `tifffile`, `tqdm`, `virtualenv` (napari#6143) Replace more np.all( ... = ...) with np.array_equal (napari#6213) remove np.all(... == ...) in test_surface.py (napari#6218) Ensure pandas Series is initialized with a list as data (napari#6226) Stop using temporary directory for store array for paint test (napari#6191) Bugfix: ensure thumbnail represents canvas when multiscale (napari#6200) cleanup np.all(... == ...) from test_points.py (napari#6217) [pre-commit.ci] pre-commit autoupdate (napari#6221) use app-model for file menu (napari#4865) Add tests to cover slicing behavior when changing layers and data (napari#4819) [pre-commit.ci] pre-commit autoupdate (napari#6128) Add test coverage for async slicing of labels (napari#5325) Add collision check when set colors for labels layer (napari#6193) Update "toggle ndview" text (napari#6192) Prevent layer controls buttons changing layout while taking screenshots with flash effect on (napari#6194) Fix typing in napari.utils.perf (napari#6132) Add GUI test coverage for changes to Labels.show_selected_label (napari#5372) Fix types in 'napari.utils.colormaps.categorical_colormap' (napari#6154) ...
Czaki
removed
the
ready to merge
References and relevant issues
Part of #4795.
Description
Adds test coverage to check that the layer slicer handles labels layers appropriately.