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 evaluate method Background3D IRF #1476

Merged
merged 5 commits into from Jul 11, 2018

Conversation

Projects
3 participants
@JouvinLea
Contributor

JouvinLea commented Jul 11, 2018

  • The evaluate method is based on the new evaluate method in NDdata array from a set of coordinates.
  • Clean a bit the test of background 3D with a more simple test

@JouvinLea JouvinLea requested review from cdeil and registerrier Jul 11, 2018

@registerrier

Thanks!
Some inline comments especially regarding naming of axes. We decided on using fov_lon and fov_lat for field of view X and Y.

FOV coordinate X-axis binning. Same dimension than det_y and energy_reco
dety: `~astropy.coordinates.Angle`
FOV coordinate Y-axis binning. Same dimension than det_x and energy_reco
energy_reco : `~astropy.units.Quantity`

This comment has been minimized.

@registerrier
@registerrier

This comment has been minimized.

@cdeil

cdeil Jul 11, 2018

Member

I'll do this before merging.

@cdeil

cdeil Jul 11, 2018

Member

I'll do this before merging.

bkg_rate = bkg_3d.data.evaluate(energy='1 TeV', detx='0.2 deg', dety='0.5 deg')
assert_allclose(bkg_rate.value, 0.00013352689711418575)
assert bkg_rate.unit == 's-1 MeV-1 sr-1'
# Evaluate at nodes in energy

This comment has been minimized.

@registerrier

registerrier Jul 11, 2018

Contributor

Shouldn't there still be a test of bkg3d.data.evaluate?

@registerrier

registerrier Jul 11, 2018

Contributor

Shouldn't there still be a test of bkg3d.data.evaluate?

This comment has been minimized.

@cdeil

cdeil Jul 11, 2018

Member

I would say no, a test for bkg_3d.data is not needed.
.data is an implementation detail that shouldn't be called.
The one existing caller should be adapted later today in a follow-up PR.

@cdeil

cdeil Jul 11, 2018

Member

I would say no, a test for bkg_3d.data is not needed.
.data is an implementation detail that shouldn't be called.
The one existing caller should be adapted later today in a follow-up PR.

@cdeil cdeil self-assigned this Jul 11, 2018

@cdeil cdeil added the feature label Jul 11, 2018

@cdeil cdeil added this to the 0.8 milestone Jul 11, 2018

@cdeil cdeil added this to To do in IRFs via automation Jul 11, 2018

@cdeil

cdeil approved these changes Jul 11, 2018

@JouvinLea - Please merge.

We can do the other suggestions from here (parameter renames and improve test case a bit) tomorrow.

@registerrier registerrier merged commit 2094a90 into gammapy:master Jul 11, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

IRFs automation moved this from To do to Done Jul 11, 2018

@registerrier

This comment has been minimized.

Show comment
Hide comment
@registerrier

registerrier Jul 11, 2018

Contributor

OK now. Merging.

Contributor

registerrier commented Jul 11, 2018

OK now. Merging.

def evaluate(self, detx, dety, energy_reco, method=None, **kwargs):
"""
Evaluate the `Background3D` at a given FOV coordinate and energy. The coordinates det_x, det_y and erngy_reco
should have the same dimension that match the numbers of point you want to evaluate

This comment has been minimized.

@cdeil

cdeil Jul 11, 2018

Member

I think you also need an empty line here above the Parameters section for the docs build to work.

@cdeil

cdeil Jul 11, 2018

Member

I think you also need an empty line here above the Parameters section for the docs build to work.

assert_allclose(bkg_rate.value, 0.00013352689711418575)
assert bkg_rate.unit == 's-1 MeV-1 sr-1'
# Evaluate at nodes in energy
res = bkg_3d.evaluate(detx=np.array([1, 0.5]) * u.deg, dety=np.array([1, 0.5]) * u.deg,

This comment has been minimized.

@cdeil

cdeil Jul 11, 2018

Member

You can remove the np.array calls here and the result should be exactly the same, just a bit less code.

@cdeil

cdeil Jul 11, 2018

Member

You can remove the np.array calls here and the result should be exactly the same, just a bit less code.

@cdeil cdeil changed the title from Add evaluate method to the background 3D to Add evaluate method Background3D IRF Aug 15, 2018

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