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

Multiscale 3D rendering warning should happen once only #5588

Closed
jni opened this issue Feb 25, 2023 · 4 comments · Fixed by #5603 or #6070
Closed

Multiscale 3D rendering warning should happen once only #5588

jni opened this issue Feb 25, 2023 · 4 comments · Fixed by #5603 or #6070
Assignees
Labels
bug Something isn't working priority-low

Comments

@jni
Copy link
Member

jni commented Feb 25, 2023

When I switch to 3D view in a multiscale array, I get the following warning:

/Users/jni/projects/napari/napari/layers/image/_slice.py:113: UserWarning: Multiscale rendering is only supported in 2D. In 3D, only the lowest resolution scale is displayed!

This is fine, except the warning happens on every slice event:

...
/Users/jni/projects/napari/napari/layers/image/_slice.py:113: UserWarning: Multiscale rendering is only supported in 2D. In 3D, only the lowest resolution scale is displayed!
/Users/jni/projects/napari/napari/layers/image/_slice.py:113: UserWarning: Multiscale rendering is only supported in 2D. In 3D, only the lowest resolution scale is displayed!
/Users/jni/projects/napari/napari/layers/image/_slice.py:113: UserWarning: Multiscale rendering is only supported in 2D. In 3D, only the lowest resolution scale is displayed!
/Users/jni/projects/napari/napari/layers/image/_slice.py:113: UserWarning: Multiscale rendering is only supported in 2D. In 3D, only the lowest resolution scale is displayed!
...

That's a fraction of the console output, and the toast pop-up on the canvas just keeps popping as you interact with the viewer.

@kephale is making good progress to help us get rid of that warning altogether, but in the meantime, a quick fix would be to set a flag for whether we've warned already, and not warn if so.

@jni jni added the feature New feature or request label Feb 25, 2023
@andy-sweet andy-sweet self-assigned this Mar 2, 2023
@andy-sweet andy-sweet changed the title Muliscale 3D rendering warning should happen once only Multiscale 3D rendering warning should happen once only Mar 2, 2023
@andy-sweet
Copy link
Member

I was optimistic that this could be fixed with warnings.simplefilter('once') and maybe warnings.catch_warnings, but that doesn't seem to have effect here.

I then noticed filterwarnings in pyproject.toml, which attempts to ignore all warnings matching this message. I'm not sure if that would have an effect in editable installs, but I could also reproduce this issue on my non-editable v0.4.17 install, so it doesn't seem to have an effect at all - or am I misinterpreting standard warning filtering in Python? I also wonder if napari is handling warnings in a special way that means these get ignored?

We could of course fix this by storing a static boolean that gets set after the first warning, but that feels silly given there are built-in mechanisms for handling this stuff.

I'm going to unassign myself for now and hope someone with more background/knowledge steps in here.

@andy-sweet andy-sweet removed their assignment Mar 2, 2023
@andy-sweet
Copy link
Member

I then noticed filterwarnings in pyproject.toml, which attempts to ignore all warnings matching this message. I'm not sure if that would have an effect in editable installs, but I could also reproduce this issue on my non-editable v0.4.17 install, so it doesn't seem to have an effect at all - or am I misinterpreting standard warning filtering in Python?

From community meeting debugging, I am misinterpreting because that filterwarnings only applies to tests. So one mystery solved.

@andy-sweet andy-sweet self-assigned this Mar 3, 2023
@andy-sweet
Copy link
Member

andy-sweet commented Mar 3, 2023

I couldn't resist picking this one back and I think this is actually a Python bug: python/cpython#73858

If I understand correctly, the problem is that on exiting catch_warnings forgets about any previously seen warnings, which is why we get repeats even though the 'default' action is to show once. The tiny minimal reproducing example there shows this off:

import warnings
for _ in range(3):
    with warnings.catch_warnings():
        pass
    warnings.warn('warning')

In our case, the troublemaker is (of course...) the thumbnail code which uses catch_warnings twice:

To fix this specific issue (i.e. when going through the slicing path), we might be able to remove the first usage (as we require scipy >= 1.4) and we may also be able to remove the second one if we use the scikit-image utility and that doesn't issue a warning.

But the general problem is that any other use of catch_warnings that gets hit while using napari will cause it to forget, so the issue may get hit again (though likely not on every slice/refresh).

andy-sweet added a commit that referenced this issue Mar 6, 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 issue 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 issue 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 issue 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 issue 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 issue 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`.
@andy-sweet
Copy link
Member

andy-sweet commented Jul 17, 2023

I think this one is back. Serves me right for not writing an explicit test... Here's one that can reproduce the issue:

def test_3d_slice_twice_with_multiscale_image_warns_once():
    data = [np.zeros((4, 4, 4, 4)), np.zeros((2, 2, 2, 2))]
    layer = Image(data, multiscale=True, rgb=False)

    with pytest.warns(
        UserWarning, match="Multiscale rendering is only supported in 2D"
    ):
        layer._slice_dims(point=(0, 0, 0, 0), ndisplay=3)

    layer._slice_dims(point=(1, 0, 0, 0), ndisplay=3)

and another that covers more code:

def test_3d_slice_twice_with_multiscale_image_warns_once():
    # Start in 2D to avoid warning when adding the layer.
    viewer = ViewerModel(ndisplay=2)
    data = [np.zeros((4, 4, 4, 4)), np.zeros((2, 2, 2, 2))]
    viewer.add_image(data, multiscale=True, rgb=False)

    # Switch to 3D to trigger the warning.
    with pytest.warns(
        UserWarning, match="Multiscale rendering is only supported in 2D"
    ):
        viewer.dims.ndisplay = 3

    # Simulate moving the slider to slice again, without a warning.
    assert viewer.dims.point[0] != 0
    viewer.dims.point = (0, 0, 0, 0)

You'll also have to remove the related ignore line in pyproject.toml.

My assumption is that there was a catch_warnings added after #5603.

Unless the underlying cpython issue is fixed and backported, it seems like the only fix here is to forbid use of catch_warnings in that code path.

This is an issue on main and for v0.4.18 unfortunately.

@psobolewskiPhD psobolewskiPhD added bug Something isn't working priority-low and removed feature New feature or request labels Jul 17, 2023
@jni jni mentioned this issue Jul 18, 2023
2 tasks
@jni jni closed this as completed in #6070 Jul 19, 2023
jni added a commit that referenced this issue Jul 19, 2023
# Fixes/Closes

Closes #5588

# Description

This bugs me all the time. Ultimately I would rather get bug reports
from
people expecting proper 3D multiscale rendering (rare in our community)
than
seeing this warning non-stop, or having to constantly cherry-pick this
commit
into my current working environment.

I also expect @kephale's work will make this redundant soon anyway, so
in the
meantime, I just want the warning gone. Hopefully others agree that it
does not
add enough value to be kept around.

## Type of change
- [x] Enhancement (non-breaking improvements of an existing feature)

## Final checklist:
- [x] My PR is the minimum possible work for the desired functionality

Co-authored-by: Genevieve Buckley <30920819+GenevieveBuckley@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority-low
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants