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 FluxPointEstimator to use Datasets #2112

Merged
merged 7 commits into from Apr 12, 2019

Conversation

@adonath
Copy link
Member

@adonath adonath commented Apr 9, 2019

This PR refactors the FluxPointEstimator to use Datasets instead of a SpectrumObservationList. It includes the following changes:

  • Refactor FluxPointEstimator to take a list of SpectrumDatasetOnOff on input.
  • Change the SpectrumEnergyGroupMaker to just take e_reco instead of a SpectrumObservation object (the only info used for now was obs.e_reco)
  • Move SpectrumDatasets._from_spectrum_observation() to SpectrumObservation.to_spectrum_dataset() as a compatibility helper method for the transition phase to SpectrumDataset and SpectrumDatasetOnOff.

TODO:

  • Adapt tutorials to the changes
@@ -57,11 +58,13 @@ def __init__(self, datasets, mask=None):

self.mask = mask

@lazyproperty
Copy link
Contributor

@registerrier registerrier Apr 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it safe to make Datasets.parameters a lazyproperty?

Imagine you want to test another spectral model by doing something like this:

for ds in datasets:
      ds.model = new_model

Will the datasets.parameters be update accordingly?

Copy link
Member Author

@adonath adonath Apr 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. This is a problem that arises when we set / modify the models on datasets after init. I'll check again what makes sense here and then change it consistently on all dataset classes.

Copy link
Member Author

@adonath adonath Apr 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've looked into this a bit and currently there is the problem, that we attach the covariance matrix to the Parameters object. If I declare Datasets.parameters as a normal property I can't set the the covariance anymore, because anytime the .parameters attribute is accessed a new merged Parameters object is created from the sub-datasets and the covariance info is lost (this is the error we see on Travis-CI).

For the parallel evaluation of datasets we have to re-think the parameter handling and distribution of parameters in the Datasets class anyway. So for now I'll probably leave it as is with a lazyproperty, but we have to come up with a better solution and documentation, how to set a global model for Datasets or how to handle the covariance.

@@ -518,6 +518,24 @@ def copy(self):
"""A deep copy."""
return copy.deepcopy(self)

def to_spectrum_dataset(self):
Copy link
Contributor

@registerrier registerrier Apr 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, it is probably much better to have this function on SpectrumObservation rather than on the Dataset itself.

@adonath adonath self-assigned this Apr 9, 2019
@adonath adonath added the cleanup label Apr 9, 2019
@adonath adonath added this to To do in gammapy.makers via automation Apr 9, 2019
@adonath adonath added this to the 0.12 milestone Apr 9, 2019
def __init__(self, obs):
self.obs = obs
def __init__(self, e_reco):
self.e_reco = EnergyBounds(e_reco)
self.groups = None

def groups_from_obs(self):
Copy link
Contributor

@kbruegge kbruegge Apr 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this method can be removed I think

Copy link
Contributor

@kbruegge kbruegge Apr 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or renamed? not sure what it does.

@kbruegge
Copy link
Contributor

@kbruegge kbruegge commented Apr 9, 2019

This might not be a point of this PR but I think the EnergyGroupMaker class is not really useful. It could just be the constructor of EnergyGroup no?

@adonath
Copy link
Member Author

@adonath adonath commented Apr 9, 2019

@mackaiver I completely agree: in the current state the SpectrumEnergyGroupMaker is not really useful. We could also just pass directly an array of energies to the FluxPointEstimator. I think the initial idea was to have additional methods (e.g. an adaptive method or equal significance per flux point) on the SpectrumEnergyGroupMaker object, but this never happened. I'd be open to completely remove it for now...

@kbruegge
Copy link
Contributor

@kbruegge kbruegge commented Apr 9, 2019

I'd be open to completely remove it for now...

Very much in favour 👍

@adonath adonath force-pushed the fpe_dataset_support branch from e2dee5a to b48fcec Apr 11, 2019
@adonath adonath force-pushed the fpe_dataset_support branch from b48fcec to aea5aa6 Apr 11, 2019
@adonath
Copy link
Member Author

@adonath adonath commented Apr 12, 2019

@mackaiver I'll merge this PR now. The clean up of SpectrumEnergyGroupEstimator will be done in a separate PR.

@adonath adonath merged commit d413724 into gammapy:master Apr 12, 2019
8 of 9 checks passed
gammapy.makers automation moved this from To do to Done Apr 12, 2019
@adonath
Copy link
Member Author

@adonath adonath commented Apr 16, 2019

@mackaiver I removed the SpectrumEnergyGroupMaker in #2114.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants