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 IRF interpolation and extrapolation #455

Merged
merged 2 commits into from Feb 17, 2016

Conversation

Projects
None yet
3 participants
@cdeil
Member

cdeil commented Feb 17, 2016

This PR cleans up interpolation / extrapolation in Gammapy:

  • Document developer guideline
  • Change aeff2d interpolation default
  • Add obs_exposure_cube stub (not implemented yet) as convenience wrapper around exposure_cube
  • Adapt exposure_cube computation and test to new interpolation / extrapolation guideline

@joleroi @adonath - Does this interpolation / extrapolation guideline for Gammapy make sense to you?

@joleroi - Are there other cases that need to be adapted besides aeff2d?

@cdeil cdeil self-assigned this Feb 17, 2016

@cdeil cdeil added this to the 0.4 milestone Feb 17, 2016

@adonath

This comment has been minimized.

Show comment
Hide comment
@adonath

adonath Feb 17, 2016

Member

Yes, the suggested default behaviour makes sense to me.

Member

adonath commented Feb 17, 2016

Yes, the suggested default behaviour makes sense to me.

@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Feb 17, 2016

Contributor

👍

In this context:
There are still some places in Gammapy where it is not clear whether the 1D objects (ARF, RMF) should also do some kind of interpolation.
https://github.com/gammapy/gammapy/blob/master/gammapy/irf/effective_area_table.py#L268
Maybe we could agree on that in this PR.
I would say: No interpolation in the 1D objects. The user can choose a binning when extracting ARF and RMF from the 2D objects. The interpolation must happen at this point IMO.

Contributor

joleroi commented Feb 17, 2016

👍

In this context:
There are still some places in Gammapy where it is not clear whether the 1D objects (ARF, RMF) should also do some kind of interpolation.
https://github.com/gammapy/gammapy/blob/master/gammapy/irf/effective_area_table.py#L268
Maybe we could agree on that in this PR.
I would say: No interpolation in the 1D objects. The user can choose a binning when extracting ARF and RMF from the 2D objects. The interpolation must happen at this point IMO.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Feb 17, 2016

Member

Merging this now (because I want to continue with this in a new PR and implement an Observation class and use that for exposure cube, background model, ....).

@joleroi - I don't know yet what interpolation behaviour for the 1D objects is best.
And there's more things to discuss / clean up concerning interpolator construction and caching.
(e.g. AEFF2D constructs the interpolator in the initialiser, but at the moment the interp_kwargs aren't exposed on the read and from_fits method).
And there's the question on whether we need splines and interpolator caching or whether the interpolator can / should always be constructed on the fly in the evaluate method. Let's discuss a bit and then hopefully someone has time to make extra pull requests to improve this stuff ...

Member

cdeil commented Feb 17, 2016

Merging this now (because I want to continue with this in a new PR and implement an Observation class and use that for exposure cube, background model, ....).

@joleroi - I don't know yet what interpolation behaviour for the 1D objects is best.
And there's more things to discuss / clean up concerning interpolator construction and caching.
(e.g. AEFF2D constructs the interpolator in the initialiser, but at the moment the interp_kwargs aren't exposed on the read and from_fits method).
And there's the question on whether we need splines and interpolator caching or whether the interpolator can / should always be constructed on the fly in the evaluate method. Let's discuss a bit and then hopefully someone has time to make extra pull requests to improve this stuff ...

cdeil added a commit that referenced this pull request Feb 17, 2016

Merge pull request #455 from cdeil/aeff-interp
Interpolation and extrapolation in Gammapy

@cdeil cdeil merged commit 2e82428 into gammapy:master Feb 17, 2016

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Feb 17, 2016

Contributor

What is the scope of this Observation class?
I guess there will be some overlap with this

https://github.com/gammapy/gammapy/blob/master/gammapy/spectrum/spectrum_extraction.py#L217

so maybe we could work together on this?

Contributor

joleroi commented Feb 17, 2016

What is the scope of this Observation class?
I guess there will be some overlap with this

https://github.com/gammapy/gammapy/blob/master/gammapy/spectrum/spectrum_extraction.py#L217

so maybe we could work together on this?

@cdeil cdeil changed the title from Interpolation and extrapolation in Gammapy to Improve IRF interpolation and extrapolation Apr 20, 2016

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