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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid possible divide-by-zero in Vectors layer thumbnail update #5192

Merged
merged 1 commit into from Oct 12, 2022

Conversation

aganders3
Copy link
Contributor

Description

Trying to run tests locally I ran into nine tests in layers/vectors/_tests/test_vectors.py tests consistently failing:

FAILED napari/layers/vectors/_tests/test_vectors.py::test_no_args_vectors - RuntimeWarning: divide by zero encountered in divide
FAILED napari/layers/vectors/_tests/test_vectors.py::test_no_data_vectors_with_ndim - RuntimeWarning: divide by zero encountered in divide
FAILED napari/layers/vectors/_tests/test_vectors.py::test_empty_vectors - RuntimeWarning: divide by zero encountered in divide
FAILED napari/layers/vectors/_tests/test_vectors.py::test_empty_vectors_with_property_choices - RuntimeWarning: divide by zero encountered in divide
FAILED napari/layers/vectors/_tests/test_vectors.py::test_empty_layer_with_edge_colormap - RuntimeWarning: divide by zero encountered in divide
FAILED napari/layers/vectors/_tests/test_vectors.py::test_empty_layer_with_edge_color_cycle - RuntimeWarning: divide by zero encountered in divide
FAILED napari/layers/vectors/_tests/test_vectors.py::test_no_data_3D_vectors_with_ndim - RuntimeWarning: divide by zero encountered in divide
FAILED napari/layers/vectors/_tests/test_vectors.py::test_empty_3D_vectors - RuntimeWarning: divide by zero encountered in divide
FAILED napari/layers/vectors/_tests/test_vectors.py::test_empty_data_from_tuple - RuntimeWarning: divide by zero encountered in divide

I will spare the repeated output because it was basically the same for each:

_________________________________________ test_no_args_vectors _________________________________________

    def test_no_args_vectors():
        """Test instantiating Vectors layer with no arguments"""
>       layer = Vectors()

napari/layers/vectors/_tests/test_vectors.py:40: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
napari/layers/vectors/vectors.py:248: in __init__
    self._update_dims()
napari/layers/base/base.py:704: in _update_dims
    self.refresh()  # This call is need for invalidate cache of extent in LayerList. If you remove it pleas ad another workaround.
napari/layers/base/base.py:1210: in refresh
    self._update_thumbnail()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <Vectors layer 'Vectors' at 0x287fdd7c0>

    def _update_thumbnail(self):
        """Update thumbnail with current vectors and colors."""
        # calculate min vals for the vertices and pad with 0.5
        # the offset is needed to ensure that the top left corner of the
        # vectors corresponds to the top left corner of the thumbnail
        de = self._extent_data
        offset = (np.array([de[0, d] for d in self._dims_displayed]) + 0.5)[
            -2:
        ]
        # calculate range of values for the vertices and pad with 1
        # padding ensures the entire vector can be represented in the thumbnail
        # without getting clipped
        shape = np.ceil(
            [de[1, d] - de[0, d] + 1 for d in self._dims_displayed]
        ).astype(int)[-2:]
>       zoom_factor = np.divide(self._thumbnail_shape[:2], shape).min()
E       RuntimeWarning: divide by zero encountered in divide

napari/layers/vectors/vectors.py:682: RuntimeWarning

The root of the problem is the undefined behavior in casting a NaN to int because self._extent_data is filled with NaN when the data is empty.

# natively on my Apple Silicon Mac
>>> np.float32("nan").astype(int)
0

# in an (emulated) x86-64 Ubuntu container on my machine
>>> np.float32("nan").astype(int)
-9223372036854775808

It looks like this or something like it was fixed or anticipated for the Shapes layer, so I copied the implementation style from there. I checked some of the other layer types and they look okay as well, though I'm still getting familiar with them.

Type of change

  • Bug-fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

References

See numpy/numpy#14412
I think this is actually set to emit a warning in a coming version of numpy, and potentially raise an error.

How has this been tested?

Vector tests pass with my change on multiple platforms.

Final checklist:

  • My PR is the minimum possible work for the desired functionality
  • I have commented my code, particularly in hard-to-understand areas
  • N/A I have made corresponding changes to the documentation
  • N/A I have added tests that prove my fix is effective or that my feature works
  • N/A If I included new strings, I have used trans. to make them localizable.
    For more information see our translations guide.

@codecov
Copy link

codecov bot commented Oct 8, 2022

Codecov Report

Merging #5192 (6e9a71a) into main (2566980) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #5192   +/-   ##
=======================================
  Coverage   88.81%   88.81%           
=======================================
  Files         574      574           
  Lines       48910    48912    +2     
=======================================
+ Hits        43441    43443    +2     
  Misses       5469     5469           
Impacted Files Coverage Δ
napari/layers/vectors/vectors.py 97.95% <100.00%> (-0.40%) ⬇️
napari/components/experimental/chunk/_pool.py 85.71% <0.00%> (-7.94%) ⬇️
napari/layers/image/image.py 95.89% <0.00%> (+0.24%) ⬆️
napari/utils/theme.py 89.79% <0.00%> (+0.68%) ⬆️
napari/utils/info.py 83.33% <0.00%> (+1.11%) ⬆️
napari/layers/image/_image_slice.py 100.00% <0.00%> (+2.38%) ⬆️
...layers/image/experimental/_chunked_image_loader.py 96.00% <0.00%> (+8.00%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@aganders3 aganders3 changed the title Fix possible divide-by-zero in Vectors layer thumbnail update Avoid possible divide-by-zero in Vectors layer thumbnail update Oct 8, 2022
@psobolewskiPhD
Copy link
Member

Thanks for addressing this!
Nice detective work as to the real cause and why it passes on CI but crops up on arm64 macs!
We'd wondered about it before:
https://napari.zulipchat.com/#narrow/stream/212875-general/topic/failing.20test.20on.20latest.20main

Tested locally, all vector tests on my M1 pass and
viewer.add_vectors() no longer results in 3 warnings.
👍

For anyone reviewing, the diff is hard to follow, no idea why it's generated like that. Easier to look here perhaps:

def _update_thumbnail(self):
"""Update thumbnail with current vectors and colors."""
# Set the thumbnail to black, opacity 1
colormapped = np.zeros(self._thumbnail_shape)
colormapped[..., 3] = 1
if len(self.data) == 0:
self.thumbnail = colormapped
else:

Those are the added lines, with the two colormapped lines being moved from 699-700. Then the rest is moved into the else: as is, with no changes but the indentation and black formatting.
VS Code does a better job of presenting it:
image

@brisvag
Copy link
Contributor

brisvag commented Oct 10, 2022

Wow, that diff really is bad! Thanks for the screenshot @psobolewskiPhD, that helps a lot :D

@aganders3 nice catch, thanks for the PR! Looks good to me.

@aganders3
Copy link
Contributor Author

Thanks for the reviews, and thanks @psobolewskiPhD for the clarification on the diff!

Copy link
Member

@psobolewskiPhD psobolewskiPhD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good to me.
Edit: sorry didn't know how to do this properly—still learning!

@brisvag
Copy link
Contributor

brisvag commented Oct 12, 2022

@aganders3 we have some merge conflicts, do you want to have a go at them?

* rebase on main, fix conflicts introduced by napari#5003
@aganders3
Copy link
Contributor Author

Thanks for the heads-up. The only difference I found was that self._dims_displayed became self._slice_input.displayed. I will request re-review if/when CI passes.

@aganders3 aganders3 requested review from brisvag and psobolewskiPhD and removed request for brisvag and psobolewskiPhD October 12, 2022 13:42
@brisvag brisvag merged commit 7117d22 into napari:main Oct 12, 2022
@aganders3 aganders3 deleted the vec-thumbnail-ub branch October 12, 2022 14:06
@andy-sweet andy-sweet added this to the 0.4.17 milestone Oct 12, 2022
@andy-sweet
Copy link
Member

As @psobolewskiPhD mentioned in Zulip, this is fairly niche, so I'm going to remove the v0.4.17 milestone from this to keep the RC changes small.

Thanks again for the fix @aganders3!

@andy-sweet andy-sweet removed this from the 0.4.17 milestone Oct 14, 2022
@Czaki Czaki mentioned this pull request Jun 7, 2023
@Czaki Czaki added this to the 0.4.18 milestone Jun 7, 2023
@Czaki Czaki added bugfix PR with bugfix triaged-0.4.18 To mark PR that is triaged in 0.4.18 release process labels Jun 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix PR with bugfix triaged-0.4.18 To mark PR that is triaged in 0.4.18 release process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants