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

Fix reflected region pixel origin issue #738

Merged
merged 2 commits into from Oct 12, 2016

Conversation

Projects
None yet
3 participants
@JouvinLea
Contributor

JouvinLea commented Oct 11, 2016

Fix the origin that was different for the on region and the pointing position in the method reflected regions

@cdeil cdeil added the bug label Oct 11, 2016

@cdeil cdeil added this to the 0.5 milestone Oct 11, 2016

@cdeil cdeil self-assigned this Oct 11, 2016

@cdeil cdeil changed the title from Fix reflected region issue with origin to Fix reflected region pixel origin issue Oct 11, 2016

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Oct 11, 2016

Member

@JouvinLea - Thanks.

I think this is a correct fix, because we should always use origin=0. And @JouvinLea mentioned on Slack that she found this issue via a test case where the result was incorrect (as verified via DS9).

I'll merge if travis-ci passes.

@joleroi - If we're mistaken here for some reason, please leave a comment explaining why and we'll fix some more...

Member

cdeil commented Oct 11, 2016

@JouvinLea - Thanks.

I think this is a correct fix, because we should always use origin=0. And @JouvinLea mentioned on Slack that she found this issue via a test case where the result was incorrect (as verified via DS9).

I'll merge if travis-ci passes.

@joleroi - If we're mistaken here for some reason, please leave a comment explaining why and we'll fix some more...

Lea Jouvin
@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Oct 12, 2016

Contributor

@cdeil @JouvinLea I don't remember why I put origin = 1 here. 👍 for this fix

Contributor

joleroi commented Oct 12, 2016

@cdeil @JouvinLea I don't remember why I put origin = 1 here. 👍 for this fix

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Oct 12, 2016

Member

The fail is unrelated:
https://travis-ci.org/gammapy/gammapy/jobs/166982006

Merging this now.

@JouvinLea - Thanks!

Member

cdeil commented Oct 12, 2016

The fail is unrelated:
https://travis-ci.org/gammapy/gammapy/jobs/166982006

Merging this now.

@JouvinLea - Thanks!

@cdeil cdeil merged commit 3451055 into gammapy:master Oct 12, 2016

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment