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

Add function to compute exposure cubes #398

Merged
merged 7 commits into from Dec 9, 2015

Conversation

Projects
None yet
3 participants
@tibaldo
Contributor

tibaldo commented Dec 8, 2015

@joleroi please review, I implemented this as we discussed yesterday within the interpolation function. I will use this to implement 3D exposure cube, and we can merge once this is working

@joleroi joleroi self-assigned this Dec 8, 2015

@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Dec 8, 2015

Contributor

In order to make sure everything is working correctly I would be useful to add tests.
In gammapy/irf/test_effective_area.py there are already some test that test different input shapes. Can you add tests there to also test for 2D input arrays?

https://github.com/tibaldo/gammapy/blob/master/gammapy/irf/tests/test_effective_area.py#L115

Contributor

joleroi commented Dec 8, 2015

In order to make sure everything is working correctly I would be useful to add tests.
In gammapy/irf/test_effective_area.py there are already some test that test different input shapes. Can you add tests there to also test for 2D input arrays?

https://github.com/tibaldo/gammapy/blob/master/gammapy/irf/tests/test_effective_area.py#L115

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Dec 8, 2015

Member

My 2 cents on tests:

My experience is that high-level tests are more important than low-level unit tests, so if @tibaldo wants to prefers this now and then add a high-level test that computes an exposure cube and calls this with an array later, I'd say it's OK to merge this without extra test now.

Of course having lots of unit tests is great, but it also slows people down, writing the tests becomes more work than implementing features.

So bottom line: my suggestion for Gammapy would be to encourage / ask contributors to add high-level and unit tests, but it's also OK to say "no, I just want this feature to be merged" or "I'll add tests later".

Member

cdeil commented Dec 8, 2015

My 2 cents on tests:

My experience is that high-level tests are more important than low-level unit tests, so if @tibaldo wants to prefers this now and then add a high-level test that computes an exposure cube and calls this with an array later, I'd say it's OK to merge this without extra test now.

Of course having lots of unit tests is great, but it also slows people down, writing the tests becomes more work than implementing features.

So bottom line: my suggestion for Gammapy would be to encourage / ask contributors to add high-level and unit tests, but it's also OK to say "no, I just want this feature to be merged" or "I'll add tests later".

@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Dec 8, 2015

Contributor

@cdeil: In principle I agree, but not in this case. An error at this point might be difficult to spot later and is very easy to avoid now, with a 3 line test that was probably made in an IPython notebook or similar anyways. An error at this point might also have implication for several high-level use cases. It also comes with the benefit that another Interpolator that might be added in the future is automatically tested.

Of course I am not keeping anyone from merging their contributions if they want to, it was just a suggestion.

Contributor

joleroi commented Dec 8, 2015

@cdeil: In principle I agree, but not in this case. An error at this point might be difficult to spot later and is very easy to avoid now, with a 3 line test that was probably made in an IPython notebook or similar anyways. An error at this point might also have implication for several high-level use cases. It also comes with the benefit that another Interpolator that might be added in the future is automatically tested.

Of course I am not keeping anyone from merging their contributions if they want to, it was just a suggestion.

@tibaldo

This comment has been minimized.

Show comment
Hide comment
@tibaldo

tibaldo Dec 9, 2015

Contributor

Exposure cube calculation added. Also Includes a method in SpectralCube class to read a count cube file (that I used for testing) and test on effective area calculation with 2D array for offset input as suggested. @joleroi @cdeil please review and let me know if it is okay to merge

Contributor

tibaldo commented Dec 9, 2015

Exposure cube calculation added. Also Includes a method in SpectralCube class to read a count cube file (that I used for testing) and test on effective area calculation with 2D array for offset input as suggested. @joleroi @cdeil please review and let me know if it is okay to merge

Show outdated Hide outdated gammapy/irf/exposure.py Outdated
Show outdated Hide outdated gammapy/irf/exposure.py Outdated
Show outdated Hide outdated gammapy/irf/exposure.py Outdated
Show outdated Hide outdated gammapy/data/spectral_cube.py Outdated
@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Dec 9, 2015

Member

I left a few nitpicky comments.

From a quick look, the API and test and code looks good to me.

@joleroi - Do you want to do any review / testing here or should we just merge (once the small issues are adressed) and continue testing / using this from master?

Member

cdeil commented Dec 9, 2015

I left a few nitpicky comments.

From a quick look, the API and test and code looks good to me.

@joleroi - Do you want to do any review / testing here or should we just merge (once the small issues are adressed) and continue testing / using this from master?

@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Dec 9, 2015

Contributor

No more comments from my side

Contributor

joleroi commented Dec 9, 2015

No more comments from my side

tibaldo added a commit that referenced this pull request Dec 9, 2015

Merge pull request #398 from tibaldo/exposure-cube
Exposure cube - abritrary ndarrays accepted as input for offset and energy in aeff interp

@tibaldo tibaldo merged commit eeae028 into gammapy:master Dec 9, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tibaldo tibaldo deleted the tibaldo:exposure-cube branch Dec 9, 2015

@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Dec 9, 2015

Contributor

Don't forget to make an entry in https://github.com/gammapy/gammapy/blob/master/CHANGES.rst 😉

Contributor

joleroi commented Dec 9, 2015

Don't forget to make an entry in https://github.com/gammapy/gammapy/blob/master/CHANGES.rst 😉

@cdeil cdeil changed the title from Exposure cube - abritrary ndarrays accepted as input for offset and energy in aeff interp to Add function to compute exposure cubes Dec 12, 2015

@cdeil cdeil added this to the 0.4 milestone Dec 12, 2015

@cdeil cdeil added the feature label Dec 12, 2015

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