-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Simplify normalization of multiple images #11380
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
Conversation
Why is this restricted to images? |
It probably shouldn't be restricted to images or |
I think I am less convinced by the class method. |
How does auto normalization work now ( I’m away from my computer) If this goes in then it might be nice to make sure we don’t have multiple auto scaling algorithms. Overall I’m not really sure this is needed but maybe has a small enough footprint to be useful. |
|
@jklymak auto normalization works via As for the |
Remark on if this is needed
Probably I want to normalize to data more often than to vmin/vmax. One could argue, that I can calculate the respective values and just pass them to the existing parameters. From a minimalistic API point of view that is correct. However, matplotlib has a lot of redundant ways of doing things. Most of them for convenience (e.g. set line properties via kwargs, fmt-string, rc_context, on the created Line2D object). I argue that this is such a case where convenience beats minimalism. In particular, normalizing to data feels quite straight forward.
I'm a bit divided if this method is necessary. On the one hand one can argue that So the question is: Are there significant real world cases in which I cannot use |
""" | ||
self.vmin = _sanitize_extrema(vmin) | ||
self.vmax = _sanitize_extrema(vmax) | ||
self.clip = clip | ||
if data is not None: | ||
self.autoscale_None(data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean self.apply_autoscale
here or am I misunderstanding? I'm not seeing how data can be a list of np arrays if you call autoscale_None
. I don't see that you have a test of this below, i.e. you only call apply_autoscale
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. I mean autoscale_None
. autoscale_None
internally calls np.asanyarray()
on the data, so a list of arrays is fine. I don't think we really need to test this, but if you want I can add a test.
norm = Normalize(data=data)
is really just a shortcut for
norm = Normalize()
norm.autoscale(data)
i.e. it scales to the data. The autoscale_None
variant is used so that you can consistently do Normalize(vmin=0, data=data)
in which case only vmax will be taken from data.
apply_autoscale()
serves a different purpose. While the above works on data, apply_autoscale()
works on AxesImages
. It creates a norm based on the data in the images and applies the norm to the images. Maybe we should better call it autoscale_images()
?
I rarely want to normalize automatically by data (except for initial exploration), and if I do, I don't find doing it by hand too onerous. I don't see what's better about the list comprehension gather versus just a loop that checks for vmin/vmax. Worse, the gather seems memory intensive to me if I have a big data set. Yes, we have lots of redundant ways to do the same thing. I'm not sure thats always a good thing 😉 Not 100% against this if there is really an audience, but it seems crufty to me. |
It probably depends on your application if you want to normalize automatically. No point in arguing individual cases here (neither mine nor yours). The "list comprehension" is actually a generator expression, and if anything less memory consuming and faster than a loop. That's actually what users should use if they have to do this themselves. 😉 Any additional feedback (positive or negative) on |
I think my very first comment still applies. Second, the norm is not updated when the images change. In that sense The new |
Ok, will remove Unfortunately, the existing example does not benefit from the The workaround would be to split the loop and first generate all the data, then create the norm and then plot all the data. But that's not an easy and natural workflow for the example either. |
Maybe it's worth looking into why the following does not work:
If it was working as expected, there wouldn't be any need for the |
The docstring says
Probably, this is done when I pass the norm to Imshow. (Didn‘t check in the code) |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a whats_new entry.
I am a bit concerned about the API growth
What's new added. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather document norm.autoscale(data)
better.
The main issue with the PR, IMO, is that we again end up with "partially overlapping kwargs", namely the behavior when both vmin/vmax and data are passed is less than optimal (IMO).
I'm not seeing enough user-level simplification here to justify the PR. The example can be simplified a bit even without the PR, but I also find there is a bug in part of the example that is not related to the PR: the colorbar is not responding properly to changes in the clim unless applied to |
See #17052 for the bug report promised above. |
Closing because there seems little approval of the idea. I'm not going to fight over it. |
PR Summary
In response to #11377. This adds two simpler ways for creating a common normalization for images.
In particular the linked example is quite verbose. That (at least the first) half gets significantly easier with this PR.
Pre-imshow normalization
if the data are known beforehand, one can create a
Normalize
instance scaled to the global min/max of the data:Post-imshow normalization
Alternatively, one can collect the images and create aNormalize
instance from their values and apply the norm to the images:(withdrawn after discussion)This is a proof of concept. Documentation, tests and examples are not yet complete. Feedback on the general API is much apreciated.
PR Checklist