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 SkyCube exposure computation #784

Merged
merged 2 commits into from Nov 22, 2016

Conversation

Projects
None yet
2 participants
@JouvinLea
Contributor

JouvinLea commented Nov 20, 2016

@adonath
This PR is just to change how you compute the exposure cube for offset > offset_max. As we discussed it's now 0.

@JouvinLea

This comment has been minimized.

Show comment
Hide comment
@JouvinLea

JouvinLea Nov 21, 2016

Contributor

@adonath
I don't understand the issue on Travis?

Contributor

JouvinLea commented Nov 21, 2016

@adonath
I don't understand the issue on Travis?

@adonath adonath added the bug label Nov 21, 2016

@adonath adonath modified the milestones: 0.5, 0.6 Nov 21, 2016

@adonath adonath self-assigned this Nov 21, 2016

@adonath

This comment has been minimized.

Show comment
Hide comment
@adonath

adonath Nov 21, 2016

Member

@JouvinLea Thanks! The fail on Travis-CI is not related to your changes.

Can you add an additional assert in gammapy/cube/tests/test_exposure.py, that checks if the exposure is really zero for offset > offset_max? Then it's ready to merge...

Member

adonath commented Nov 21, 2016

@JouvinLea Thanks! The fail on Travis-CI is not related to your changes.

Can you add an additional assert in gammapy/cube/tests/test_exposure.py, that checks if the exposure is really zero for offset > offset_max? Then it's ready to merge...

Lea Jouvin
@JouvinLea

This comment has been minimized.

Show comment
Hide comment
@JouvinLea

JouvinLea Nov 21, 2016

Contributor

I added the test for offset > offset max and it passed can we merge now?

Contributor

JouvinLea commented Nov 21, 2016

I added the test for offset > offset max and it passed can we merge now?

@cdeil cdeil changed the title from Improve SKyExposureCube to Improve exposure SkyCube computation Nov 22, 2016

@JouvinLea

This comment has been minimized.

Show comment
Hide comment
@JouvinLea

JouvinLea Nov 22, 2016

Contributor

@adonath
Is it possible to merge? It's just I need that for my 3D analysis.... and I don't want to wait too long since then the rebase will be difficult...

Contributor

JouvinLea commented Nov 22, 2016

@adonath
Is it possible to merge? It's just I need that for my 3D analysis.... and I don't want to wait too long since then the rebase will be difficult...

@adonath adonath merged commit b334c2e into gammapy:master Nov 22, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@adonath

This comment has been minimized.

Show comment
Hide comment
@adonath

adonath Nov 22, 2016

Member

@JouvinLea Merged, Thanks!

Member

adonath commented Nov 22, 2016

@JouvinLea Merged, Thanks!

@cdeil cdeil changed the title from Improve exposure SkyCube computation to Improve SkyCube exposure computation Dec 12, 2016

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