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

Implement ReflectedRegionsBackgroundMaker #2475

Merged
merged 6 commits into from Oct 18, 2019

Conversation

@adonath
Copy link
Member

adonath commented Oct 18, 2019

This PR implements a ReflectedRegionsBackgroundMaker and a small first test in test_reflected.py. Other existing tests will be moved over step by step in following PRs.

gammapy/spectrum/reflected.py Outdated Show resolved Hide resolved
@cdeil cdeil added this to the 0.15 milestone Oct 18, 2019
@cdeil cdeil added the cleanup label Oct 18, 2019
@cdeil cdeil added this to To do in gammapy.spectrum via automation Oct 18, 2019
self.max_region_number = max_region_number

def _get_finder(self, observation):
finder = ReflectedRegionsFinder(

This comment has been minimized.

Copy link
@cdeil

cdeil Oct 18, 2019

Member

Another coding style suggestion. If a temp variable is just created and returned, but never used, I suggest to put return directly and avoid the temp variable. I've changed to that coding pattern already mostly throughout Gammapy

This comment has been minimized.

Copy link
@adonath

adonath Oct 18, 2019

Author Member

Done

gammapy/spectrum/reflected.py Outdated Show resolved Hide resolved
Copy link
Contributor

registerrier left a comment

Thanks @adonath.
A small issue with the dataset.counts.copy() that should yield a non empty CountsSpectrul

wcs = finder.reference_map.geom.wcs
events_off = observation.events.select_region(region_union, wcs)

counts_off = dataset.counts.copy()

This comment has been minimized.

Copy link
@registerrier

registerrier Oct 18, 2019

Contributor

This should copy the contents as well. It should be created from scratch to have an empty vector

This comment has been minimized.

Copy link
@adonath

adonath Oct 18, 2019

Author Member

Ouch...that's indeed a serious bug. I guess I'd have noticed, when moving the existing tests over, but it's good to fix it now. Thanks!

This comment has been minimized.

Copy link
@adonath

adonath Oct 18, 2019

Author Member

No, actually I realised CountsSpectrum.fill_events() overwrites the existing data (see here). This makes the behaviour different from Map.fill_by_coord() which adds the data. I guess we should make it consistent...

This comment has been minimized.

Copy link
@adonath

adonath Oct 18, 2019

Author Member

I changed to not copy and will do the consistency change in a follow up PR (it might affect other parts in gammapy.spectrum)...

min_distance_input="0.1 rad",
max_region_number=10000,
exclusion_mask=None,
binsz="0.01 deg",

This comment has been minimized.

Copy link
@registerrier

registerrier Oct 18, 2019

Contributor

If there is a default value, it is better to state it in the docstring no?

This comment has been minimized.

Copy link
@adonath

adonath Oct 18, 2019

Author Member

I think at some point we decided (not sure if it's documented somewhere), that it's easier to maintain, if the default values are not repeated in the docstring again. Users always see the defaults together with the docstring when they use e.g. help(ReflectedRegionsBackgroundMaker) or ReflectedRegionsBackgroundMaker? in interactive environments. The same goes for the online documentation see e.g. here. So the information would be basically repeated...

@adonath adonath merged commit 80706e8 into gammapy:master Oct 18, 2019
9 checks passed
9 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Scrutinizer Analysis: 7 updated code elements – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
gammapy.gammapy Build #20191018.14 succeeded
Details
gammapy.gammapy (DevDocs) DevDocs succeeded
Details
gammapy.gammapy (Lint) Lint succeeded
Details
gammapy.gammapy (Test Python36) Test Python36 succeeded
Details
gammapy.gammapy (Test Windows36) Test Windows36 succeeded
Details
gammapy.gammapy (Test Windows37) Test Windows37 succeeded
Details
gammapy.spectrum automation moved this from To do to Done Oct 18, 2019
@adonath adonath changed the title Implement `ReflectedRegionsBackgroundMaker` Implement ReflectedRegionsBackgroundMaker Nov 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
3 participants
You can’t perform that action at this time.