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 EDispMap and PSFMap stacking #2085

Merged
merged 10 commits into from Mar 28, 2019

Conversation

2 participants
@registerrier
Copy link
Contributor

registerrier commented Mar 25, 2019

This PR changes the behavior of PSFMap.stack(other) and EDispMap.stack(other).

The two geometries have to be identical in the current version. With this change, the other IRF Map is reprojected on the target geometry. This is necessary to allow IRFMapMaker to use cutouts in the similar manner as MapMaker. See discussion in PR #2056 .

@registerrier registerrier self-assigned this Mar 25, 2019

@registerrier registerrier requested a review from adonath Mar 25, 2019

@registerrier registerrier added this to To do in Map analysis via automation Mar 25, 2019

@registerrier registerrier added this to the 0.11 milestone Mar 25, 2019

Map analysis automation moved this from To do to In progress Mar 27, 2019

@adonath
Copy link
Member

adonath left a comment

Thanks @registerrier! I've left one inline comment...

@@ -315,17 +315,17 @@ def containment_radius_map(self, energy, fraction=0.68):

return m

def stack(self, other):
def stack(self, other, in_place=True):

This comment has been minimized.

Copy link
@adonath

adonath Mar 27, 2019

Member

I think the in_place option is not used at the moment. Another suggestion: maybe change to a copy option instead? So Always return the stacked map, but in one case a copy is made, whereas in the other self is returned. I think this corresponds to the API we have in other places as well e.g. Map.cutout().

This comment has been minimized.

Copy link
@registerrier

registerrier Mar 27, 2019

Author Contributor

Done.

@registerrier

This comment has been minimized.

Copy link
Contributor Author

registerrier commented Mar 27, 2019

I have changed to the behavior you suggested for the copy parameter for the stack method.

I have removed the properties quantity data and geom from the EDispMap and the PSFMap to avoid the multiple access to PSFMap.psf_map.quantity and PSFMap.quantity. Only the former is possible now.

I also updated the docstring example which did not include the calculation of the exposure map.

@adonath

This comment has been minimized.

Copy link
Member

adonath commented Mar 28, 2019

Thanks @registerrier, looks good to me now!

@adonath adonath merged commit 924153f into gammapy:master Mar 28, 2019

3 checks passed

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

Map analysis automation moved this from In progress to Done Mar 28, 2019

@adonath adonath changed the title Modify irfmap stacking Improve EDispMap and PSFMap stacking Mar 28, 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.