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

Apply FOV mask to all maps in ring background estimator #1354

Merged
merged 2 commits into from Mar 27, 2018

Conversation

Projects
None yet
3 participants
@lmohrmann
Copy link
Contributor

commented Mar 27, 2018

With this PR, the OFF counts and exposure as computed by the RingBackgroundEstimator are set to zero where the ON exposure is zero. This is in agreement with the AdaptiveRingBackgroundEstimator, where this is done as well.

I've also modified the test such that this is checked.

@cdeil cdeil added the cleanup label Mar 27, 2018

@cdeil cdeil added this to the 0.8 milestone Mar 27, 2018

@cdeil
Copy link
Member

left a comment

That seems reasonable to me.

Is this a cosmetic change for pixels that are disregarded later anyways before making e.g. a total background or significance map for several runs?

Or is this a bug fix that will change the total background map (and then source morphology parameters when putting that in a morphology fit)?

@registerrier - Thoughts?

@lmohrmann

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2018

I guess it might change the total background map, at least if you run the RingBackgroundEstimator on the individual runs and stack results later, since the pixels with zero ON exposure usually aren't the same for different runs. Nevertheless, it seems more reasonable to me to zero the pixels for which the ON exposure is zero. As I wrote, this is also how it's handled in the AdaptiveRingBackgroundEstimator.

@registerrier

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2018

This seems fine to me.

@cdeil

This comment has been minimized.

Copy link
Member

commented Mar 27, 2018

I guess it might change the total background map

I'm a bit surprised that this change doesn't lead to a change in the total background map here:

class TestIACTBasicImageEstimator:

See https://travis-ci.org/gammapy/gammapy/jobs/358919662#L2914 from this PR, the test still runs with assert on background image sum unchanged.

Would be good to understand this: maybe the image estimator applied the FOV mask outside the ring estimator? or the test case is bad and for some reason doesn't really assert on changes in the background estimate?

I'll try to extend this test case a bit at

def test_run(self):

and add a commit here to have better asserts / understand this.

@cdeil cdeil changed the title RingBackgroundEstimator: Set OFF counts and exposure to zero where ON exposure is zero Apply FOV mask to all maps in ring background estimator Mar 27, 2018

@cdeil cdeil assigned cdeil and unassigned registerrier Mar 27, 2018

@cdeil cdeil dismissed their stale review via 1f76132 Mar 27, 2018

@cdeil

cdeil approved these changes Mar 27, 2018

Copy link
Member

left a comment

I've done a little bit of code cleanup in 1f76132 .

I noticed that the current image estimator doesn't even store or give access to the n_off map. Given that @registerrier has started to rewrite the map-making code based on gammapy.maps and structured in a simpler way, I don't want to pursue this here and introduce changes / better tests for the maps.

I think the conclusion here is that there was no change in the estimated background map, probably because the IACTImageEstimator applied the right masks outside the RingBackgroundEstimator (which very few people used directly).

Merging now. @lmohrmann - Thanks!

@cdeil cdeil merged commit a861a63 into gammapy:master Mar 27, 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

@lmohrmann lmohrmann deleted the lmohrmann:ring-bg-est branch Mar 28, 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.