Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
ENH: allow image to interpolate post RGBA#18782
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This just needs tests and improved docs. But the idea is complete and could be discussed.@tacaswell@brunobeltran@efiring@dopplershift@anntzer ? |
ping@tacaswell - if you had time you said you'd weigh in on this one. I actually think this is a pretty simple addition, even if it will be abit tricky to explain... |
5017129 tof50ab9aCompare#19838 is another nice example where data-interpolation on down-sampling is very undesirable. |
f50ab9a to0711ed3Comparejklymak commentedJul 9, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
For Comment: this kwarg is currently |
IIUC#19748 (comment) essentially suggests that this should rather be something like |
OK, so changed to |
40bd8a9 to5560fb0Comparelib/matplotlib/axes/_axes.py Outdated
| @@ -5380,6 +5381,12 @@ def imshow(self, X, cmap=None, norm=None, aspect=None, | |||
| which can be set by *filterrad*. Additionally, the antigrain image | |||
| resize filter is controlled by the parameter *filternorm*. | |||
| interp_space : str, default: None | |||
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.
str ->{'data', 'rgba'}, default: 'data'
lib/matplotlib/axes/_axes.py Outdated
| interp_space : str, default: None | ||
| Supported values are 'data' and 'rgba'. If 'data' interpolation | ||
| is carried out on the data provided by the user. If 'rgba', the | ||
| interpolation is carried out afte the colormapping has been |
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.
after (typo)
lib/matplotlib/axes/_axes.py Outdated
| interpolation=None, alpha=None, vmin=None, vmax=None, | ||
| origin=None, extent=None, *, filternorm=True, filterrad=4.0, | ||
| resample=None, url=None, **kwargs): | ||
| interpolation=None, interp_space=None, alpha=None, |
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 don't thinkanyone is passing alpha positionally, so I guess the API break is safe, but in that case you may as well make all kwargs starting frominterp_space keyword-only (as you effectively broke backcompat on passingalpha positionally).
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.
OK, not intentionally. How far down the stack do we want to break it, if we had our druthers? I'd probably put the* afternorm and beforeaspect?
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.
Seems about right to me. OTOH you can also just stickinterp_space in the kwonly section for now and put a@_api.make_keyword_only("3.5", "aspect") and fix the order in 3.7 to strictly follow the policy. No strong opinion there.
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 deprecated most of them! (ahem, maybe we don't want this, but I really doubt too many people are using positional arguments for all of these)
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.
you only need to do it for the first one; the following ones implicitly become keyword-only as well (because you can't pass them positionally anymore without passing the first one positionally) :)
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.
ooops, duh!
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.
If I do this, I get an error on import
Traceback (most recent call last): File "/Users/jklymak/matplotlib/examples/images_contours_and_fields/image_antialiasing.py", line 20, in <module> import matplotlib.pyplot as plt File "/Users/jklymak/matplotlib/lib/matplotlib/pyplot.py", line 2875, in <module> def imshow( File "/Users/jklymak/matplotlib/lib/matplotlib/pyplot.py", line 101, in _copy_docstring_and_deprecators func = decorator(func) File "/Users/jklymak/matplotlib/lib/matplotlib/_api/deprecation.py", line 417, in make_keyword_only assert (name in signature.parametersAssertionError: Matplotlib internal error: 'aspect' must be a positional-or-keyword parameter for imshow()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.
Oops, I guess (I'm almost sure, actually) that make_keyword_only currently doesn't work with functions that have pyplot wrappers, because make_keyword_only fakes the displayed function signature to pretend that the switch to kwonly already occurred, but the pyplot wrapper should be generated using the old (non-kwonly) signature, but that's not the case right now.
It's going to be slightly annoying to fix but probably just needs some wrangling, can you open a separate issue to track this? In any case I don't think this is required for this PR.
lib/matplotlib/image.py Outdated
| Parameters | ||
| ---------- | ||
| s : str or None | ||
| str should be one on "data", "normed", or "rgba" |
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.
not "normed", for now.
ditto for the parameter description (as above)
lib/matplotlib/image.py Outdated
| s = "data" # placeholder for maybe having rcParam | ||
| if s not in ["data", "rgba"]: | ||
| raise ValueError("interp_space must be one of 'data' " | ||
| "or 'rgba'.") |
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.
_api.check_in_list.
lib/matplotlib/image.py Outdated
| Supported values are 'data' and 'rgba'. If 'data' interpolation | ||
| is carried out on the data provided by the user. If 'rgba', the | ||
| interpolation is carried out afte the colormapping has been | ||
| applied (visual interpolation). |
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.
one space too many before the last opening parenthesis.
lgtm, modulo tests/docs. Thanks for doing this :) |
0c855aa toa90a206Compare@tacaswell I'll try and pop this onto your review queue, at least for high-level comment |
197516b toef3f515CompareThere 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 would like a version of#18487 to go in with this.
I'm still not fully convinced that this is not wrong, but have accepted that my instinct is from a career of working with data from 2D detectors and this behavior is sensible in some domains.
Another way to look at this is that it is putting in an option to get back to the mpl <2.0 behavior.
lib/matplotlib/image.py Outdated
| @@ -885,6 +911,7 @@ def __init__(self, ax, | |||
| cmap=None, | |||
| norm=None, | |||
| interpolation=None, | |||
| interp_space=None, | |||
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 should go at the end (kwarg only preferably), or we need a deprecation note.
| # | ||
| # Conversely, when the anti-aliasing occurs in 'rgba' space, the red and | ||
| # blue are combined visually to make purple. This behaviour is more like a | ||
| # typical image processing package. |
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.
| # typical image processing package. | |
| # typical image processing package but may produce colors in output which are not present | |
| # in the colormap confounding interpretation of the image. |
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.
Changed to
#... This behaviour is more like a# typical image processing package, but note that purple is not in the # original colormap, so it is no longer possible to invert individual# pixels.The point here is that if you have adequate resolution, you shouldnot be inverting individual pixels.
| subsampling the smoothed data. In Matplotlib, we can do that | ||
| smoothing before mapping the data to colors, or we can do the smoothing | ||
| on the RGB(A) data in the final image. The difference between these is | ||
| shown below, and controlled with the *interp_space* keyword argument. |
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 am not a huge fan of this name, I think of this more as changing the order of the pipeline rather than changing the space of the interpolation. I agree it does change the space, but the re-ordering (from resample -> norm -> colormap to norm -> colormap -> resample) is the key difference.
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'm not wed to the name, which was suggested by@anntzer, though I think its pretty good:interp_stage?interp_step?
I don't have any problem with that. Are you suggesting it should be part of this PR? |
3f07bff to548a2dfCompareName of the kwarg has been changed to |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
8793e03 toc460dbcCompare
Uh oh!
There was an error while loading.Please reload this page.
PR Summary
This implements a new boolean kwarg
interp_postrgbatoimshowthat implements the interpolation after the data has been normed and mapped to RGBA.Motivation
In the depths of time, we interpolated images in RGBA space, but this was deemed unacceptable in that it produces image pixels that are not in the colormap. So the interpolation was moved to be before the norm and colormapping. That has proven troublesome with respect to over/under data, but generally seems to work.
However, there are many cases, particularly when trying to antialias, that we really want to average in RGBA space, and allow colors that are out of the colormap because they visually merge adjacent pixels.
Proposal:
A kwarg,
interp_spacethat allows the interpolation to be carried out before or after the RGBA mapping has been made. The way this is implemented now, it is just a toggle - i.e. you cannot interpolate both beforeand after the RGBA mapping. That is not impossible, but probably not typically needed or desired, and would make the code considerably messier. As it is, its quite a straightforward change.Note the change could be made a level higher in
_axes.imshow, however, this implementation allows us to skip all the mapping jiggery that we do for data space.Result
400 dpi
At 400 dpi the images are not downsampled, so all three versions are the same
200 dpi
Notice how at 100 dpi the alternating blue/red in the data interpolation go to white (the average of -1 and 1), with some obvious residual aliasing. The RBGA interpolation goes to purples (red+blue = purple).
100 dpi
50 dpi
Note that pixel peeping at the "over" and "under" you can definitely see pixels that leak out of the colormap (i.e. cyan+purple, or yellow + blue).
PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsandpydocstyle<4and runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).