Update clims in color transform whenever texture._data_limits changes

[tlambert03]

fixes #2229.

looks like the if not new_if and new_cl and not self._need_colortransform_update: conditional in the current _build_texture method is a little too strict. Back in #1911, we fully updated the color transform whenever the image texture was rebuilt. #1920 tried to make that a little more performant, by only updating when necessary, but it looks like the heuristic isn't perfect. This is somewhere in the middle, only rebuilding the color transform when the internal format has changed, otherwise updating the texture clims stored in the color transform (which have likely changed during the scale_and_set_data call)

with the time available, I wasn't able to determine a better way to know for certain whether those texture clims have changed. but that could perhaps be a future improvement. As is, updating self.shared_program.frag['color_transform'][1]['clim'] took 10s of microseconds on my computer... which is probably a fraction of the cost of the full _build_texture method anyway.

cc @brisvag

[djhoese]

So what is the case that doesn't work with what is currently in main?

[tlambert03]

ok, I think what we really need to know is whether the CPU texture's _data_limits has changed during scale_and_set_data (not whether texture._clims has changed).
However, that's not a base _ScaledTextureMixin attribute... do you want me to do this?

        pre_lims = getattr(self._texture, '_data_limits', None)
        self._texture.scale_and_set_data(self._data)
        post_lims = getattr(self._texture, '_data_limits', None)

or something else?

I'm sorry, I don't have a great vispy-only MRE. this is based on the visual effect of changing the clims on an image in napari (which essentially just changed the vispy image clims) and based on this test. In some cases a full redraw is necessary to trigger the change, and this fixes that.

[djhoese]

In some cases a full redraw is necessary to trigger the change

Any idea what those cases are? @brisvag and I had looked at that test in the last month or so, but it was hard to understand what it was doing. I know the .visible is meant to force a vispy .update() call but it wasn't clear to me how that effects things. The image should be visible right away (on creation) and the visibility is never toggled so I don't understand how that test is testing the changes in this PR.

[tlambert03]

Any idea what those cases are?

pretty sure the case is when texture._data_limits has changed during scale_and_set_data

like I said, I know that it would be great to have a vispy-only test here... but I don't have time this week to re-orient myself to those tests. perhaps @brisvag can narrow this down?

[djhoese]

I'll see if I can think of a test case in the next couple days and maybe we can narrow this down to the point that we know we are fixing it rather than only providing a workaround.

[brisvag]

I'll also look into it, but yeah it's complicated :)

[brisvag]

I just found out that this is a problem with volume as well, it's just that we don't have a test for it in napari.

[djhoese]

@brisvag @tlambert03 One thing that doesn't make sense to me (again) about the napari test, if the visible toggle is calling .refresh which is triggering a "set_data" event, does that mean it is calling ImageVisual.set_data()? And if so, is it with new data? If not and it is the same data, then how is the visible toggle doing anything for the data limits in the texture class in vispy? Everything should be the same...unless the problem is that some data limits or color limits aren't being stored properly in the ImageVisual and .set_data is resetting them incorrectly.

[brisvag]

Yes, that's also where I landed @djhoese. I'm a bit lost as to why this causes this inconsistent state.

[djhoese]

Due to the nature of "events" I can't figure out what the "set_data" event in napari does. Can someone point me to the code for what happens when it is emitted by the layer (at least as far as the layer/ImageVisual) is concerned?

[brisvag]

As far as I can tell, the only thing that gets fired in both cases (vispy only and napari) is ImageVisual.set_data. I just tried to debug a bit, and so far I see exactly the same data being sent to the texture (just an additional firing of set_data in napari when visible is set). So, the data being different is not the issue. My current hypothesis is that something goes somehow differently in CPUScaledTextureMixin.

Can someone point me to the code for what happens when it is emitted by the layer (at least as far as the layer/ImageVisual) is concerned?

These are the lines fired by that event in napari. There are a few if statements for changing between volume and image visual, but then it simply calls node.set_data, where node is a vispy.scene.visuals.Image.

[tlambert03]

will try to look closer this week, but approaching this from a different direction, I do think that the core change in this PR (updating the color_transform when the texture data_limits changes) makes a lot of sense. That really is the thing we need to coordinate... not the current clim. I do get the desire to figure out why the test failed in the first place and why this fixes it (and I think we should do that) ... but I think that end result here also makes logical sense

[djhoese]

I'd still like a test that fails without this PR and passes with this PR.

[tlambert03]

yep yep! thats also what I meant by "I think we should do that"

[brisvag]

Yes, that's the thing that bugs me; I cannot find where things go differently in napari. I tried triggering set_data manually in pure vispy, but nothing goes wrong there :/

[brisvag]

Finally getting to work on this. first of all, this is specifically a float issue, which is why it wouldn't show up with my previous experiments using the code in examples/scene/image.py. The bug shows up in several different circumstances, so there are several ways to test it. The most straight-forward I found is this:

Run this in rhe REPL to set up the canvas:

from vispy import scene, app
import numpy as np

canvas = scene.SceneCanvas()
canvas.size = 300, 300
canvas.show()
view = canvas.central_widget.add_view()

np.random.seed(0)
data = np.random.rand(100, 100) * 100
image = scene.visuals.Image(data, parent=view.scene)

view.camera = scene.PanZoomCamera(aspect=1)
view.camera.set_range()

app.run()

then, run

image.clim = 0, 10

then run it again.

If you're on main, the first time you get a wrong image, and the second time you get it right.
In this PR, instead, they are both the same (and identical to the second image).

[brisvag]

Unfortunately, the same thing does not seem to cause problems with pytest. This passes on main:

@requires_application()
def test_change_clim_float():
    """
    Test that with an image of floats, clim is correctly set from the first try

    see https://github.com/vispy/vispy/pull/2245
    """
    size = (40, 40)
    np.random.seed(0)
    data = np.random.rand(*size) * 100

    with TestingCanvas(size=size[::-1], bgcolor="w") as c:
        image = Image(data=data, parent=c.scene)

        image.clim = 0, 10
        rendered1 = c.render()
        # set clim to same values
        image.clim = 0, 10
        rendered2 = c.render()

        assert np.allclose(rendered1, rendered2)

[brisvag]

There must be somthing with TestingCanvas which "fixes" the bug. This code passes on main, and if you do it step by step you'll see that the first image is already correct!

from vispy.scene.visuals import Image
from vispy.testing._testing import TestingCanvas

import numpy as np


size = (40, 40)
np.random.seed(0)
data = np.random.rand(*size) * 100

with TestingCanvas(size=size[::-1], bgcolor="w") as c:
    image = Image(data=data, parent=c.scene)

    image.clim = 0, 10
    rendered1 = c.render()
    # set clim to same values
    image.clim = 0, 10
    rendered2 = c.render()

    assert np.allclose(rendered1, rendered2)

@djhoese do you have any suggestions?

[brisvag]

I rectify: it's not TestingCanvas causing the problem, but the context manager. Due to the context manager, canvas.show() is called before the image is created. This seems to cause the difference...

[djhoese]

Due to the context manager, canvas.show() is called before the image is created. This seems to cause the difference...

Very interesting. What else does the context manager do on the test canvas? Can it be not used?

[brisvag]

Very interesting. What else does the context manager do on the test canvas? Can it be not used?

I think we have to use it, cause it prevents some failures in windows and other CI-related issues.

But the issue is already in SceneCanvas.__enter__, cause that does the same.

[djhoese]

Good job everyone. I'm really busy today and I'd really like to sit down and understand everything going on here before merging it, so I might not get to it today. I will try to finish reviewing this this weekend or early next week. If I don't get to it by then then we can just merge it and make a patch release and I'll fix anything I find later on.

[djhoese]

So set aside some time tonight and dove into this. All these shortcuts and trying to be smart is terrible. It is some how better than before my float textures PR, but still too many shortcuts. I found an alternate solution that I like better, but I'll wait to see what the rest of you think before pushing it to this PR. The test is unchanged and my solution also works. Here is the series of events and why things work or don't work (this may not make sense as it is how my brain walked through the process, but I may need this for reference in the future):

1. You create the image and everything needs to be built (color transform) and uploaded (texture data) so any shortcuts are basically skipped. You get a default "auto" clim on a CPU-scaled texture.
2. You set .clim = (0, 10) which sets the new color limits on the texture object but requires data to be re-uploaded to the texture. When uploading the texture data, there is no texture internalformat change and no color limit change as far as the self._texture.clim property is concerned because the clim setter already set the value on the texture. In this case the clim on the texture doesn't change, but the data limits do and the clim_normalized values do (because they are based on data_limits).
3. In the old code no internalformat change is detected and no clim change is detected so the color transform is never updated with the new normalized color limits. In the new code the if statements are restructured so that internalformat being unchanged doesn't effect whether or not we can shortcut the colortransform. This is just smarter logic regardless of what solution we end up choosing.

My proposed solution:

diff --git a/vispy/visuals/image.py b/vispy/visuals/image.py
--- a/vispy/visuals/image.py	(revision 3918872421dc59d4f7679660dd3cce9efadaea51)
+++ b/vispy/visuals/image.py	(date 1637635555974)
@@ -521,18 +521,22 @@
         self._prepare_transforms(view)
 
     def _build_texture(self):
-        pre_lims = getattr(self._texture, '_data_limits', None)
+        try:
+            pre_clims = self._texture.clim_normalized
+        except RuntimeError:
+            pre_clims = "auto"
         pre_internalformat = self._texture.internalformat
         self._texture.scale_and_set_data(self._data)
-        post_lims = getattr(self._texture, '_data_limits', None)
+        post_clims = self._texture.clim_normalized
         post_internalformat = self._texture.internalformat
         # color transform needs rebuilding if the internalformat was changed
-        # new color limits need to be assigned if the texture data limits changed
+        # new color limits need to be assigned if the normalized clims changed
         # otherwise, the original color transform should be fine
-        # Note that this assumes that if clim changed, clim_normalized changed
-        if post_internalformat != pre_internalformat:
+        new_if = post_internalformat != pre_internalformat
+        new_cl = post_clims != pre_clims
+        if new_if:
             self._need_colortransform_update = True
-        elif post_lims != pre_lims and not self._need_colortransform_update:
+        elif new_cl and not self._need_colortransform_update:
             # shortcut so we don't have to rebuild the whole color transform
             self.shared_program.frag['color_transform'][1]['clim'] = self._texture.clim_normalized
         self._need_texture_upload = False

The reasons I like this:

1. My original comment on this method mentioned "normalized clims". I ended using the regular "clims" because normalized clims don't exist (RuntimeError) when clims are "auto".
2. Data limits are specific to the CPU-scaled texture. I'd like if this logic could use interfaces that are shared between both CPU and GPU scaled textures.
3. Data limits aren't used in the color transform, clim_normalized is. We should base our logic for decisions based on that.
4. It still uses the swapped if statements which as I noted above are much smarter and needed for any solution to work. The internalformat remains None for most CPU-scaled textures as a signal to OpenGL/gloo to choose whatever format is best.

Thoughts?

[djhoese]

Also FYI @brisvag it is possible to get this to fail with integer data if you keep everything the same but on creation of the Image you pass clim=(0, 5). Then when you set the clim to (0, 10) it forces a new texture upload and the same series of events happens where self._texture.clim is unchanged.

Edit: I'll add a test for this no matter what we choose.

[brisvag]

I don't feel strongly either way. It seems that the core issue is that using clim does not work, but using clim_normalized and _data_limits does. I guess since your version uses a public property, it's probably better.

[brisvag]

Actually, your code breaks if passing clim as an argument to Image, because it tries to access self._texture.clim_normalized when texture._data_limits is still None.

    @property
    def clim_normalized(self):
        """Normalize current clims to match texture data inside the shader.

        If data is scaled on the CPU then the texture data will be in the range
        0-1 in the _build_texture() method. Inside the fragment shader the
        final contrast adjustment will be applied based on this normalized
        ``clim``.

        """
        if isinstance(self.clim, str) and self.clim == "auto":
            raise RuntimeError("Can't return 'auto' normalized color limits "
                               "until data has been set. Call "
                               "'scale_and_set_data' first.")

>       range_min, range_max = self._data_limits
E       TypeError: cannot unpack non-iterable NoneType object

[brisvag]

Yeah, I think we need to access the private property to make sure this works on instantiation as well (when not everything is ready to go). an alternative is to check for both RuntimeError and TypeError, but then I feel like we're getting in the dangerous territory of hidden exceptions.

I propose we keep @tlambert03 's code.

[tlambert03]

Data limits are specific to the CPU-scaled texture. I'd like if this logic could use interfaces that are shared between both CPU and GPU scaled textures.

Part of the point here is that this logic is never needed for GPU textures, so it's not surprising that it's a CPU-scaled texture-specific thing. If you only want to use only the abstract interface, we could add data_limits = None the base class and never touch it for GPU?

[djhoese]

an alternative is to check for both RuntimeError and TypeError

I was thinking adding a check in the clim_normalized to see if data limits hasn't been set yet, but...

Part of the point here is that this logic is never needed for GPU textures

Yeah, I guess the only time the GPU-scaled texture has the normalized clim change is if the internal format changes. The clim being set previously should "stage" the normalized clim for all future steps.

However, I think the point still stands that what we really care about is clim_normalized. There is a reason _data_limits is private. It isn't something the "user" (in this case the ImageVisual) is supposed to need to worry about, at least in my opinion.

So either way, we need:

1. More tests which I'll add for the integer case I pointed out and the case @brisvag pointed out (assuming that isn't captured by one of the existing tests).
2. I will add an extra RuntimeError to the clim_normalized to detect when data limits is None because that is something that should be caught.

FYI if you guys are bored there is a vispy monthly meeting in ~10 minutes if you want to join (DM me on gitter).

[djhoese]

@tlambert03 I was going to push my changes to this branch. And idea why I don't have permissions?

[djhoese]

FYI I was going to push a new error check, an updated test, and my first of the _build_texture method as separate commits so we could revert in the future if needed.

[tlambert03]

Sorry im not sure, I've had this issue before, but never unchecked the allow admins button... let me know what you want to do. Or, if you want to just open a new pr that's ok

[djhoese]

You could add me as a collaborator to your fork? If you aren't comfortable with that then yeah I can make PR on your fork or make a new PR here.

[tlambert03]

Added!

…

On Nov 23, 2021, at 12:39 PM, David Hoese ***@***.***> wrote:  You could add me as a collaborator to your fork? If you aren't comfortable with that then yeah I can make PR on your fork or make a new PR here. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[brisvag]

Looks good to me! Test failures are also in other PRs, so I think they are unrelated.

[djhoese]

Nah there are definitely some from my changes. That's what I get for trying to be right.

[brisvag]

Nah there are definitely some from my changes. That's what I get for trying to be right.

whoops, spoke too fast! :D

[djhoese]

Ok osmesa tests now pass. Is my solution "good enough" now to use? and merge?

[tlambert03]

LGTM. I'm happy with anything that gets @brisvag's test to pass :)

[brisvag]

Good for me too!

[djhoese]

Thanks guys. I just want to say that I wasn't ignoring your desire to have _data_limits be used, but I also think I am "more" right with the reasons I specified and I didn't really hear strong arguments against them. Thanks for dealing with this.

[djhoese]

I think we work on @brisvag's other PR #2262 and then make a bug fix release.