I suppose there is no way to include this in your updated textures so it isn't needed here?

This is needed here because Image saves self._data, which should be saved in the "correct" version, I think.

I did remove it in #2351.

If using the CPU-scaled texture, don't we not need this downcasting to happen? I mean, if we were given 64-bit floats, the CPU-based scaling would deal with the conversion to a GPU-compatible dtype, right?

Uhm, the problem is that the self._data is kept for later and used again... If we don't keep a "correct" (downcast) version of the data, then we'll get a million warnings every time the texture needs to be rebuilt (like when clim gets changed). I find it more correct to simply hold the data was was actually sent to the gpu, and only warn once.

(this is true for both volume and image)

that was actually sent to the gpu

But in the CPU-scaled texture case this casting should never happen and shouldn't be needed so no warning was going to be generated anyway. Now, with this PR, it would produce an unnecessary warning and make a copy of the data.

I see. Let's see if tests pass without that casting.

Things seem to be doing fine!

This was needed to fix a test failure; the reason why it was working before, is because we were doing unnecessary copies which resulted in copy=False not mattering here. But now that we respect the copy argument better, this example was failing because noise was getting changed between this line and then next.

I'm not sure I understand. So one of the Visuals (the image?) was modifying the noise array? Doesn't that suggest that it wasn't copying internally when it should have because the copying was necessary to not modify the input data, right?

Kinda the opposite: note that by default copy=False, and, despite that, noise was clearly getting copied or the example would have crashed. With the current changes, the copy kwarg is getting respected, so without copy=True we get the failure.

So wait, with copy=False (the default), who is modifying noise in this example? Shouldn't the Visual copy if it is going to modify the data? Or does copy=False in these Visuals that it will modify the data inplace no matter what?

So wait, with copy=False (the default), who is modifying noise in this example?

image1

Shouldn't the Visual copy if it is going to modify the data? Or does copy=False in these Visuals that it will modify the data inplace no matter what?

It will modify in place no matter what if it can.

I think the confusion here comes from 2 different use cases for specifying copy=True:

1. "Please copy it because I might change it externally before you send it to the gpu and I want you to send THIS EXACT data" (this is the use specified in the docs)
2. "Please copy it before altering it because I will need it elsewhere"

This is case 2.

I also noticed that in your screencast that the bottom two boxes are the ones that don't display at all, but those are only Isocurves, not the ImageVisuals created in the example.

Lol, you're right, thanks for double checkin... I could swear I saw the same error. I must be wrong though, I can't reproduce anymore. Maybe I was on this branch and though it was main 🤦

The error above is indeed happening on main without any changes, but happens regularly if I rerun the example in the same ipython session. Definitely a bug, and definitely unrelated.

I know I said this before, but I think that I really found the spot now xD

With some good-old print-debugging, I singled out this PR's equivalent of this line (as you expected, @djhoese):

This is where data was being modified. I simply added an explicit copy just before, in cases where we want to modify and a copy was not already made.

Ok, looking at the diff now I see how we raised an error before this PR if updating couldn't be done without copying. I think copying when we have to is a reasonable assumption, but we may have to triple check the docstrings for the ImageVisual and the textures. Looks good to me though.

...except for the test failures 😉

Is this test just checking that a certain data type can be ingested by the Texture? Is there something we should be checking after it is set to confirm validity or is the fact that it doesn't raise an exception good enough?

Otherwise, the 2D versus 3D could/should be made into another pytest parameter and then an if statement to handle if a Texture2D or Texture3D should be made.

Also, typo, tex2D[:10] in the 3D block of code.

This is just testing that no error arises. The image and volume tests make sure that the rendering goes as expected as well, since Texture does not provide a way to get back the data from the GPU afaik. Updated the test!

Out of bounds in the sense that 128 is over the 127 limit? Shouldn't 128 then be clipped to 127 and made white?

No, because:

In [1]: import numpy as np

In [2]: np.array(128, dtype=np.int8)
Out[2]: array(-128, dtype=int8)

We can add extra machinery to clip instead, if you think that would be better, hto I'm not sure how that would work. This was intended more as a "do not do crazy stuff like passing the wrong dtype" kind of thing.

But this is a CPU scaled ImageVisual and this new_data array is a 64-bit float. The initial data dtype doesn't matter here, does it? The CPU-scaling should be scaling the data, based on the clims, to a uint8 texture and sending it to the GPU.

It's kinda the opposite; the CPUScaledTexture always sends np.float32 to the GPU, because the scaling happens on the CPU side. So the flow in the test above is:

• image last time had int8 data
• image.set_data(): data is saved as-is float in image._data
• image._build_texture()
• texture.scale_and_set_data(): here data is converted to the previous texture._data_dtype (int8) so we can correctly interpret/build clims
• texture._scale_data_on_cpu(): converts data to float32 if necessary

So in case if we wanted to do the clipping, it should happen in the scale_and_set_data step

Ah ok, so that casting to the old/original dtype is new behavior for CPU scaling from what I can tell. Lines like:

data = np.array(data, dtype=self._data_dtype, copy=copy)

didn't exist in the old version from what I can tell in the github diff. I'm ok with that change but it should probably be documented somewhere (in the ImageVisual or VolumeVisual or at least in the texture classes). Based on your comments in the code and your comment above I'm guessing the odd behavior that happens if we don't do this casting is that the limits (and interpolation?) are messed up?

I wasn't suggesting that we add clipping, I think there is clipping in the shader and I thought that's what this test was assuming/using to verify the output. It is actually wrapping/overflowing the floating point 128 64-bit float value to a -128 8-bit signed integer value. That makes sense. I think overall the confusing thing for me was not knowing that the data type is now preserved between .set_data calls. Again, that is fine, just not what I expected.

the odd behavior that happens if we don't do this casting is that the limits (and interpolation?) are messed up?

Yes, I wasn't very clear, my bad. I think it makes sense to clip as you say, actually. We could use np.iinfo and np.finfo to get min/max and clip before casting.

Nah, I think any clipping should happen on the GPU/shader. I'd like to avoid any unnecessary processing on the CPU. Especially since this won't apply to most cases where people are using the same data type and they probably have data in the proper range.

Saw the answer late, already tried an implementation :P See what you think about it. I agree that ideally you'd do clipping on the GPU, but with a CPU scaled texture we don't really have a choice, it's either we clip it, or the result is gonna be nonsense to the user.

Same argument as for the ImageVisual test, shouldn't 128 get clipped to 127 and be white on the output?

@brisvag sorry for digging up this old code, but I got here looking through warnings that come up in tests. Should this be as above? Otherwise I'm not sure what this line is meant to be doing.

oof... It's been some time, not sure I remember much about these changes. Your change definitely looks like the most likely intended code ^^' That probably means we have some test missing that should catch a problem here.