Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
base: main
Are you sure you want to change the base?
API: add antialiased to interpolation-stage in image #28061
Changes from 6 commits
51a7455
11f0359
122aaed
a8cf3b0
e64f8ba
afbdfd2
2e22643
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
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 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".
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.
'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.
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 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.
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 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:
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).
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.
Well, 'antialiased' is pretty new for interpolation. We could keep it, but add 'auto' and make that the default for both interpolation and interpolation_stage. A bit of doc rewriting...
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 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?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 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.
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.
can we stuff this and the kernel selection into a helper function so that they do not get out-of-sync?
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.
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.