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 SpectrumFit class #2123

Merged
merged 15 commits into from Apr 23, 2019
Merged

Remove SpectrumFit class #2123

merged 15 commits into from Apr 23, 2019

Conversation

@adonath
Copy link
Member

@adonath adonath commented Apr 18, 2019

This PR removes the SpectrumFit and SpectrumFitResult class from Gammapy. It contains
the following changes:

  • Remove SpectrumFit from the Gammapy code base, tutorials and docs
  • Remove SpectrumFitResult object. The I/O functionality was moved to SpectrumAnalysisIACT.write(), which was the only placed it was used.
  • For the transition phase I implemented a SpectrumObservationList.to_spectrum_datasets() method, which is used in a few places.
  • Remove test duplications in spectrum/test/test_fit.py and spectrum/test/test_dataset.py
@adonath adonath self-assigned this Apr 18, 2019
@adonath adonath added this to the 0.12 milestone Apr 18, 2019
@adonath adonath added this to To do in gammapy.makers via automation Apr 18, 2019
--------- --------- --------- --------------- --------- ---
index 2.761e+00 1.094e-01 nan nan
amplitude 5.118e-11 4.849e-12 1 / (cm2 s TeV) nan nan
reference 1.000e+00 0.000e+00 TeV 0.000e+00 nan
Copy link
Member Author

@adonath adonath Apr 18, 2019

Choose a reason for hiding this comment

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

Note, that the numbers here changed for the sherpa example (see below) equivalently. So the different numbers are likely to a change in the input data and not because of the SpectrumFit -> Fit transition.

Copy link
Contributor

@registerrier registerrier Apr 23, 2019

Choose a reason for hiding this comment

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

OK

@adonath adonath requested a review from registerrier Apr 18, 2019
backend : minuit
method : minuit
success : True
nfev : 99
total stat : 63.00
nfev : 115
Copy link
Contributor

@registerrier registerrier Apr 23, 2019

Choose a reason for hiding this comment

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

Any idea why this changed?

Copy link
Member Author

@adonath adonath Apr 23, 2019

Choose a reason for hiding this comment

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

I was also surprised by this change, because in the tests not a single number changed. Interestingly the sherpa example in the docs changed as well and also gave different, but consistent numbers with the other example. So I think we probably forgot to update this docs example to the latest version of the hess-dl3 dataset. The change is definitely not related to the SpectrumFit -> Fit transition.

--------- --------- --------- --------------- --------- ---
index 2.761e+00 1.094e-01 nan nan
amplitude 5.118e-11 4.849e-12 1 / (cm2 s TeV) nan nan
reference 1.000e+00 0.000e+00 TeV 0.000e+00 nan
Copy link
Contributor

@registerrier registerrier Apr 23, 2019

Choose a reason for hiding this comment

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

OK

if not isinstance(datasets, Datasets):
datasets = Datasets(datasets)

datasets = datasets.copy()

if not datasets.is_all_same_type and datasets.is_all_same_shape:
Copy link
Contributor

@registerrier registerrier Apr 23, 2019

Choose a reason for hiding this comment

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

This could be checked before the copy?

Copy link
Member Author

@adonath adonath Apr 23, 2019

Choose a reason for hiding this comment

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

Done

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

def to_spectrum_dataset(self):
def to_spectrum_dataset(self, model=None):
Copy link
Contributor

@registerrier registerrier Apr 23, 2019

Choose a reason for hiding this comment

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

What is the use case here?

Copy link
Member Author

@adonath adonath Apr 23, 2019

Choose a reason for hiding this comment

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

As I've written below it's just meant to be a preliminary solution to help with the transition from SpectrumObservation to SpectrumDatasetOnOff. I just thought it was better to have this transition layer in one single place. But thinking about it again it's probably better to set the model afterwards, because that's what we have to do for the SpectrumDatasetOnOff anyway...I guess this is what you meant?

Copy link
Member Author

@adonath adonath Apr 23, 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 again and the problem was that SpectrumDataset does not allow model=None on init. But it's probably the much better idea to change it there...

)
self.fit.run(optimize_opts=optimize_opts)
modelname = self.fit.result[0].model.__class__.__name__
datasets_fit = self.extraction.spectrum_observations.to_spectrum_datasets(**self.config["fit"])
Copy link
Contributor

@registerrier registerrier Apr 23, 2019

Choose a reason for hiding this comment

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

I suppose we can keep like this for the moment, but this will likely have to significantly change if SpectrumObservation and SpectrumObservationList are removed.

I suppose that a loop on all Dataset to set the model is unavoidable step for the user.

Copy link
Member Author

@adonath adonath Apr 23, 2019

Choose a reason for hiding this comment

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

Yes, this is only meant to be a preliminary solution to help with the transition from SpectrumObservation to SpectrumDataset. I've opened a reminder issue #2113 to discuss the API question how to set models on Datasets. Maybe keeping the loop over dataset objects is just fine...

@@ -96,7 +96,7 @@
" reference=1 * u.TeV,\n",
")\n",
"\n",
"flux_point_binning = EnergyBounds.equal_log_spacing(0.7, 30, 5, u.TeV)"
"flux_point_binning = EnergyBounds.equal_log_spacing(0.77, 30, 5, u.TeV)"
Copy link
Contributor

@registerrier registerrier Apr 23, 2019

Choose a reason for hiding this comment

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

why the change?

Copy link
Member Author

@adonath adonath Apr 23, 2019

Choose a reason for hiding this comment

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

This change was not intended, I'll revert it.

@@ -601,9 +637,17 @@
"source": [
"## Exercises\n",
"\n",
"- Turn of the containment correction of the 1D spectral analysis and compare the result to the reference Crab spectrum\n",
Copy link
Contributor

@registerrier registerrier Apr 23, 2019

Choose a reason for hiding this comment

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

Turn off

"cell_type": "markdown",
"metadata": {},
"source": [
"The difference we see compared to the reference H.E.S.S. 2006 model is because of the appilcation of the containment correction. "
Copy link
Contributor

@registerrier registerrier Apr 23, 2019

Choose a reason for hiding this comment

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

application

Copy link
Contributor

@registerrier registerrier Apr 23, 2019

Choose a reason for hiding this comment

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

The difference should not come from containment correction. It is also applied in the 2006 Crab paper.

Copy link
Member Author

@adonath adonath Apr 23, 2019

Choose a reason for hiding this comment

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

As a quick check I found that the un-corrected spectrum is much closer to the 2006 crab reference, but as I didn't check in detail it's probably better to remove the statement again.

@@ -96,7 +96,7 @@
" reference=1 * u.TeV,\n",
Copy link
Contributor

@registerrier registerrier Apr 23, 2019

Choose a reason for hiding this comment

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

There is SkyCoord.from_name("crab") that breaks Travis CI. Maybe this should be removed?

Copy link
Member Author

@adonath adonath Apr 23, 2019

Choose a reason for hiding this comment

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

Done

@@ -152,47 +123,6 @@ def setup(self):
)


def test_basic_results(self):
Copy link
Contributor

@registerrier registerrier Apr 23, 2019

Choose a reason for hiding this comment

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

All those tests are replaced by tests in test_dataset.py, right?

Copy link
Member Author

@adonath adonath Apr 23, 2019

Choose a reason for hiding this comment

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

Yes, I removed the duplication of tests between test_fit.py and test_dataset.py. The distinction is a bit arbitrary, but for now it shouldn't matter. I guess it's OK to clean up the tests in future.

@adonath
Copy link
Member Author

@adonath adonath commented Apr 23, 2019

The remaining Travis-CI fail is unrelated. I'll merge this PR now.

@adonath adonath merged commit 06d07b9 into gammapy:master Apr 23, 2019
7 of 9 checks passed
gammapy.makers automation moved this from To do to Done Apr 23, 2019
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

2 participants