-
-
Notifications
You must be signed in to change notification settings - Fork 66
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
Conversation
Codecov Report
@@ 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 |
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.
@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?
4a6f1b8
to
c01e403
Compare
@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. |
I a thinking a warning if the default is being used - maybe a FutureWarning is most appropriate? (@Cadair what do you think?) |
Yeah I would say |
8319ce9
to
16fb519
Compare
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 |
@svank Incase you missed it, reproject 0.9 is out with all your PRs in. |
reproject_adaptive
, and removed order
argument.
@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 |
While this kernel introduces some slight blurring of the output image, it significantly enhances the anti-aliasing properties that this algorithm touts.
16fb519
to
85585da
Compare
@astrofrog Done! |
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 usingreproject_adaptive
in their code will see very different results after updating to a version ofreproject
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.