Reset contrast range when update layer data from magicgui FunctionGui

[Czaki]

Description

Reset contrast limits range and contrast limits when updating layer data from functions that return ImageData etc.

import numpy as np

import napari
from napari.types import ImageData


def add_function(layer1: ImageData, layer2: ImageData) -> ImageData:
    return layer1 + layer2


data1 = np.random.random((100, 100))
data2 = np.random.random((100, 100))
data3 = np.random.random((100, 100)) * 50

viewer = napari.Viewer()
viewer.window.add_function_widget(add_function)


viewer.add_image(data1)
viewer.add_image(data2)
viewer.add_image(data3)

napari.run()

[Czaki]

This PR solves the problem mentioned here #5127 (comment) (and above in discussions), but I'm not sure if it solves the general problem from #5127 issue.

cc @haesleinhuepf, @psobolewskiPhD

[github-actions]

Click here to download the docs artifacts
docs
(zip file)

[codecov]

Codecov Report

Attention: Patch coverage is 33.33333% with 4 lines in your changes missing coverage. Please review.

Project coverage is 92.71%. Comparing base (25abe27) to head (8296e15).

Files with missing lines	Patch %	Lines
napari/_qt/_qapp_model/injection/_qprocessors.py	33.33%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5136      +/-   ##
==========================================
- Coverage   92.81%   92.71%   -0.10%     
==========================================
  Files         623      623              
  Lines       57347    57352       +5     
==========================================
- Hits        53228    53176      -52     
- Misses       4119     4176      +57     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[haesleinhuepf]

Thanks for working on this @Czaki !

I'm a little bit worried about side effects. Would it be possible that the visualization appears blinking if contrast limits are updated all the time? Also, does this cause a delay?

I must admit, I liked the idea by @psobolewskiPhD for adding a reset button more than resetting all the time...

[Czaki]

I'm a little bit worried about side effects. Would it be possible that the visualization appears blinking if contrast limits are updated all the time? Also, does this cause a delay?

But this is reset only if new data is set as the function's output.
The delay will be same as the calculation min and max of returned data.

[brisvag]

I agree that I don't love updating all the time. I understand the use case of updating a layer inplace with a dramatically different image (as in @haesleinhuepf's example of image1 / image2) but I would expect most use cases to update inplace with something similar to the original data. In that case, I think consistency of contrast limits and range is more important for effective comparison.

Wouldn't it be better to do this if the auto option is selected?

[brisvag]

Actually, maybe only update the range automatically (when auto is selected)`.

[kandarpksk]

Aside from the solutions being proposed, we might also want to consider:

(1) indicating in the UI that the contrast limit is "out of date"
Showing a warning icon with a tooltip to explain the issue (and solution) would improve visibility of the existing reset feature, assuming we have a way to show a warning state.

(2) providing a setting to auto-reset the contrast limit
I don't have an opinion of whether the setting should be enabled by default or not (esp. when auto-contrast is set to auto), but seems like there are likely to be users on both sides, and it's a way to offer flexibility.

cc: @isabela-pf

[jni]

@Czaki given concerns about possible side effects, should we close this? Or do you think we can address those concerns? I agree mostly with @brisvag's comment that auto-rescale should be opt-in.

[Czaki]

If you have a simple widget for summing layers and three layers A, B, C.

For the current solution, if You first execute A+B and then A+C, the effect will be different from just executing A+C, as in the first case the contrast will be calculated based on the result of A+B, in the second it will be based on A+C.

So we have a situation where image presence depends on execution order, and it looks like a bug to me. This PR is fixing this behavior.

At least reset_contrast_limits_range should be called.

[Czaki]

I think that at least reset_contrast_limits_range should be called.

[brisvag]

Took a look again; I stand by my point, the cost of having this run all the time is too high and not worth it.

The problem is also easily circumvented by setting the auto-contrast to continuous, which does exactly what we want, and is explicit.

If anything, I would suggest a PR that exposes the autocontrast programmatically somehow?

[Czaki]

Took a look again; I stand by my point, the cost of having this run all the time is too high and not worth it.

Why is too high? If the layer is created using a functional widget, the calculation of contrast limit is performed.

If anything, I would suggest a PR that exposes the autocontrast programmatically somehow?

Programmatically, the plugin author may just return dict with contrast limits.

[brisvag]

Here's a contrived example explaining why I think it's bad:

import numpy as np
import napari
from magicgui import magicgui
from napari.types import ImageData

data = np.random.random((100, 100))

@magicgui(auto_call=True, x={'widget_type': 'Slider'})
def random_change(x) -> ImageData:
    data[50, 50] = np.random.rand() * 10
    return data

viewer = napari.Viewer()
viewer.window.add_dock_widget(random_change)

viewer.add_image(data)

Move the slider around. I think it should be explicitly requested by using auto-contrast continuous if one wants this behaviour. Not only that, but this PR would make the other behaviour (no flashing, just change the values maintaining contrast) simply impossible to achieve.

[Czaki]

So your example is an example that updates a single pixel, where my example is when we have generated, a whole new layer each time.

I do not see an example of a plugin like yours. But see @haesleinhuepf plugins that are wrappers around SimpleITK, scikit-image and other libraries that will benefit from at least reset_contrast_range

[Czaki]

And your example does not show any problem if we reset only the contrast limit range.

[brisvag]

My point was that there are cases where we make many updates to the data (such as with a slider) and calculating min/max values might be too expensive done that often, unless requested. Or are you saying that is this negligible?

[Czaki]

My point was that there are cases where we make many updates to the data (such as with a slider) and calculating min/max values might be too expensive done that often, unless requested. Or are you saying that is this negligible?

I'm saying that magic should be more correct than performant.
The plugin/widget author has multiple way to make it performant:

1. use Image instead of image data and then image.data[50,50] = something; image.refresh(),
2. return contrast limit return data, {'contrast_limit': (0, 100)},

and much more.

But with the current approach, the user has no message that the black screen is because of wrong contrast limits, not because the widget returns arrays of zeros.
So by carrying about experienced users (to reduce the number of lines to write), the new users meet behavior that they may not expect.

[brisvag]

Fair enough, I'm convinced 👍 But would be nice to warn the "experienced user" about the performance issue, somehow.

[Czaki]

Did update documentation will be enough for such warn?

[jni]

We discussed this at the napari-eurasia meeting (me, @DragaDoncila, @brisvag, and @Czaki in attendance) and let's just say it was contentious. 😂 Overall, we all agreed that it would be good to have more eyes on this PR, and to think hard about all the possible options to address the issues that this PR means to address:

• the contrast limits of a magicgui-generated layer depend on the first execution of the magicgui function — if that execution was somehow wrong (e.g. the first threshold is 0 so 100% of pixels are 255 so the contrast limits can't be computed), then later executions will have those incorrect contrast limits and it will be difficult for the user to know what went wrong. (Importantly, though, it is no longer difficult to recover: the user can hit the "Reset" button under the contrast limits.)
• a user presented with blown-out contrast-limits would have no visual indication to know whether the data was wrong on whether the contrast limits were wrong (and users are inclined to think it was the data).

Some options that were discussed (also above), as alternatives to always updating the contrast:

• warn the user when the image is low-contrast. (This would have the same performance cost but could be done asynchronously and would be less magical, because it would require an action from the user.)
• flash or otherwise emphasise the contrast limits adjustments and reset buttons when the new data is added, to indicate to the user that they might want to adjust the contrast now
• make contrast limits and an image histogram more prominent in the UI in general
• stop overwriting the data when ImageData is passed — which is inconsistent with a LayerDataTuple without the layer name set. (This could be very disruptive to users though.)

We'd like to hear from @psobolewskiPhD in particular since he has done a lot of user-facing work and might have encountered this issue before. We also plan to discuss this PR at upcoming community meetings.

[psobolewskiPhD]

Gah I lost my post.
So I"m of two minds about this.
First, my experience with end users is that contrast limits -- in napari and otherwise -- are a source of confusion for novices. Histograms help a lot here -- not having an easy way to show histograms makes using napari as a most basic teaching tool harder -- I know, I could use matplotlib and numpy, but then we dive deeper into python.

So, getting back to the issue at hand, there are two cases:

1. where you want the contrast limits to update, because you are doing something that you expect to change them, e.g. the data1+3 example in the OP.
2. where you don't want the contrast limits to be updated, when comparing or doing something quantitative where changing the contrast limits would mislead you into a lack of a change. I can use the same example from OP, on main if you do data1+data1 and then then when you do data1+data3 you will clearly see that the data changed. But if you use this PR, it will look similar even though the values are vastly different. Of course, if you do this in the opposite order you will get a black box. Adding to this, if you do data1+data1 for example and then adjust the contrast limits to reflect something about the data and then run the widget you lose those contrast limits. This would be irritating.

So I feel like this is an issue that should be resolved by ensuring the current auto-contrast buttons handle these behaviors.
On main, if you run the widget from the OP the first time, you get the "proper" (auto) contrast. You can then click continuous and you get the behavior from this PR as far as I can tell.
Otherwise you can click once to set the contrast limits to the data range. And if you manually change them, then the widget respects that.

Now there is no reset to set the contrast limits to full scale after having used auto
There is the right-click contrast limits slider, which has a reset, but it doesn't do what I expect as it appears to match auto-contrast: once.

[Czaki]

where you don't want the contrast limits to be updated, when comparing or doing something quantitative where changing the contrast limits would mislead you into a lack of a change. I can use the same example from OP, on main if you do data1+data1 and then then when you do data1+data3 you will clearly see that the data changed. But if you use this PR, it will look similar even though the values are vastly different. Of course, if you do this in the opposite order you will get a black box. Adding to this, if you do data1+data1 for example and then adjust the contrast limits to reflect something about the data and then run the widget you lose those contrast limits. This would be irritating.

With this case it could be simply changed by returning data, {'contrast_limit': (0, 100)}, 'image', not simple data.
The original contrast limit may be read from data.

[jni]

The original contrast limit may be read from data.

No, we are talking about the case when the user has manually and intentionally changed the contrast limits based on the previous output.

With this case it could be simply changed by returning data, {'contrast_limit': (0, 100)}, 'image', not simple data.

All cases of confusion can be cleared up by the implementation using a LayerDataTuple instead of ImageData, so I don't think that's really relevant. What's important is, we are acting on limited information in this case, so we have to make some guesses (despite the Zen of Python), and the questions are:

• which cases are more common: I think in this case, it's unclear, we've heard arguments in both directions based on different core devs' experience.
• how easy is it for the user to tell that a potentially incorrect guess has been made: currently, it's hard in both scenarios, but we can improve that.
• how easy is it for the user to recover from a potentially incorrect guess: in this case, not adjusting the limits wins by a very large margin.

[Czaki]

Maybe this will be a good option to not reset range when someone adjusts contrast limits manually:

Suggested change

	layer.reset_contrast_limits_range()
	layer.reset_contrast_limits()
	if layer.contrast_limits == layer.contrast_limits_range:
	layer.reset_contrast_limits_range()
	layer.reset_contrast_limits()

[Czaki]

@jni @DragaDoncila @psobolewskiPhD ?