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

Refactor SpectrumExtraction to use SpectrumDatasetOnOff #2139

Merged
merged 32 commits into from May 22, 2019

Conversation

2 participants
@registerrier
Copy link
Contributor

commented May 14, 2019

This PR is a further step on the transition of the spectrum package to the Dataset approach. The spectral extraction now produces a list of SpectrumDatasetOnOff instead of a SpectrumObservationList.

  • The SpectrumObservation and SpectrumObservationList become redundant with the SpectrumDatasetOnOff. Some functionalities have been transferred but some cannot.
    In particular,
    • there are two levels of masks in the SpectrumDatasetOnOff. One is defined by the data quality thresholds, the other is the fitting range. It is important to keep track of the two distinct notions. The first one is stored in the PHACountsSpectrum, and can be accessed and modified through its .lo_thresholds and .hi_thresholds. The fit_range is defined with a mask setter property on the SpectrumDatasetOnOff. A mask property gives the interaction of the two masks.
    • The existing SpectrumObservationListStacker is modified into SpectrumDatasetOnOffStacker which takes a list of datasets and produces a SpectrumDatasetOnOff as output. Its behavior might be further adapted since it could simply work as a function.

The PR is still work in progress. In particular, the SpectrumObservation and SpectrumObservationList have to be removed.

@registerrier registerrier self-assigned this May 14, 2019

@registerrier registerrier requested a review from adonath May 14, 2019

@registerrier registerrier added this to To do in Spectrum analysis (1D) via automation May 14, 2019

@registerrier registerrier added this to the 0.12 milestone May 14, 2019

@registerrier registerrier force-pushed the registerrier:spectrum_extract_dataset branch from bd56cc6 to f34b97b May 15, 2019

@registerrier registerrier force-pushed the registerrier:spectrum_extract_dataset branch from 3cab9c2 to 6f75985 May 21, 2019

@adonath
Copy link
Member

left a comment

Thanks a lot @registerrier!

My main comment here would be API of the handling of the energy range(s), which we should try to unify with MapDataset and Datasets, but this can be done in a follow-up PR. I've left one minor inline comment, otherwise I would suggest to merge this PR quickly, so that we can continue with the clean-up.

if self.counts_off is not None:
self.counts_off.hi_threshold = threshold

def reset_thresholds(self):

This comment has been minimized.

Copy link
@adonath

adonath May 22, 2019

Member

Is this method really needed? Maybe we can just allow dataset.hi_threshold = None and dataset.lo_threshold? This would be in line with what we have in the set_fit_energy_range() method and geom.energy_mask().

@adonath adonath merged commit df84d2d into gammapy:master May 22, 2019

4 of 9 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
gammapy.gammapy Build #20190522.2 had test failures
Details
gammapy.gammapy (Test Python35) Test Python35 failed
Details
gammapy.gammapy (Test Windows35) Test Windows35 failed
Details
gammapy.gammapy (Test Windows37) Test Windows37 failed
Details
Scrutinizer Analysis: 5 new issues, 39 updated code elements – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
gammapy.gammapy (DevDocs) DevDocs succeeded
Details
gammapy.gammapy (Lint) Lint succeeded
Details

Spectrum analysis (1D) automation moved this from To do to Done May 22, 2019

@adonath

This comment has been minimized.

Copy link
Member

commented May 22, 2019

Merged, the travis build showed green light. Thanks, @registerrier!

@adonath adonath changed the title Spectrum extract dataset Refactor SpectrumExtraction to use SpectrumDatasetOnOff May 27, 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.