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

Improve table unit standardisation and flux points #1032

Merged
merged 5 commits into from May 18, 2017

Conversation

Projects
None yet
1 participant
@cdeil
Member

cdeil commented May 18, 2017

I wanted to do a pull request that fixes FluxPoints.plot to work for the case where asymmetric errors are given (they currently don't show up). But then I got sidetracked when I wanted to replace the unit standardisation code in FluxPoints.__init__ with the version in gammapy.utils, and ran into weird behaviour for QTable objects which get generated in the flux points tests.

Astropy issue concerning the QTable issues is here: astropy/astropy#6098

This pull request:

  • Changes the table column unit standardisation so that it can work for QTable
  • Call the new function from FluxPoints.__init__
  • Change from QTable to Table in two places in Gammapy where it's not really needed. (will leave a comment in #980).
  • Misc cleanup of FluxPoint class. Remove peek method because it was the same as plot. I'm -1 on adding a duplicate method just to have peek everywhere, users will be confused when to call plot or peek.

The FluxPoints.plot method is unchanged here, it just appears in the diff because I moved it to the end of the class code (like where the plotting code is for most classes in Gammapy). I'll make a follow-up PR tomorrow to actually refactor FluxPoints.plot (it's currently super long and hard to read and test) and fix the plotting of spectra with asymmetric errors (like we have cases in gammacat-status).

@cdeil cdeil added the cleanup label May 18, 2017

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

@cdeil cdeil self-assigned this May 18, 2017

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil May 18, 2017

Member

The fails in the Sphinx build are unrelated, probably a regression in Sphinx 1.6.1 I reported here:
sphinx-doc/sphinx#3766
Merging this now.

Member

cdeil commented May 18, 2017

The fails in the Sphinx build are unrelated, probably a regression in Sphinx 1.6.1 I reported here:
sphinx-doc/sphinx#3766
Merging this now.

@cdeil cdeil merged commit 5004407 into gammapy:master May 18, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@cdeil cdeil referenced this pull request May 19, 2017

Merged

Some cleanup of FluxPoints code and tests #1035

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