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 PHACountsSpectrumList class #2166

Merged
merged 1 commit into from May 27, 2019

Conversation

@registerrier
Copy link
Contributor

@registerrier registerrier commented May 27, 2019

The PR removes PHACountsSpectrumList, which is there to perform I/O on OGIP type II files.
Support for OGIP type II spectra is not provided yet in the SpectrumDataset framework for now. We need to see if this is needed or not.
In the meantime we remove this feature.

Note that the export to Type II files was assuming identical arf and rmf for all spectra, which is a strong limitation.

@registerrier registerrier self-assigned this May 27, 2019
@registerrier registerrier requested a review from cdeil May 27, 2019
@registerrier registerrier added this to To do in gammapy.makers via automation May 27, 2019
@registerrier registerrier added this to the 0.12 milestone May 27, 2019
@cdeil cdeil changed the title Removed PHACountsSpectrumList Remove PHACountsSpectrumList May 27, 2019
cdeil
cdeil approved these changes May 27, 2019
@cdeil
Copy link
Member

@cdeil cdeil commented May 27, 2019

For reference: this was added in #783

IMO it would be nice to have a way to write and read a list of spectrum extractions to a single FITS file. This could create 1000 HDUs and be 100 MB - that's fine, its still nice to have have a single-file solution. But also making ZIP files for a serialisation to a folder is a single-file solution.

Overall I think removing this is good, and then to only add back stuff with tests (in this case this functionality was untested) and if there's a clear need (which might or might not be there for this OGIP II format, once we have added nice support for OGIP I format).

@cdeil cdeil merged commit da2a0c5 into gammapy:master May 27, 2019
6 of 9 checks passed
gammapy.makers automation moved this from To do to Done May 27, 2019
@adonath adonath changed the title Remove PHACountsSpectrumList Remove PHACountsSpectrumList class 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