Skip to content
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

API: add antialiased to interpolation-stage in image #28061

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Apr 12, 2024

This PR changes the default interpolation_stage in imshow (and friends) to 'antialiased'.

'antialiased' follows interpolation 'antialiased' in behaving differently if the image is upsampled by more than a factor of three or if it is weakly upsampled or downsampled. If upsampled, interpolation_stage will happen at the data stage, like the previous default. If downsampled, interpolation_stage has been changed to happen at the RGBA stage.

When upsampling and using a smoother on the pixels, interpolation in RGBA space leads to non-sensical colors, and was the original motivation behind #5718. Hence upsampling continues to happen in data space as has been the default since 2.0

sphx_glr_image_antialiasing_010_2_00x-2

Conversely, downsampling should have an anti-aliasing filter applied, but this leads to all sorts of strange effects if applied at the data stage (eg, #21167, and many other fixes since #5718 went in). The new behaviour is to sidestep all these issues and do the interpolation in RGBA space if downsampling.

sphx_glr_image_antialiasing_008_2_00x

See the much longer description at https://output.circle-artifacts.com/output/job/edd10dae-2a13-48cf-af33-dca97ed402eb/artifacts/0/doc/build/html/gallery/images_contours_and_fields/image_antialiasing.html#sphx-glr-gallery-images-contours-and-fields-image-antialiasing-py

@jklymak jklymak force-pushed the api-change-data-interpolation-stage branch from 90b823e to 0c5adf9 Compare April 12, 2024 04:33
@github-actions github-actions bot added topic: rcparams Documentation: examples files in galleries/examples labels Apr 12, 2024
@jklymak jklymak force-pushed the api-change-data-interpolation-stage branch 6 times, most recently from 6ad31ab to 73269ae Compare April 15, 2024 04:05
@jklymak jklymak marked this pull request as ready for review April 15, 2024 15:16
if (dispx < 3) or (dispy < 3):
interpolation_stage = 'rgba'
else:
interpolation_stage = 'data'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure there's a really sane default in the (pathological) case of an image being stretched along x and squeezed along y, but I'd have thought that the "neutral" solution would be to set the cutoff at dispx * dispy < 1 (or < 3)? i.e. rgba if there's fewer total pixels at the end than at the beginning, data, if there's more?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm erring on the side of using 'rgba' if any down-sampling is occurring - 'rgba' works exactly the same as 'data' if there is up-sampling and interpolation is 'nearest', which is the default. The only issue with 'rgba' happens when the user wants to smooth upsampled pixels, and specifies 'sinc' or 'bilinear' (etc) smoothing. Given that, I'm willing to ask such users to definitively specify 'data' if they are on the cusp of downsampling and getting weird artifacts.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we stuff this and the kernel selection into a helper function so that they do not get out-of-sync?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I tried that but the kernel selection is buried at a different level in the code (in _resample) with different ways of getting the data and figure size, so I took the easy way out and quasi repeated the logic here. I think a reasonable follow up is simplifying _resample and this part of the code base.

@jklymak jklymak force-pushed the api-change-data-interpolation-stage branch from 73269ae to 2679f98 Compare April 15, 2024 16:48
@anntzer
Copy link
Contributor

anntzer commented Apr 15, 2024

As briefly mentioned during the call, perhaps the option could be named in a way that does not preclude the future addition of an interpolation_stage="norm" ("normed", i.e. post-normalized data) option, and thus also of interpolation_stage="norm" when upsampling / "rgba" when downsampling. For example the mixed options could be named "up:data,down:rgba" (so that "up:norm,down:rgba" can also be supported in the future)? (Exact names up to bikeshedding...)

@jklymak
Copy link
Member Author

jklymak commented Apr 15, 2024

I don't think that is a problem. However we still need a default, and I think using 'antialiased', the same as the interpolation keyword default, makes sense. Would it be a problem for that to by synonymous with 'up:data;down:rgba' if we wanted to offer that?

@anntzer
Copy link
Contributor

anntzer commented Apr 16, 2024

I guess the question is whether we'd be happy to change the meaning of "antialiased" to mean "up:norm,down:rgba" (which I consider a saner default) once "norm" comes in?

@jklymak
Copy link
Member Author

jklymak commented Apr 16, 2024

Oh, I see, sorry didn't catch on to that.

Thats always an interesting question if one interpolates linearly or in log space. In my field, people sometimes interpolate in log space, and then say incorrect things about the (arithmetic) mean. Though I agree that there are aesthetic reasons to interpolate in log space depending on the problem.

If we were to add a norm stage of interpolation, as a default, I would tend to err on the side of simple averaging (eg up:data) rather than after normalization. At the lowest level it's easiest to explain. However, if there is a consensus that we should default to interpolating after the norm, maybe we should try and put that in during the same cycle as this.

@anntzer
Copy link
Contributor

anntzer commented Apr 16, 2024

maybe we should try and put that in during the same cycle as this.

OK; I guess this is not going to make it to 3.9(?) (If it does, let's just go with "antialiasing" for now and explicitly mark this name as experimental (...but default) and reserve the right to change it later.)
Assuming this goes in 3.10 we should also get the stop-remapping-to-0/1 in as well, and probably first, as that should make interpolation_stage="norm" somewhat easier to implement (_make_image is already super unwieldy).

@tacaswell tacaswell added this to the v3.10.0 milestone Apr 16, 2024
Comment on lines 176 to 178
# recommend the default *interpolation* of 'hanning'/'antialiased', and
# *interpolation_stage* of 'rgba'/'antialiased' for most down-sampling
# situations (last panel).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# recommend the default *interpolation* of 'hanning'/'antialiased', and
# *interpolation_stage* of 'rgba'/'antialiased' for most down-sampling
# situations (last panel).
# recommend the either using default *interpolation* of 'hanning'/'antialiased',
# and *interpolation_stage* of 'rgba'/'antialiased' for most down-sampling
# situations (last panel) or *interpolation* of 'nearest' and *interpolation_stage*
# of 'data'.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems more confusing than helpful. We've already said what the different arguments do, and we have a default that we recommend.

#
# The second method is to exactly match the size of your axes to the size of
# your data. in the following, the figure is exactly 2 inches by 2 inches, and
# the dpi is 200, so the 400x400 data is not resampled at all. If you download
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is some normal/high dpi magic going on with saved images so it is either 100 or 200 based on the screen the reader has.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK< I put the dpi in as hard coded. Not sure what happens for a low dpi screen if I do this.

@@ -552,7 +566,7 @@ def _make_image(self, A, in_bbox, out_bbox, clip_bbox, magnification=1.0,
cbook._setattr_cm(self.norm, vmin=s_vmin, vmax=s_vmax):
output = self.norm(resampled_masked)
else:
if A.ndim == 2: # _interpolation_stage == 'rgba'
if A.ndim == 2: # interpolation_stage == 'rgba'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do this as an elif and add an else that raises? Or do we do sufficient validation else where that we are sure it will always be one of the valid keys when we get here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, keys are validated elsewhere, and this needs to be an else because it covers both ndim>2 and rgba.


grid = np.random.rand(4000, 4000)
ax = fig_ref.subplots()
ax.imshow(grid, interpolation='antialiased', cmap='viridis',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ax.imshow(grid, interpolation='antialiased', cmap='viridis',
ax.imshow(grid, interpolation='hanning', cmap='viridis',

leave nothing to chance?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well the opposite - I'd like this to fail if we change the default, so at least we do it purposefully?

@tacaswell
Copy link
Member

Overall 👍🏻 thank you for putting in the leg work on this @jklymak !

I think we should target this for 3.10 to give us enough time to get early users and to get the additional changes proposed by @anntzer in.

I left word choice edits that I think soften the prose a bit without compromising the core meaning.

@anntzer
Copy link
Contributor

anntzer commented Apr 16, 2024

(I didn't mention this but the docs are awesome.)

@tacaswell
Copy link
Member

(I didn't mention this but the docs are awesome.)

I completely agree! Should have lead with that instead of a bunch of picky edits.

@jklymak
Copy link
Member Author

jklymak commented Apr 16, 2024

Edits all seem good, I'll put those in. I think 3.10 is completely reasonable, and happy to work on the rest of the chain, but maybe in a few weeks. The todos are to simplify the 0-1 clipping issues, and then add a Norm interpolation stage (which would be easier if the 0-1 clipping issue were cleared up.

I think this could go in when I make the edits above, and then update if we get the rest in. Some of the docs here will need to change and expand anyhow, and it would be easier to start with what is here than amalgamate

@jklymak jklymak force-pushed the api-change-data-interpolation-stage branch 2 times, most recently from eefc329 to 6791fb1 Compare April 17, 2024 16:56
@jklymak
Copy link
Member Author

jklymak commented Apr 17, 2024

If possible, I'd like to get this in before moving on to the next tasks, (fixing the float scaling) and adding a "norm" interpolation_stage. Working off these changes will be easier than adding those features, going back to these changes and rebasing. The only drawback is that there may be a bit of test image churn, but I think it will be minimal.

jklymak and others added 6 commits April 22, 2024 12:03
imshow used to default to interpolating in data space.  That
makes sense for up-sampled images, but fails in odd ways for
down-sampled images.

Here we introduce a new default value for *interpolation_stage*
'antialiased', which changes the interpolation stage to 'rgba'
if the data is downsampled or upsampled less than a factor of three.
Co-authored-by: Thomas A Caswell <tcaswell@gmail.com>
@jklymak jklymak force-pushed the api-change-data-interpolation-stage branch from d4cd758 to afbdfd2 Compare April 22, 2024 16:03
Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome improvement! 🚀

doc/api/next_api_changes/behavior/28061-JMK.rst Outdated Show resolved Hide resolved
is carried out on the data provided by the user. If 'rgba', the
interpolation is carried out after the colormapping has been
applied (visual interpolation).
interpolation_stage : {'antialiased', 'data', 'rgba'}, default: 'antialiased'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
interpolation_stage : {'antialiased', 'data', 'rgba'}, default: 'antialiased'
interpolation_stage : {'antialiased', 'data', 'rgba'}, default: :rc:`interpolation_stage`

interpolation_stage : {'antialiased', 'data', 'rgba'}, default: 'antialiased'
If 'data', interpolation is carried out on the data provided by the user.
If 'rgba', the interpolation is carried out in RGBA-space after the
color-mapping has been applied (visual interpolation). If 'antialiased',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is 'antialiased' the correct name?

  • The other two options 'data' and 'rgba' refer to types of data in the processing pipeline. In contrast 'antialiased' is an effect.

  • From https://en.wikipedia.org/wiki/Spatial_anti-aliasing:

    In digital signal processing, spatial anti-aliasing is a technique for minimizing the distortion artifacts (aliasing) when representing a high-resolution image at a lower resolution.

    In my understanding anti-aliasing is only connected to downsampling. It's thus surprising that a mode 'antialiased' tries to
    cover down-sampling and up-sampling.

Suggestion: Simply use 'auto', which hints at "use the best (from existing modes 'data', 'rgba') for the given context".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'antialiased' is meant to be the same for both interpolation and interpolation_stage keyword arguments. The analogy is with most viewers, which have an "antialiased" toggle, regardless of whether the image is actually up or downsampled.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. But still the name does not speak to me. I recommend to get some opinions from core devs. If they are fine with the name, I'll give in.

lib/matplotlib/axes/_axes.py Show resolved Hide resolved
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants