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

Improve reflected background region finder #1616

Merged
merged 4 commits into from Aug 1, 2018

Conversation

@registerrier
Copy link
Contributor

@registerrier registerrier commented Aug 1, 2018

this PR proposes a solution to issue #1449 by adding an optional parameter to take only the first max_region_number regions that come out of the algorithm.

The PR also corrects a bug that could allow the user to create an infinite loop by passing an increment_angle equal to 0.

@registerrier registerrier added the bug label Aug 1, 2018
@registerrier registerrier added this to the 0.8 milestone Aug 1, 2018
@registerrier registerrier self-assigned this Aug 1, 2018
@registerrier registerrier added this to To do in gammapy.makers via automation Aug 1, 2018
@registerrier registerrier requested a review from cdeil Aug 1, 2018
Copy link
Member

@cdeil cdeil left a comment

@registerrier - Thanks!

One suggestion inline above.

Another one would be to improve http://docs.gammapy.org/dev/background/reflected.html a little bit by using this option, and adding links in the text at the top of that page to gammapy.background.ReflectedRegionsFinder so that one can quickly go back and forth between that example page and the API docs.

Finally, it's hard to review this PR locally, because there's fails concerning tevcat_exclusion.fits that was removed a few hours ago. If you want to make that simpler for me, you could rebase this branch against upstream master.

@@ -190,6 +197,9 @@ def find_regions(self):
curr_angle = curr_angle + self.angle_increment

log.debug('Found {} reflected regions'.format(len(reflected_regions)))
if self.max_region_number is not None:

This comment has been minimized.

@cdeil

cdeil Aug 1, 2018
Member

Isn't it easier to move this check up into the while loop, and break if the max is reached?
Should be one line less, and a bit faster, no?

@cdeil cdeil changed the title Correct reflected bkg Improve reflected background region finder Aug 1, 2018
@registerrier registerrier force-pushed the registerrier:correct_reflected_bkg branch from 9a56776 to 1a1530c Aug 1, 2018
…ified check for max_region
@cdeil
cdeil approved these changes Aug 1, 2018
Copy link
Member

@cdeil cdeil left a comment

Thanks!

@cdeil cdeil merged commit 19d5746 into gammapy:master Aug 1, 2018
0 of 2 checks passed
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
gammapy.makers automation moved this from To do to Done Aug 1, 2018
@cdeil
Copy link
Member

@cdeil cdeil commented Aug 1, 2018

Small follow-up in master: 783f660, 7d7f4c3

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

Successfully merging this pull request may close these issues.

None yet

2 participants