Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

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

Merged

Conversation

jklymak
Copy link
Member

@jklymakjklymak commentedApr 12, 2024
edited
Loading

UPDATE: 6 July 2024:

This PR changes the defaultinterpolation_stage in imshow (and friends) to 'auto'.

It nowalso changes the defaultinterpolation from 'antialiased' to 'auto'.

'auto' followsinterpolation 'auto' 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 athttps://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

@jklymakjklymakforce-pushed theapi-change-data-interpolation-stage branch from90b823e to0c5adf9CompareApril 12, 2024 04:33
@jklymakjklymakforce-pushed theapi-change-data-interpolation-stage branch 6 times, most recently from6ad31ab to73269aeCompareApril 15, 2024 04:05
@jklymakjklymak marked this pull request as ready for reviewApril 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 atdispx * 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
MemberAuthor

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 andinterpolation 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.

anntzer reacted with thumbs up emoji
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
MemberAuthor

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.

@jklymakjklymakforce-pushed theapi-change-data-interpolation-stage branch from73269ae to2679f98CompareApril 15, 2024 16:48
@anntzer
Copy link
Contributor

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
MemberAuthor

jklymak commentedApr 15, 2024
edited
Loading

I don't think that is a problem. However we still need a default, and I think using 'antialiased', the same as theinterpolation 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

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
MemberAuthor

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 (egup: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

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).

@tacaswelltacaswell added this to thev3.10.0 milestoneApr 16, 2024
@jklymakjklymakforce-pushed theapi-change-data-interpolation-stage branch 2 times, most recently fromeefc329 to6791fb1CompareApril 17, 2024 16:56
@jklymak
Copy link
MemberAuthor

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.

@jklymakjklymakforce-pushed theapi-change-data-interpolation-stage branch fromd4cd758 toafbdfd2CompareApril 22, 2024 16:03
Copy link
Member

@timhoffmtimhoffm 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! 🚀

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.

  • Fromhttps://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
MemberAuthor

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 bothinterpolation andinterpolation_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.

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 the trap were are in here is that for the interpolation kernel we went with "antialiased" (maybe "auto" would have been better there, but I think the case is less good there than here (as it only considers 2 of many possible. Either way, that is out the door and we should not change it). That leaves us with the choice between:

  • use a better name here ("auto") but have the difference between the two which makes it rougher for users
  • use a less-good name ("antialaised") but better coherence for users

I come down on the side of "use the same name" when looking at the aggregate (even if though I think locally "auto" is better).

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Well, 'antialiased' is pretty new forinterpolation. We could keep it, but add 'auto' and make that the default for bothinterpolation andinterpolation_stage. A bit of doc rewriting...

timhoffm reacted with thumbs up emoji
Copy link
Contributor

Choose a reason for hiding this comment

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

I think "auto" would be better for both as it implies some magic selection is going on for the user to determine what is best. I agree, it seems like we coudl point "antialiased" -> "auto" if a user inputs "antialiased" and just rewrite the docs to make everything show "auto" now.

axs[0].set_title("interpolation='antialiased', stage='rgba'")
axs[0].imshow(np.triu(img), cmap=cm, norm=norm, interpolation_stage='rgba')

# Should be same as previous
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you do a figure reference/test comparison test here instead with these two axes?

@greglucasgreglucas mentioned this pull requestJun 29, 2024
5 tasks
@jklymakjklymakforce-pushed theapi-change-data-interpolation-stage branch from2e22643 tod0e3ce8CompareJuly 6, 2024 16:44
@jklymak
Copy link
MemberAuthor

OK, changed the default from 'antialiased' to 'auto'

Also changedinterpolation default to 'auto' from 'antialiased'. Kept 'antialiased' for back compatibility, but should behave exactly as 'auto'.

Copy link
Member

@timhoffmtimhoffm left a comment

Choose a reason for hiding this comment

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

Only minor documentation suggestions.

jklymak reacted with thumbs up emoji
jklymakand others added2 commitsJuly 8, 2024 13:45
imshow used to default to interpolating in data space.  Thatmakes sense for up-sampled images, but fails in odd ways fordown-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.Apply suggestions from code reviewCo-authored-by: Thomas A Caswell <tcaswell@gmail.com>
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
@jklymakjklymakforce-pushed theapi-change-data-interpolation-stage branch frombd64bfc to45fac07CompareJuly 8, 2024 20:46
@greglucasgreglucas merged commit8b33378 intomatplotlib:mainJul 12, 2024
44 checks passed
@greglucas
Copy link
Contributor

Thanks for putting in the work to make these changes,@jklymak!

@jklymak
Copy link
MemberAuthor

Thanks for the help - the is a relatively big change, but hopefully will not cause too much blowback and reduce bug reports for weird under sampled images...

@jklymakjklymak deleted the api-change-data-interpolation-stage branchJuly 12, 2024 19:23
@tacaswell
Copy link
Member

Thank you everyone!

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@tacaswelltacaswelltacaswell left review comments

@anntzeranntzeranntzer left review comments

@timhoffmtimhoffmtimhoffm approved these changes

@greglucasgreglucasgreglucas approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
v3.10.0
Development

Successfully merging this pull request may close these issues.

5 participants
@jklymak@anntzer@tacaswell@greglucas@timhoffm

[8]ページ先頭

©2009-2025 Movatter.jp