Avoid setting corner pixels for empty layers #5423
Conversation
| data_bbox_clipped = np.clip( | ||
| data_bbox_int, displayed_extent[0], displayed_extent[1] | ||
| ) | ||
| corners[:, displayed_axes] = data_bbox_clipped | ||
| self.corner_pixels = corners |
There was a problem hiding this comment.
Alternatively, we could set Layer.corner_pixels to be an empty array in this case, but I'm not sure which dimension would be empty in that case.
Codecov Report
@@ Coverage Diff @@
## main #5423 +/- ##
==========================================
+ Coverage 87.54% 89.10% +1.56%
==========================================
Files 597 597
Lines 50610 50612 +2
==========================================
+ Hits 44304 45096 +792
+ Misses 6306 5516 -790
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| # contains nans, in which case the integer valued corner pixels | ||
| # cannot be meaningfully set. | ||
| displayed_extent = self.extent.data[:, displayed_axes] | ||
| if not np.all(np.isnan(displayed_extent)): |
There was a problem hiding this comment.
Ideally, I'd want to have something like if self.is_empty() here instead. The existing code effectively serves as a proxy for that and is deliberately distinct from checking if there any nans in displayed_extent (which will also cause the same warning to be issued).
There was a problem hiding this comment.
| if not np.all(np.isnan(displayed_extent)): | |
| if not np.any(np.isnan(displayed_extent)): |
I think that the existence nan on nay position should prevent this.
There was a problem hiding this comment.
I did consider this as a more general fix (i.e. to prevent the warning), but my concern is that it might hide other bugs.
Like I mentioned above, I really want something like Layer.is_empty() in which case layer extent and corner pixels are not well defined. If we have one nan in the layer extent (i.e. instead of all nans), something probably went very wrong. Though in that case, I'd probably expect worse things to happen, so I'd also be fine with this suggestion.
|
I agree it would be helpful to have some indicator the layer is empty. For what it's worth Points and some other layers use Maybe |
|
I can pick this up again in a few hours, but if someone wants to get I think we can follow up with extra things (like |
|
Thanks for the approval! I'm going to make an exception to the typical process of waiting at least 24 hours here, in order to get main and PR tests passing again. I will also write up an issue to capture some of the other ideas/fixes. |
|
This pull request has been mentioned on Image.sc Forum. There might be relevant details there: |
Description
This avoids trying to set
Layer.corner_pixels(which should be integer indices in the data space of the layer) when a layer is empty. For all non-image layers, this causes a warning to be raised from numpy (see #5420) becauseLayer._data_extentis a (2,Layer.ndim) array of all nans. For image layers, ifLayer.datais empty (which seems much less likely) we get all kinds of other errors before even reaching this warning.Type of change
References
Closes #5420
How has this been tested?