@outofculture @j9ac9k
Hi guys, do you want to weigh in on this? Are there any potential obstacles I must avoid with this PR? I felt like I was very close to (or maybe even touching) some fairly large APIs that I'm not familiar with in this draft, so maybe my changes have implications that I'm unaware of. Cheers

Is there anything I can do to make this PR more reviewable? Is there some other platform I should have used to ask these questions to make sure I'm on the right track? Thanks

Hi there @SimenZhor

Sorry I've been quiet. I am on vacation with very limited internet connectivity. I'll be back in a few days and will review this PR more closely with @outofculture

I have a few comments from a far-away perspective:

• Could you say a few words about the situation with the stacked autoscaling / fixed PlotItems? That looks like extra overhead.
• You've conveniently commented that the order of preference for the source of color map changes. It might be nice to avoid that, otherwise somebody's diagrams somewhere will confusingly end up the wrong color.
• Silimar for renaming self.enableAutoLevels to self.enableautolevels. Probably better not to touch things that somebody might be using. Weird capitalization is usually applied to things that someone here considered "exposed to the user" before. That's where we typically have naming conventions that mimic what Qt does (rather than classic Python).

I also have the impression that this PR may change the defaults on ColorBarItem a little too far in the direction of the "animated image" use case. As far as I can tell, there are actually three typical uses:
(1) Plot an image, autoscale, show a non-interactive color map.
(2) Plot an image, autoscale, but then allow interaction to adjust the levels as needed
(3) Plot an image, keep the current scaling, but allow interaction to manually change it.

I think (2) might be the most common use case, and it is also what we traditionally seemed to be optimizing for.
"Interactive ColorBarItems will always attempt to disable auto-scaling for its assigned image items.": Will that still work conveniently with case (2)? The toggle from case (2) to your target of (3) should be a toggle that needs to be set only once on initialization, but I guess (2) should remain the default?

And as a bonus challenge, since we are probably not doing that correctly at this time:
If there's a displayed color bar, it should always match what is displayed in the attached plot or plots. Other behavior is an invitation to messed up science and disaster in general. I have a suspicious eye on the self.setLevels(img_levels, update_items=False) in L214 of ColorBarItem here, but it might be good to keep that in mind while you are experimenting with behavior.

Hi @NilsNemitz, thanks for the thoughtful comments. I'll try my best to answer your questions.

Could you say a few words about the situation with the stacked autoscaling / fixed PlotItems? That looks like extra overhead.

If you mean in the example code, I just wanted to show that it is possible to enable and disable autoscaling, and intuitively illustrate how much of an effect this can make in a case like what the example was already showing:

Basically, they both have their drawbacks that I wanted to showcase. For autoscaling, it's pretty much impossible to get an impression of the absolute values each color represents (during animation that is. This does not translate well to the screenshot above), while for consistent scaling it's pretty difficult to see the high-frequency components of the signal because the absolute value is dominated by the low-frequency bias that is hidden in the autoscaling usecase.

You've conveniently commented that the order of preference for the source of color map changes. It might be nice to avoid that, otherwise somebody's diagrams somewhere will confusingly end up the wrong color.

Yeah, I kind of agree. I just didn't like the thought of repeating the same loop twice just to avoid this. I'll see if I can find another solution.

Silimar for renaming self.enableAutoLevels to self.enableautolevels. Probably better not to touch things that somebody might be using. Weird capitalization is usually applied to things that someone here considered "exposed to the user" before. That's where we typically have naming conventions that mimic what Qt does (rather than classic Python).

This is actually not entirely the same case as the change in colormap prioritization. self.enableAutoLevels was introduced by me in this PR, which is not included in any releases yet. What I've done here is basically to rename the variable because I wanted to introduce methods for enabling and disabling this feature. I can of course change it back regardless :)

I also have the impression that this PR may change the defaults on ColorBarItem a little too far in the direction of the "animated image" use case

I have not (intentionally) changed the defaults of ColorBarItem other than the, already mentioned, colormap prioritization which is changed from first to last image in the supplied list.

As far as I can tell, there are actually three typical uses

There is a fourth use case, which I'd argue is quite commonly needed for animated images (I agree that usecase (2) will be the most common for still images).

(4) Plot an image, keep the current scaling, do not allow interaction to manually change it.

This usecase was the whole rationale behind the, already mentioned, PR where self.enableAutoLevels was introduced.

"Interactive ColorBarItems will always attempt to disable auto-scaling for its assigned image items.": Will that still work conveniently with case (2)?

That was my intention at least, but I realize now that I may have misunderstood your three usecases slightly. I guess I may have forgot to account for the "middle state" between plotting a single image and plotting an animation, which is reusing the same "static image" plot for a new, totally unrelated, static image that doesn't need to be compared with the previous dataset. I'd have to play around a bit with that to see how it would work.

If there's a displayed color bar, it should always match what is displayed in the attached plot or plots. Other behavior is an invitation to messed up science and disaster in general.

Yes, I totally agree. This is what I attempted to solve with the sigLevelsChanged-signal that I refer to in my original PR comment: "The way I've solved this is by adding a signal that PColorMeshItem can use to notify ColorBarItem that it has changed its values, so that the ColorBarItem can update accordingly. [...] So far, I've only implemented this signal in PColorMeshItem because I want to hear some thoughts on this idea before potentially expanding ImageItem as well."

@SimenZhor , I think you got all the points I was trying to make 👍

Sorry about getting the example mixed up with the main code, I totally withdraw that objection.

@NilsNemitz @j9ac9k Sorry for taking such a long time to finish this PR. I think I've now made the requested changes from Nils' earlier comment, except that I've not reverted the change of the variable named self.enableAutoLevels to self.enableautolevels. The reason for this is that I felt that enableAutoLevels was a more appropriate method name than a variable name, and that the variable was introduced by me in #2463, which is not yet included in any releases.

Regarding this comment:

I have a suspicious eye on the self.setLevels(img_levels, update_items=False) in L214 of ColorBarItem

The reason that update_items is False here is because this line is triggered when the levels are changed from the image item instead of from the color bar, and leaving it to True would cause an eternal recursive loop.

So, I think I'll finally change this PR from draft to ready for review.

Bah, I just introduced some merge conflicts for you @SimenZhor if you'd be fine with it, I don't mind resolving them on my end.

No problem @j9ac9k, I’ve been following the dataBounds development as I ran into that issue myself a few weeks ago, so I think I’ll be able to sort out the conflicts some time next week. That being said, I don’t mind at all if you have time to look at it before that.

No problem @j9ac9k, I’ve been following the dataBounds development as I ran into that issue myself a few weeks ago, so I think I’ll be able to sort out the conflicts some time next week. That being said, I don’t mind at all if you have time to look at it before that.

If I'm itching to do a release before you get to it, I'll be sure to patch the conflicts myself first 😆

@j9ac9k The merge conflicts should now be resolved

Thanks for the PR @SimenZhor This LGTM, thanks for reviewing @NilsNemitz Merging!