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

Add EDispMap class #2031

Merged
merged 13 commits into from Feb 18, 2019

Conversation

2 participants
@registerrier
Copy link
Contributor

registerrier commented Feb 14, 2019

This PR introduces the EDispMapclass to be used to contain the energy dispersion at each position in a Map. The 1D EnergyDispersion used to convert from the e_true space to e_reco space is obtained with the EDispMap.get_energy_dispersion(position, e_reco) method.
At the moment, the stacking method works only for maps with identical MapGeom.

There is significant code duplication with PSFMap. This might be partly solved by using inheritance from e.g. a more general IRFMap

@registerrier registerrier requested review from adonath and AtreyeeS Feb 14, 2019

@registerrier registerrier self-assigned this Feb 14, 2019

@registerrier registerrier added this to To do in Map analysis via automation Feb 14, 2019

@registerrier registerrier added this to the 0.11 milestone Feb 14, 2019

@adonath
Copy link
Member

adonath left a comment

Thanks @registerrier! I've left a few minor inline comments. what I'm not happy with is the complexity of the get_energy_dispersion function. I'll take a closer look now and see whether it can be simplified, so that the code duplication does not matter that much....

Parameters
----------
psf : `~gammapy.irf.EnergyDispersion2D`

This comment has been minimized.

Copy link
@adonath

adonath Feb 15, 2019

Member

psf -> edisp

This comment has been minimized.

Copy link
@registerrier

registerrier Feb 15, 2019

Author Contributor

done

edisp_map : `~gammapy.maps.Map`
the input Energy Dispersion Map. Should be a Map with 2 non spatial axes.
migra and true energy axes should be given in this specific order.

This comment has been minimized.

Copy link
@adonath

adonath Feb 15, 2019

Member

Add exposure_map to the docstring...

----------
position : `~astropy.coordinates.SkyCoord`
the target position. Should be a single coordinates

This comment has been minimized.

Copy link
@adonath

adonath Feb 15, 2019

Member

Add e_reco and migra_step here...

This comment has been minimized.

Copy link
@registerrier

registerrier Feb 15, 2019

Author Contributor

OK corrected

Returns
-------
psf_table : `~gammapy.irf.EnergyDependentTablePSF`

This comment has been minimized.

Copy link
@adonath

adonath Feb 15, 2019

Member

The return type is not correctly documented, change to EnergyDispersion

edisp2d = EnergyDispersion2D.from_gauss(etrue, migra, 0.0, 0.2, offsets)

geom = WcsGeom.create(
skydir=pointing, binsz=0.2, width=5, axes=[migra_axis, energy_axis]

This comment has been minimized.

Copy link
@adonath

adonath Feb 15, 2019

Member

Could you use a smaller map for testing? Let's say 5x5 spatial pixels instead of 25 x 25?

This comment has been minimized.

Copy link
@registerrier

registerrier Feb 15, 2019

Author Contributor

Done

@adonath

This comment has been minimized.

Copy link
Member

adonath commented Feb 15, 2019

@registerrier I have some ideas how to simplify .get_energy_dispersion(). If you're OK with that I could do a follow-up PR next week, once this one is merged. So we should just make sure that the test coverage is good enough...

Map analysis automation moved this from To do to In progress Feb 15, 2019

@adonath
Copy link
Member

adonath left a comment

Thanks, @registerrier! I have no further comments.

@registerrier registerrier merged commit e04b3f2 into gammapy:master Feb 18, 2019

3 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Scrutinizer Analysis: 6 new issues, 22 updated code elements – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Map analysis automation moved this from In progress to Done Feb 18, 2019

@adonath adonath changed the title Introduce EDispMap class Add EDispMap class Mar 26, 2019

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.