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

Migrate ring background to use gammapy.maps #1582

Merged
merged 7 commits into from Jul 28, 2018

Conversation

2 participants
@registerrier
Copy link
Contributor

registerrier commented Jul 27, 2018

This is the initial commit of the PR, don't review yet.
The code works. Still need to check things and adapt tests.

@registerrier registerrier self-assigned this Jul 27, 2018

@registerrier registerrier added this to To do in Map analysis via automation Jul 27, 2018

@registerrier registerrier added this to the 0.8 milestone Jul 27, 2018

registerrier added some commits Jul 28, 2018

@registerrier registerrier moved this from To do to In progress in Map analysis Jul 28, 2018

@registerrier

This comment has been minimized.

Copy link
Contributor Author

registerrier commented Jul 28, 2018

The migration is done, please review PR.

It works for multidimensional maps, too. The same ring estimator is applied to all 2D images in the map.

@registerrier registerrier requested a review from cdeil Jul 28, 2018

@cdeil

cdeil approved these changes Jul 28, 2018

Copy link
Member

cdeil left a comment

@registerrier - Thanks!

I did some follow-up edits in 4dbbc78

I had to change to float64 images to get the tests to pass (or reduce accuracy from rtol 1e-7 to 1e6)

I did remove the one-line helper functions to convert between the ring parameters from the docs by removing them from the __all__ list.

I'll merge this now.

One thing I find weird is that the normal ring uses scipy convolve and has no FFT option, and the adaptive ring uses astropy convolve and has an FFT option. Shouldn't it be the same, either FFT is useful to offer or not?

Another suggestion would be to make the test case the same, and to test the FFT option. Pick one good test case and use it for both normal and adaptive ring test, no?

I don't consider those suggestions high priority, so didn't spend time on those here. Just mentioning them since I noticed it here. If you agree, please open a reminder issue to follow up (for v0.9 milestone)

@cdeil cdeil merged commit 9d825da into gammapy:master Jul 28, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

Map analysis automation moved this from In progress to Done Jul 28, 2018

@cdeil

This comment has been minimized.

Copy link
Member

cdeil commented Jul 28, 2018

@registerrier - Thanks!

@cdeil cdeil added the cleanup label Jul 29, 2018

@cdeil cdeil changed the title Adapting RingBackground to Map Migrate ring background to use gammapy.maps Aug 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.