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

Changed default filter kernel and boundary mode in reproject_adaptive, and removed order argument. #291

Merged
merged 3 commits into from
Sep 7, 2022

Conversation

svank
Copy link
Contributor

@svank svank commented May 21, 2022

Split from #276

This PR changes the defaults for reproject_adaptive to use the Gaussian kernel, with the Hann left as an option. This is the most significant change in this PR series, as anyone using reproject_adaptive in their code will see very different results after updating to a version of reproject with this change. Their output images will be a bit blurrier, but better anti-aliased. (To be sure, every output image will also change due to the bug fixes, though those changes will hopefully be much smaller.) I'll argue that making this kernel the default is the right choice, though, because it strengthens the anti-aliasing quite a bit, and that anti-aliasing is what sets this algorithm apart from the interpolating and exact algorithms. Using the Gaussian by default offers the best anti-aliasing by default when a user comes to this function for anti-aliasing. I'm happy to discuss this point further and offer plots and examples.

As discussed, this should probably wait until after some sort of deprecation period.

EDIT: To avoid a bunch of merge conflicts, I've moved into this PR from #293 the commit that changes the default kernel. This PR now also removes the warnings we added about these future changes, removes the now-deprecated order argument, and makes notes in CHANGES.md. Should be ready to go for the next-next release.

@codecov
Copy link

codecov bot commented May 21, 2022

Codecov Report

Merging #291 (3f29f13) into main (3f29f13) will not change coverage.
The diff coverage is n/a.

❗ Current head 3f29f13 differs from pull request most recent head 85585da. Consider uploading reports for the commit 85585da to get more accurate results

@@           Coverage Diff           @@
##             main     #291   +/-   ##
=======================================
  Coverage   94.69%   94.69%           
=======================================
  Files          23       23           
  Lines         792      792           
=======================================
  Hits          750      750           
  Misses         42       42           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

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

@svank - sorry for the delay in getting back to these PRs - I've now merged #276 so could you rebase this? (and definitely add an entry in CHANGES.rst for this PR). We can then wait until after the next release to actually merge this change.

Is there a warning yet in main about this upcoming change?

@svank
Copy link
Contributor Author

svank commented Aug 23, 2022

@astrofrog What kind of warning are you thinking about? A mention in the changelog or the docs, or something like a DeprecationWarning when the code runs with the old default in effect? In any case, there is not currently a warning.

@astrofrog
Copy link
Member

I a thinking a warning if the default is being used - maybe a FutureWarning is most appropriate? (@Cadair what do you think?)

@Cadair
Copy link
Member

Cadair commented Aug 24, 2022

Yeah I would say FutureWarning is the most appropriate given this: https://docs.python.org/3/library/warnings.html#warning-categories

@svank svank force-pushed the gaussian-kernel-default branch 2 times, most recently from 8319ce9 to 16fb519 Compare September 1, 2022 22:54
@svank
Copy link
Contributor Author

svank commented Sep 1, 2022

To avoid a bunch of merge conflicts, I've moved into this PR the commit that changes the default kernel. This PR now also removes the warnings we added about these future changes, removes the now-deprecated order argument, and makes notes in CHANGES.md. Should be ready to go for the next-next release.

@svank svank changed the title Use Gaussian kernel by default for adaptive reprojections Make all the pending future changes for adaptive reprojection Sep 1, 2022
@Cadair
Copy link
Member

Cadair commented Sep 5, 2022

@svank Incase you missed it, reproject 0.9 is out with all your PRs in.

@astrofrog astrofrog changed the title Make all the pending future changes for adaptive reprojection Changed default filter kernel and boundary mode in reproject_adaptive, and removed order argument. Sep 6, 2022
@astrofrog
Copy link
Member

@svank - we've updated the infrastructure here to no longer require manual editing of the changelog but instead relying on PR titles, so I've made the title here more descriptive. Could you rebase this PR and remove the changes to CHANGES.md?

While this kernel introduces some slight blurring of the output image,
it significantly enhances the anti-aliasing properties that this
algorithm touts.
@svank
Copy link
Contributor Author

svank commented Sep 6, 2022

@astrofrog Done!

@Cadair Cadair merged commit 1aeb647 into astropy:main Sep 7, 2022
@svank svank deleted the gaussian-kernel-default branch June 23, 2023 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants