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

Fix background image estimate #1331

Merged

Conversation

@registerrier
Copy link
Contributor

@registerrier registerrier commented Feb 28, 2018

This PR is meant to provide a correct IACTBasicImageEstimator with better acceptance map calculation as well as a FoVBackgroundEstimator.

Before:
capture d ecran 2018-02-28 a 23 28 57

After:
figure_2

with Fov background:
figure_3

@registerrier registerrier requested review from cdeil and adonath Feb 28, 2018
@cdeil cdeil added the bug label Feb 28, 2018
@cdeil cdeil added this to the 0.8 milestone Feb 28, 2018
@cdeil
Copy link
Member

@cdeil cdeil commented Feb 28, 2018

@registerrier - I wanted to do the v0.7 release now. OK to put this for the next release? It will be in a few weeks, we need stable releases now. We have other bugs e.g. in 1D spec analysis, so from my side getting one or two more PRs in v0.7 is not important.

When you open a PR, can you please always comment if you're posting it for feedback, or if it's ready for review / merging from your side. Obviously tests that show if this works are missing, but given that we want to write new code based on gammapy.maps to do this anyways very soon, I'd be OK to just merge this in now as-is without tests.


offsets = observation.pointing_radec.separation(acceptance.coordinates())

# This is a somewhat dirty approach to deal with the different background IRFs

This comment has been minimized.

@cdeil

cdeil Feb 28, 2018
Member

You could just add a uniform evaluate on the two background classes for now as a new method, with any name and signature you like. We will have to work on IRF classes for weeks / months, I don't think we should be shy to edit them, we will have to iterate there, and anyways IRFs should be considered low-level, not super stable API for now that most users don't use directly.

This comment has been minimized.

@registerrier

registerrier Mar 2, 2018
Author Contributor

I suppose we won't be supporting EnergyOffsetArray for long so I am not sure it is worth changing the API for this one, no?

This comment has been minimized.

@cdeil

cdeil Mar 2, 2018
Member

Agreed!

@kbruegge
Copy link
Contributor

@kbruegge kbruegge commented Mar 1, 2018

Nice. This means we can remove the old/wrong one right?

Copy link
Member

@cdeil cdeil left a comment

I reviewed this, but apparently forgot to submit my review. Sorry! Here it comes.

This test needs to be updated before this can go in:
https://travis-ci.org/gammapy/gammapy/jobs/347508233#L3766

else:
tmp_array = observation.bkg.evaluate(offset=offsets.ravel(), energy=ebins)

acceptance.data = np.reshape(np.sum(0.5 * np.diff(ebins) * (tmp_array[:-1, :] + tmp_array[1:, :]).T, 1),

This comment has been minimized.

@cdeil

cdeil Mar 2, 2018
Member

This super long line is very hard to read. Can you please split it in two lines and introduce a temp variable?

@@ -8,9 +8,12 @@
from astropy import units as u
from astropy.coordinates import Angle
from astropy.wcs import WCS
from ..utils.energy import Energy
from ..utils.energy import Energy, EnergyBounds
#from ..background import EnergyOffsetArray

This comment has been minimized.

@cdeil

cdeil Mar 2, 2018
Member

Is this used? If no, please remove commented-out code.

else:
norm=1
result = SkyImageList()
result['background'] = SkyImage(data=norm * exposure_on.data, wcs=wcs)

This comment has been minimized.

@cdeil

cdeil Mar 2, 2018
Member

Store the norm in meta? Could be an interesting diagnostic, no?

This comment has been minimized.

@registerrier

registerrier Mar 5, 2018
Author Contributor

Done

@registerrier
Copy link
Contributor Author

@registerrier registerrier commented Mar 5, 2018

This updated version includes corrections for the review.
The test was also failing because of the absence of bkg IRF in some GAMMAPY_EXTRA data. A test is added that will assume a flat acceptance map in case no bkg IRF can be found.
Note that this will create bad results in case a FoVBackgroundEstimator is used without normalization on the counts.

@cdeil
cdeil approved these changes Mar 5, 2018
Copy link
Member

@cdeil cdeil left a comment

@registerrier - merge?

@registerrier registerrier merged commit a1c92be into gammapy:master Mar 5, 2018
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@cdeil cdeil changed the title Correct background iact image Fix background iact image Apr 11, 2018
@cdeil cdeil changed the title Fix background iact image Fix background image estimate Apr 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants