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

Improve edisp.apply to support different true energy axes #864

Merged
merged 1 commit into from Feb 1, 2017

Conversation

@joleroi
Copy link
Contributor

@joleroi joleroi commented Feb 1, 2017

EnergyDispersion.apply had a bug insofar as it assumed the input data has the same energy axis as the true energy axis of the matrx. This PR fixes this.

Reported by @robertazanin

@joleroi joleroi added the feature label Feb 1, 2017
@joleroi joleroi self-assigned this Feb 1, 2017
@joleroi
Copy link
Contributor Author

@joleroi joleroi commented Feb 1, 2017

I am merging this because @robertazanin needs it 😄

@joleroi joleroi merged commit f7c48b8 into gammapy:master Feb 1, 2017
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@joleroi joleroi deleted the joleroi:update_apply branch Feb 1, 2017
@cdeil cdeil added this to the 0.6 milestone Feb 1, 2017
@cdeil cdeil changed the title Allow e_true argument in edisp.apply Fix energy dispersion apply if true and reco energy axis is different Feb 1, 2017
@joleroi
Copy link
Contributor Author

@joleroi joleroi commented Feb 2, 2017

Ok, it was a bad idea. Master is currently broken, I will fix it in a follow up PR

@joleroi joleroi changed the title Fix energy dispersion apply if true and reco energy axis is different Allow e_true argument in edisp.apply Feb 2, 2017
@joleroi
Copy link
Contributor Author

@joleroi joleroi commented Feb 2, 2017

Fix energy dispersion apply if true and reco energy axis is different

is not what this PR does

@cdeil
Copy link
Member

@cdeil cdeil commented Feb 2, 2017

The basic idea behind the pull request titles at the moment is that I want to use them 1:1 in the changelog: http://docs.gammapy.org/en/latest/changelog.html#gammapy-0p6-release

So basically I want to have them high-level and start either with "Fix" or "Add" or "Improve" in most cases.

Clearly there are other options to organise and handle pull requests and changelog and we should discuss offline ... this was just to explain why I changed it (apparently incorrectly in this case).

@joleroi joleroi changed the title Allow e_true argument in edisp.apply Improve edisp.apply to support different true energy axes Feb 3, 2017
@joleroi
Copy link
Contributor Author

@joleroi joleroi commented Feb 3, 2017

So basically I want to have them high-level and start either with "Fix" or "Add" or "Improve" in most cases.

I see, I'll stick to this scheme in the future

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants