Remove catch_warnings in slicing #5603
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## main #5603 +/- ##
==========================================
- Coverage 89.51% 89.51% -0.01%
==========================================
Files 610 610
Lines 51200 51195 -5
==========================================
- Hits 45834 45829 -5
Misses 5366 5366
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Czaki
approved these changes
Mar 3, 2023
jni
approved these changes
Mar 4, 2023
Czaki
removed
the
ready to merge
|
This is probably small and contained enough to be brought into 0.4.18, but if it's difficult to cherry pick I think it's OK to drop this too. |
Czaki
pushed a commit
that referenced
this pull request
Jun 18, 2023
# Description This removes use of `catch_warnings` in slicing because [using that context manager has the side of effect of forgetting any previously raised warnings](python/cpython#73858). Using those in slicing means that any warnings raised in slicing are shown every time, whereas [the default behavior for Python is to only show the first time per location](https://docs.python.org/3/library/warnings.html#the-warnings-filter). The removal of `catch_warnings` in `Image._update_thumbnail` is justified because [napari requires scipy >=1.4.1](https://github.com/andy-sweet/napari/blob/196fb164ba12402dd10869fa8e4fd62ed8b6de00/setup.cfg#L71) and because the warning that was catching was [removed in scipy 1.4.0](scipy/scipy#10395) (thanks @jni!). The one in the `Layer.thumbnail` setter is a little harder to justify, but here's my reasoning. - I protected one common case of warnings, which is non-finite values. - Even if we get a warning, it will be shown once by default. - Since we depend on scikit-image now, I considered just using its `img_to_ubyte` instead of our `convert_to_uint8`, but I think it warns in more cases. Obviously, we have other usage of `catch_warnings` that could cause similar problems and this PR doesn't touch those. In general, we should be cautious about using `catch_warnings` in code paths that are hit frequently. ## Type of change - [x] Bug-fix (non-breaking change which fixes an issue) # References Closes #5588. # How has this been tested? - [x] all existing tests pass with my change I manually tested with `examples/add_multiscale_image`.
Czaki
pushed a commit
that referenced
this pull request
Jun 19, 2023
# Description This removes use of `catch_warnings` in slicing because [using that context manager has the side of effect of forgetting any previously raised warnings](python/cpython#73858). Using those in slicing means that any warnings raised in slicing are shown every time, whereas [the default behavior for Python is to only show the first time per location](https://docs.python.org/3/library/warnings.html#the-warnings-filter). The removal of `catch_warnings` in `Image._update_thumbnail` is justified because [napari requires scipy >=1.4.1](https://github.com/andy-sweet/napari/blob/196fb164ba12402dd10869fa8e4fd62ed8b6de00/setup.cfg#L71) and because the warning that was catching was [removed in scipy 1.4.0](scipy/scipy#10395) (thanks @jni!). The one in the `Layer.thumbnail` setter is a little harder to justify, but here's my reasoning. - I protected one common case of warnings, which is non-finite values. - Even if we get a warning, it will be shown once by default. - Since we depend on scikit-image now, I considered just using its `img_to_ubyte` instead of our `convert_to_uint8`, but I think it warns in more cases. Obviously, we have other usage of `catch_warnings` that could cause similar problems and this PR doesn't touch those. In general, we should be cautious about using `catch_warnings` in code paths that are hit frequently. ## Type of change - [x] Bug-fix (non-breaking change which fixes an issue) # References Closes #5588. # How has this been tested? - [x] all existing tests pass with my change I manually tested with `examples/add_multiscale_image`.
Czaki
pushed a commit
that referenced
this pull request
Jun 21, 2023
# Description This removes use of `catch_warnings` in slicing because [using that context manager has the side of effect of forgetting any previously raised warnings](python/cpython#73858). Using those in slicing means that any warnings raised in slicing are shown every time, whereas [the default behavior for Python is to only show the first time per location](https://docs.python.org/3/library/warnings.html#the-warnings-filter). The removal of `catch_warnings` in `Image._update_thumbnail` is justified because [napari requires scipy >=1.4.1](https://github.com/andy-sweet/napari/blob/196fb164ba12402dd10869fa8e4fd62ed8b6de00/setup.cfg#L71) and because the warning that was catching was [removed in scipy 1.4.0](scipy/scipy#10395) (thanks @jni!). The one in the `Layer.thumbnail` setter is a little harder to justify, but here's my reasoning. - I protected one common case of warnings, which is non-finite values. - Even if we get a warning, it will be shown once by default. - Since we depend on scikit-image now, I considered just using its `img_to_ubyte` instead of our `convert_to_uint8`, but I think it warns in more cases. Obviously, we have other usage of `catch_warnings` that could cause similar problems and this PR doesn't touch those. In general, we should be cautious about using `catch_warnings` in code paths that are hit frequently. ## Type of change - [x] Bug-fix (non-breaking change which fixes an issue) # References Closes #5588. # How has this been tested? - [x] all existing tests pass with my change I manually tested with `examples/add_multiscale_image`.
Czaki
pushed a commit
that referenced
this pull request
Jun 21, 2023
# Description This removes use of `catch_warnings` in slicing because [using that context manager has the side of effect of forgetting any previously raised warnings](python/cpython#73858). Using those in slicing means that any warnings raised in slicing are shown every time, whereas [the default behavior for Python is to only show the first time per location](https://docs.python.org/3/library/warnings.html#the-warnings-filter). The removal of `catch_warnings` in `Image._update_thumbnail` is justified because [napari requires scipy >=1.4.1](https://github.com/andy-sweet/napari/blob/196fb164ba12402dd10869fa8e4fd62ed8b6de00/setup.cfg#L71) and because the warning that was catching was [removed in scipy 1.4.0](scipy/scipy#10395) (thanks @jni!). The one in the `Layer.thumbnail` setter is a little harder to justify, but here's my reasoning. - I protected one common case of warnings, which is non-finite values. - Even if we get a warning, it will be shown once by default. - Since we depend on scikit-image now, I considered just using its `img_to_ubyte` instead of our `convert_to_uint8`, but I think it warns in more cases. Obviously, we have other usage of `catch_warnings` that could cause similar problems and this PR doesn't touch those. In general, we should be cautious about using `catch_warnings` in code paths that are hit frequently. ## Type of change - [x] Bug-fix (non-breaking change which fixes an issue) # References Closes #5588. # How has this been tested? - [x] all existing tests pass with my change I manually tested with `examples/add_multiscale_image`.
Czaki
pushed a commit
that referenced
this pull request
Jun 21, 2023
# Description This removes use of `catch_warnings` in slicing because [using that context manager has the side of effect of forgetting any previously raised warnings](python/cpython#73858). Using those in slicing means that any warnings raised in slicing are shown every time, whereas [the default behavior for Python is to only show the first time per location](https://docs.python.org/3/library/warnings.html#the-warnings-filter). The removal of `catch_warnings` in `Image._update_thumbnail` is justified because [napari requires scipy >=1.4.1](https://github.com/andy-sweet/napari/blob/196fb164ba12402dd10869fa8e4fd62ed8b6de00/setup.cfg#L71) and because the warning that was catching was [removed in scipy 1.4.0](scipy/scipy#10395) (thanks @jni!). The one in the `Layer.thumbnail` setter is a little harder to justify, but here's my reasoning. - I protected one common case of warnings, which is non-finite values. - Even if we get a warning, it will be shown once by default. - Since we depend on scikit-image now, I considered just using its `img_to_ubyte` instead of our `convert_to_uint8`, but I think it warns in more cases. Obviously, we have other usage of `catch_warnings` that could cause similar problems and this PR doesn't touch those. In general, we should be cautious about using `catch_warnings` in code paths that are hit frequently. ## Type of change - [x] Bug-fix (non-breaking change which fixes an issue) # References Closes #5588. # How has this been tested? - [x] all existing tests pass with my change I manually tested with `examples/add_multiscale_image`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This removes use of
catch_warningsin slicing because using that context manager has the side of effect of forgetting any previously raised warnings. Using those in slicing means that any warnings raised in slicing are shown every time, whereas the default behavior for Python is to only show the first time per location.The removal of
catch_warningsinImage._update_thumbnailis justified because napari requires scipy >=1.4.1 and because the warning that was catching was removed in scipy 1.4.0 (thanks @jni!).The one in the
Layer.thumbnailsetter is a little harder to justify, but here's my reasoning.img_to_ubyteinstead of ourconvert_to_uint8, but I think it warns in more cases.Obviously, we have other usage of
catch_warningsthat could cause similar problems and this PR doesn't touch those. In general, we should be cautious about usingcatch_warningsin code paths that are hit frequently.Type of change
References
Closes #5588.
How has this been tested?
I manually tested with
examples/add_multiscale_image.