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

Implement `MapDataset.to_spectrum_dataset()` method #2292

Merged
merged 15 commits into from Sep 26, 2019

Conversation

@registerrier
Copy link
Contributor

commented Jul 17, 2019

In the discussion on data reduction, we have proposed to perform a succession of distinct tasks on a per observation basis.

For spectral data reduction the approach is to perform :

  • SpectrumDataset creation:
    • creation of the dataset, fill it with counts, aeff, edisp and possibly with the irf background template
  • OFF background extraction is needed and creation of a SpectrumDatasetOnOff

This PR proposes a prototype for the first step described here.
It reuses a large fraction of the code of ~gammapy.spectrum.SpectrumExtraction but removes the loop over observations and the OFF events binning.

To compute the background per energy bin in the spatial region, a spatial integration is necessary. it is performed by creating the background map in a small cutout of the reference tangent map and integrating over pixels inside the region.

Additionally the capability to perform averaging of the effective area over the region is introduced to better handle the case of extended regions.

@registerrier registerrier requested a review from adonath Jul 17, 2019
@registerrier registerrier added this to To do in Spectrum analysis (1D) via automation Jul 17, 2019
@registerrier registerrier added this to the 0.14 milestone Jul 17, 2019
@adonath

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

Thanks @registerrier! I have thought about a bit what to do about this PR. I agree 100% with the functionality. It's an important use-case, where I know some users are already waiting for it. So it would be nice to get this in even for v0.14.

What I'm not happy yet with is the API and implementation. I think introducing a separate SpectrumDatasetMakerObs creates complexity in passing down the configuration e.g. to the MapMaker, which is used internally. The other problem I currently see with this implementation is that the counts spectrum is computed from the event list, while the background is computed from a binned map. Another problem I see is that the background norm is currently not adjusted. I think it would be cleaner (concerning configuration and implementation) to introduce either a class or a helper method which creates the SpectrumDataset from an existing MapDataset. E.g.:

dataset = MapDataset(...)

# e.g. fit background norm and tilt
fit = Fit(dataset)
fit.run()

maker = SpectrumDatasetMaker(region, spatial_averaging, containment_correction)
spectrum_dataset = maker.run(dataset)

My hope is that we'll find some time to finish the data reduction PIG and define an API soon, so that we arrive at a flexible maker and pipeline system, where this SpectrumDatasetMaker would be one building block e.g. like so: MapDatasetMaker -> BackgroundFitter -> SpectrumDatasetMaker. What do you think?

@registerrier registerrier force-pushed the registerrier:prototype_spectrumdatasetmakerobs branch 2 times, most recently from a768a1b to 9a31a65 Sep 23, 2019
@@ -683,6 +688,66 @@ def to_dict(self, filename=""):
data["filename"] = filename
return data

def to_spectrum_dataset(self, on_region):

This comment has been minimized.

Copy link
@adonath

adonath Sep 25, 2019

Member

Maybe also add the containment_correction argument here?

This comment has been minimized.

Copy link
@registerrier

registerrier Sep 25, 2019

Author Contributor

I have added it here. It requires that the MapDataset contains a PSFMap as psf attribute.

@registerrier registerrier force-pushed the registerrier:prototype_spectrumdatasetmakerobs branch from 15e3736 to 22fd797 Sep 25, 2019
@registerrier

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2019

This PR is heavily modified since the initial commit. It now only consists in an addition to MapDataset to add a .to_spectrum_dataset method to extract a spectrum in any region of a MapDataset.
This can then be used to compute average responses over extended sources (so far only average aeff and total background).

@adonath

This comment has been minimized.

Copy link
Member

commented Sep 26, 2019

Thanks a lot @registerrier! In bdfc0cd I did a little bit of clean-up, I'll go ahead and merge now.

@adonath adonath merged commit caf52d1 into gammapy:master Sep 26, 2019
6 of 9 checks passed
6 of 9 checks passed
Codacy/PR Quality Review Hang in there, Codacy is reviewing your Pull request.
Details
Scrutinizer Running
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
gammapy.gammapy Build #20190926.18 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
Spectrum analysis (1D) automation moved this from To do to Done Sep 26, 2019
@adonath adonath changed the title First prototype for a SpectrumDatasetMakerObs Implement `MapDataset.to_spectrum_dataset()` method Sep 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
2 participants
You can’t perform that action at this time.