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

Remove warning in Kernel.normalize #5531

Merged
merged 1 commit into from
Dec 2, 2016

Conversation

cdeil
Copy link
Member

@cdeil cdeil commented Dec 1, 2016

Currently Kernel.normalize emits a warning

>>> from astropy.convolution import Tophat2DKernel
>>> Tophat2DKernel(radius=10).normalize('peak')
WARNING: The kernel normalization factor is exceptionally large, > 100. [astropy.convolution.core]

when

if np.abs(1.0 / normalization) > MAX_NORMALIZATION:

where MAX_NORMALIZATION = 100, i.e. if

normalization > 0.01

A tophat kernel with 10 pix radius, normalised to peak value 1 is perfectly OK usage.
There should be no warning.
This pull request removes the check and warning.


FYI: In the past months we've been getting this warning from several places in Gammapy, and I'd prefer to just remove it here instead of having to catch it everywhere.

It looks like this warning was added in #3747 by @larrybradley, a PR that was reviewed by @adonath . As far as I can see, the motivation for #3747 was a different bug and different checks were added for that, and this check wasn't motivated / discussed.

I did discuss this in person with @adonath today and he agrees the warning should go.

@larrybradley - Can you please review this?
If possible I'd like to get this in for the 1.3 release.
Does this need a changelog entry? (I don't think so, but I can add if others think yes)

@larrybradley
Copy link
Member

larrybradley commented Dec 1, 2016

@cdeil I didn't add the normalization warning. In #3747, I simply fixed it for the case of zero-sum kernels.

The normalization warning was first added by @adonath in #1255. (b830674#diff-ae1e779ed6e36f7c08dd3e6841ae6538R38)

I actually never liked this warning, so I'm happy to see it removed! 😃.

I don't think it needs a changelog entry.

@larrybradley
Copy link
Member

You still need to remove the normalization tests that check for the warning. That's why the tests are failing.

@cdeil
Copy link
Member Author

cdeil commented Dec 1, 2016

I actually never liked this warning, so I'm happy to see it removed! 😃.

Looks like this is my first Astropy PR that's uncontroversial, then.
Maybe I should make more of those ... fingers crossed.
:-)

You still need to remove the normalization tests that check for the warning. That's why the tests are failing.

I've amended the commit to remove the superfluous test.
This is ready for final review.

@pllim
Copy link
Member

pllim commented Dec 1, 2016

Is tagging 1.3 now too ambitious?

@astrofrog astrofrog added this to the v1.3.0 milestone Dec 1, 2016
@astrofrog
Copy link
Member

@pllim - no :)

@larrybradley larrybradley merged commit 5a776fe into astropy:master Dec 2, 2016
@cdeil
Copy link
Member Author

cdeil commented Dec 2, 2016

@larrybradley - Thank you for the quick review and merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants