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 SpectrumObservation and SpectrumObservationList classes #2153

Merged
merged 5 commits into from May 23, 2019

Conversation

@registerrier
Copy link
Contributor

@registerrier registerrier commented May 22, 2019

This PR removes the SpectrumObservation and SpectrumObservationList which are no longer needed after the modifications from PR #2139.

The SpectrumStats class definition has been moved to data/obs_stats.py where ObservationStats is implemented.
Tests have been added.

@registerrier registerrier requested a review from adonath May 22, 2019
@registerrier registerrier self-assigned this May 22, 2019
@registerrier registerrier added this to To do in gammapy.makers via automation May 22, 2019
@registerrier registerrier added this to the 0.12 milestone May 22, 2019
Copy link
Member

@adonath adonath left a comment

Thanks @registerrier! This looks good to me. My only question is whether it's worth to keep the .to_sherpa() method?

# TODO: optimize layout
plt.subplots_adjust(wspace=0.3)

def to_sherpa(self):
Copy link
Member

@adonath adonath May 23, 2019

Choose a reason for hiding this comment

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

Shall we keep this as SpectrumDatasetOnOff.to_sherpa()? Or just replace it with a docs example maybe?

Copy link
Contributor Author

@registerrier registerrier May 23, 2019

Choose a reason for hiding this comment

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

The conversion to sherpa is still possible through the export to OGIP files. This is the approach suggested in the notebook spectrum_fitting_with_sherpa.ipynb.

livetime=self.livetime,
)

def stats_table(self):
Copy link
Member

@adonath adonath May 23, 2019

Choose a reason for hiding this comment

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

Just a side comment: I think this is basically the way we should do these stats info methods on the datasets. I.e. return either astropy.Table or dict objects. I'll keep it in mind...

@registerrier
Copy link
Contributor Author

@registerrier registerrier commented May 23, 2019

The to_sherpa() method is not really needed anymore, and we can still export to OGIP to later load and perform the fit with sherpa. So we do not export it.

@registerrier registerrier merged commit d832d19 into gammapy:master May 23, 2019
9 checks passed
gammapy.makers automation moved this from To do to Done May 23, 2019
@adonath adonath changed the title Remove spectrum observation Remove SpectrumObservation and SpectrumObservationList classes May 27, 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