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

Remove MapMaker class #2444

Merged
merged 11 commits into from Oct 8, 2019
Merged

Remove MapMaker class #2444

merged 11 commits into from Oct 8, 2019

Conversation

@adonath
Copy link
Member

adonath commented Oct 7, 2019

This PR removes the MapMaker class and replaces it with the following pattern:

stacked = MapDataset.create(geom=geom)

for obs in observations:
    maker = MapMakerObs(obervation=obs, geom=geom)
    dataset = maker.run()
    stacked.stack(dataset)

As we now have the high-level analysis interface I think it's acceptable to write a few more lines of code in the 2nd level API, but gaining the possibility to fit e.g. background model norms before stacking. Or first doing the data reduction for a joint-analysis and only later stack the reduced data for a stacked analysis.

As the MapMakerObs (we should rename it soon) now actually produces a MapDataset this reduces a lot of code where create MapDataset object by hand in the tutorials.

@adonath adonath self-assigned this Oct 7, 2019
@adonath adonath added the cleanup label Oct 7, 2019
@adonath adonath added this to the 0.15 milestone Oct 7, 2019
@adonath adonath added this to To do in gammapy.maps via automation Oct 7, 2019
@cdeil

This comment has been minimized.

Copy link
Member

cdeil commented Oct 7, 2019

From the description, that does seem like a good idea to me, having both MapMaker and Analysis that do that kind of data reduction seems like unnecessary duplication.
I guess a similar change would come in 1D spectral analysis?
Is a PIG still planned for data reduction, or is this just the clear way how data reduction should be done, and no PIG and design discussion is needed any more?

@cdeil
cdeil approved these changes Oct 7, 2019
Copy link
Contributor

registerrier left a comment

Thanks @adonath. This looks good to me. I have no specific comment so far.

I can imagine that the MapMakerRing will go away too once we create a clear step for background extraction. This might require the addition of a MapDatasetOnOff though.

@adonath

This comment has been minimized.

Copy link
Member Author

adonath commented Oct 8, 2019

@cdeil I think these are a few first clean-up steps. We might still need a PIG to plan and describe a general Maker and MakerChain system for Gammapy (which also includes configuration).

@registerrier I agree once we have the MapDatasetOnOff, we can proceed with the clean-up of the MapMakerRing. Let's discuss the detail in the call later.

@adonath adonath merged commit c5bd028 into gammapy:master Oct 8, 2019
8 of 9 checks passed
8 of 9 checks passed
Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
Scrutinizer Analysis: 5 updated code elements – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
gammapy.gammapy Build #20191007.3 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
gammapy.maps automation moved this from To do to Done Oct 8, 2019
@adonath

This comment has been minimized.

Copy link
Member Author

adonath commented Oct 8, 2019

Thanks @cdeil and @registerrier for the quick approval.

@adonath adonath changed the title Remove `MapMaker` class Remove MapMaker class Nov 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
gammapy.maps
  
Done
3 participants
You can’t perform that action at this time.