Convolution: treat infs as nans by keflavich · Pull Request #18364 · astropy/astropy

keflavich · 2025-06-28T17:57:34Z

Resolves #8099.

convolve_fft has always treated inf as nan, since it is not possible to do otherwise and get a usable result.

convolve has allowed infs to be propagated around by the kernel size.

from astropy.convolution import convolve, convolve_fft, Gaussian1DKernel
ar = np.ones(10)
ar[5] = np.inf
gaussian = Gaussian1DKernel(1, x_size=5)
convolve_fft(ar, gaussian)
 # array([0.70130997, 0.94551132, 1.        , 1.        , 1.        ,
 #      1.        , 1.        , 1.        , 0.94551132, 0.70130997])
convolve(ar, gaussian)
 # array([0.70130997, 0.94551132, 1.        ,        inf,        inf,
 #             inf,        inf,        inf, 0.94551132, 0.70130997])

This PR prevents the latter behavior. I think it is a correct solution, but it is theoretically possible that someone has relied on this 'bug' over the last 7 years, so it warrants allowing for some minimal discussion.

github-actions · 2025-06-28T17:57:54Z

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

Do the proposed changes actually accomplish desired goals?
Do the proposed changes follow the Astropy coding guidelines?
Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

github-actions · 2025-06-28T17:57:56Z

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

github-actions · 2025-11-26T05:47:03Z

Hi humans 👋 - this pull request hasn't had any new commits for approximately 4 months. I plan to close this in 30 days if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock.

If you believe I commented on this pull request incorrectly, please report this here.

keflavich · 2025-11-28T12:37:48Z

I used an LLM to write the tests, but I checked the numerical values by hand (they got them wrong sometimes anyway... unfortunately my by-hand checking is not guaranteed to be correct at only 0.75 coffees into the day).

TODO: Why is the coverage <100%?

pllim · 2025-11-28T17:56:07Z

Why is the coverage <100%?

The report in Details might give you clue. A rebase might also help (but maybe not but doesn't hurt).

keflavich · 2025-11-28T17:57:28Z

thanks. i thought i had rebased already, but maybe i lost this one in the mix...

pllim · 2025-11-28T17:59:30Z

Well, I am just conveying that GH is telling me. 😅

keflavich · 2025-11-28T18:18:30Z

I had rebased, but apparently hadn't fetched recently enough.

pllim · 2025-11-28T18:23:58Z

Still need that change log fragment. Thanks!

github-actions · 2026-04-28T06:28:00Z

Hi humans 👋 - this pull request hasn't had any new commits for approximately 4 months. I plan to close this in 30 days if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock.

If you believe I commented on this pull request incorrectly, please report this here.

larrybradley

Thanks, @keflavich. I have a few comments to address.

As a general comment (not for this PR), we should eventually clean up the tests to parameterize convolve/convolve_fft. There is a lot of duplicated test code.

Also, this PR has merge conflicts and needs a rebase.

larrybradley · 2026-04-28T22:40:01Z

+    convolve_fft should treat infinities like NaN values.
+    Regression test for issue #8099.
+    """
+    from astropy.convolution import Gaussian1DKernel


Move import to top of file.

larrybradley · 2026-04-28T23:03:44Z

+    assert not np.any(np.isnan(result))
+
+
+def test_convolve_fft_kernel_with_inf():


This test is identical to test_kernel_with_inf. Remove one of them.

I bet this was from a bad rebase way back months ago...

Co-authored-by: Larry Bradley <[email protected]>

larrybradley · 2026-04-29T22:42:17Z

pre-commit.ci autofix

larrybradley · 2026-04-29T22:42:46Z

@keflavich This PR stills need a rebase to resolve merge conflicts.

github-actions · 2026-10-15T05:44:43Z

Hi humans 👋 - this pull request hasn't had any new commits for approximately 6 months. I plan to close this in a month if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock.

If you believe I commented on this pull request incorrectly, please report this here.

github-actions Bot added the convolution label Jun 28, 2025

pllim added this to the v7.2.0 milestone Jun 30, 2025

astrofrog modified the milestones: v7.2.0, v9.0.0, v8.0.0 Nov 3, 2025

github-actions Bot added the Close? Tell stale bot that this issue/PR is stale label Nov 26, 2025

keflavich force-pushed the issue8099 branch from ab119f1 to bfe6bb3 Compare November 28, 2025 12:21

keflavich removed the Close? Tell stale bot that this issue/PR is stale label Nov 28, 2025

keflavich added 2 commits November 28, 2025 13:18

treat infs as nans

3b09eca

more and more accurate error messages, plus extensive regression tests

2414258

keflavich force-pushed the issue8099 branch from bfe6bb3 to 2414258 Compare November 28, 2025 18:18

keflavich marked this pull request as ready for review November 28, 2025 18:18

keflavich requested a review from larrybradley as a code owner November 28, 2025 18:18

add changelog entry

3dd1679

github-actions Bot added the Close? Tell stale bot that this issue/PR is stale label Apr 28, 2026

keflavich added keep-open and removed Close? Tell stale bot that this issue/PR is stale labels Apr 28, 2026

larrybradley requested changes Apr 28, 2026

View reviewed changes

Update astropy/convolution/convolve.py

228d858

Co-authored-by: Larry Bradley <[email protected]>

keflavich and others added 4 commits April 28, 2026 20:17

Update docs/changes/convolution/18364.api.rst

316d27a

Co-authored-by: Larry Bradley <[email protected]>

Update astropy/convolution/convolve.py

d02432e

Co-authored-by: Larry Bradley <[email protected]>

Update astropy/convolution/convolve.py

52f4aff

Co-authored-by: Larry Bradley <[email protected]>

remove one of the redundant regression tests

0d1665a

astrofrog modified the milestones: v8.0.0, v8.1.0 May 5, 2026

pllim removed the keep-open label Oct 14, 2026

github-actions Bot added the Close? Tell stale bot that this issue/PR is stale label Oct 15, 2026

		assert not np.any(np.isnan(result))


		def test_convolve_fft_kernel_with_inf():

Conversation

keflavich commented Jun 28, 2025

Uh oh!

github-actions Bot commented Jun 28, 2025

Uh oh!

github-actions Bot commented Jun 28, 2025

Uh oh!

github-actions Bot commented Nov 26, 2025

Uh oh!

keflavich commented Nov 28, 2025

Uh oh!

pllim commented Nov 28, 2025

Uh oh!

keflavich commented Nov 28, 2025

Uh oh!

pllim commented Nov 28, 2025

Uh oh!

keflavich commented Nov 28, 2025

Uh oh!

pllim commented Nov 28, 2025

Uh oh!

github-actions Bot commented Apr 28, 2026

Uh oh!

larrybradley left a comment

Choose a reason for hiding this comment

Uh oh!

larrybradley Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

larrybradley Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

keflavich Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

larrybradley commented Apr 29, 2026

Uh oh!

larrybradley commented Apr 29, 2026

Uh oh!

github-actions Bot commented Oct 15, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants