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

Consistently use mode='constant' in convolutions of RingBackgroundEstimator #1204

Merged
merged 2 commits into from Nov 13, 2017

Conversation

Projects
None yet
3 participants
@lmohrmann
Contributor

lmohrmann commented Nov 9, 2017

Commit 6d614a1 has set the mode keyword of scipy.ndimage.convolve to 'constant' for the computation of the background counts, but not for the exposure. This leads to wrong results and should be fixed by this PR.

@lmohrmann lmohrmann requested a review from adonath Nov 9, 2017

@lmohrmann

This comment has been minimized.

Show comment
Hide comment
@lmohrmann

lmohrmann Nov 9, 2017

Contributor

Just noticed that this has already been discussed in #1198. The fix is still needed, though.

Contributor

lmohrmann commented Nov 9, 2017

Just noticed that this has already been discussed in #1198. The fix is still needed, though.

@cdeil cdeil self-assigned this Nov 10, 2017

@cdeil cdeil added the bug label Nov 10, 2017

@cdeil cdeil added this to the 0.7 milestone Nov 10, 2017

@cdeil

@lmohrmann - Please adjust the reference values in this failing test:
https://travis-ci.org/gammapy/gammapy/jobs/299683185#L2641

As mentioned in #1198 further changes and test improvements would be desirable, so if you have time, please do it here. But if no I think we should be pragmatic and merge this, and then when someone has time to work more on the ring background estimation, they can read over #1198 and make another PR in the future.

@lmohrmann

This comment has been minimized.

Show comment
Hide comment
@lmohrmann

lmohrmann Nov 10, 2017

Contributor

@cdeil Tests are fixed now. I'd suggest to merge now, to have this obvious bug fixed.

Contributor

lmohrmann commented Nov 10, 2017

@cdeil Tests are fixed now. I'd suggest to merge now, to have this obvious bug fixed.

@adonath

Looks good to me, I have no further comments.

@cdeil

cdeil approved these changes Nov 13, 2017

@cdeil cdeil merged commit 8b0c092 into gammapy:master Nov 13, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Nov 13, 2017

Member

Thanks!

Member

cdeil commented Nov 13, 2017

Thanks!

@lmohrmann lmohrmann deleted the lmohrmann:ring-bg-conv branch Nov 13, 2017

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