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 spectrum butterfly class #1025

Merged
merged 1 commit into from May 16, 2017

Conversation

Projects
None yet
2 participants
@cdeil
Member

cdeil commented May 15, 2017

This PR removes the spectrum butterfly class.

It was implemented as a QTable sub-class with one method plot added in, which was duplicating the code in SpectralModel.plot_error. (I'll fix an issue in SpectralModel.plot_error in a separate PR, that's how I stumbled on this code).

We can always re-introduce SpectrumButterfly later (with a table member instead of as a Table sub-class) if we find it useful, but for now I'd suggest we remove it (to avoid the code duplication with SpectralModel.plot_error

@adonath @joleroi - Thoughts?

@cdeil cdeil added the cleanup label May 15, 2017

@cdeil cdeil added this to the 0.7 milestone May 15, 2017

@cdeil cdeil requested a review from joleroi May 15, 2017

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil May 15, 2017

Member

I made the fix for SpectralModel.plot_error directly in master: a97eb54
It fixes the plotting issue for all butterflies in gamma-cat: cdeil/gamma-cat-status@dfe3757
I think this could be considered an MPL bug, so I also filed an issue in the MPL tracker: matplotlib/matplotlib#8623

Member

cdeil commented May 15, 2017

I made the fix for SpectralModel.plot_error directly in master: a97eb54
It fixes the plotting issue for all butterflies in gamma-cat: cdeil/gamma-cat-status@dfe3757
I think this could be considered an MPL bug, so I also filed an issue in the MPL tracker: matplotlib/matplotlib#8623

@adonath

This comment has been minimized.

Show comment
Hide comment
@adonath

adonath May 16, 2017

Member

+1 To remove the current SpectrumButterfly class. It doesn't hold up to our implementation standards and doesn't offer any useful extra features. I'm not even sure if we need a SpectrumButterfly class at all...maybe this can be just handled by an improved TableModel or SpectralTemplate class.

Currently the spectrum_simulation_cta.ipynb notebook is not working...because of a call to SpectrumButterfly.plot()

Member

adonath commented May 16, 2017

+1 To remove the current SpectrumButterfly class. It doesn't hold up to our implementation standards and doesn't offer any useful extra features. I'm not even sure if we need a SpectrumButterfly class at all...maybe this can be just handled by an improved TableModel or SpectralTemplate class.

Currently the spectrum_simulation_cta.ipynb notebook is not working...because of a call to SpectrumButterfly.plot()

@cdeil cdeil assigned cdeil and unassigned adonath May 16, 2017

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil May 16, 2017

Member

I'm merging this now, and will fix spectrum_simulation_cta.ipynb next.

Member

cdeil commented May 16, 2017

I'm merging this now, and will fix spectrum_simulation_cta.ipynb next.

@cdeil cdeil merged commit 0e5a9db into gammapy:master May 16, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil May 16, 2017

Member

Notebook adapted in gammapy/gammapy-extra@1fc5bf6

Member

cdeil commented May 16, 2017

Notebook adapted in gammapy/gammapy-extra@1fc5bf6

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