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

Simplify `EdispMap.stack()`and `PsfMap.stack()` #2347

Merged

Conversation

@luca-giunti
Copy link
Contributor

commented Sep 7, 2019

Simplify EDispMap.stack() and PSFMap.stack() to not reproject the exposure map, but instead reproject the exposure map in make_edisp_map and make_psf_map.

@luca-giunti luca-giunti requested a review from registerrier Sep 7, 2019
@adonath adonath self-requested a review Sep 9, 2019
@adonath

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

Thanks a lot @luca-giunti for the quick PR. When I started the review I realised I was imprecise with the description of the changes I proposed. Sorry for that. Could you modify the implementation as follows:

  • Use an "in-place" stacking i.e. the other map is stacked into self, so that only self.psf_map and self.exposure are modified.
  • Allow to stack cutouts into a large map by using .fill_by_coord(). So it's not required, that self and other have exactly the same geom, but are just "aligned". So other can be a cutout of self, actually also the other way around, but there's not really a use case for this. We can maybe add a check for this later....

This is basically how the stacking part of the current MapMaker works, we just split it out into the .stack() methods.

@luca-giunti luca-giunti force-pushed the luca-giunti:Simplify-stacking-in-EdispMap-and-PSFMap branch from 02be13c to 2a94b93 Sep 11, 2019
@adonath adonath self-assigned this Sep 11, 2019
@adonath adonath added the cleanup label Sep 11, 2019
@adonath adonath added this to the 0.14 milestone Sep 11, 2019
Copy link
Member

left a comment

Thanks @luca-giunti! I have no further comments...

@adonath adonath merged commit b10daac into gammapy:master Sep 16, 2019
9 checks passed
9 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Scrutinizer Analysis: 2 new issues, 2 updated code elements – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
gammapy.gammapy Build #20190911.6 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
@adonath adonath changed the title Simplify stacking in edisp map and psf map Simplify `EdispMap.stack()`and `PsfMap.stack()` Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.