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 Background 2D class #1312

Merged
merged 5 commits into from Feb 16, 2018

Conversation

Projects
None yet
2 participants
@JouvinLea
Contributor

JouvinLea commented Feb 15, 2018

Here just the Background 2D class added with the evaluate method.
Keep the Background 3D improvements and integrate methode for another PR!

(this is a follow-up to #1311)

Lea Jouvin

@JouvinLea JouvinLea requested a review from cdeil Feb 15, 2018

@cdeil

I left some inline comments, asking for an even simpler test case and to have I/O methods on 2d and 3d background be the same.

"""Background 2D.
Data format specification: :ref:`gadf:bkg_3d`

This comment has been minimized.

@cdeil

cdeil Feb 15, 2018

Member

3d -> 2d

def make_test_array():
# Create a dummy `Background2D`

This comment has been minimized.

@cdeil

cdeil Feb 15, 2018

Member

I think it would be better to have an even simpler test object. (even less code, easier to think about in the test methods below)

How about this?

energy = [1, 3, 5, 10] * u.TeV
offset = [0, 1, 5] * u.deg
data = energy.value * offset.value * u.Unit('s-1 MeV-1 sr-1')
return Background2D(
    energy_lo=energy[:-1], energy_hi=energy[1:],
    offset_lo=offset[:-1], offset_hi=offset[1:],
    data=data,
)

This is small, i.e. fast. And it's less code. And it's simpler to see what the expected value is in evaluate calls, if you choose a few points carefully (e.g. at a node and in the middle between two nodes, and throw in a third case that leads to extrapolation (e.g. offset=0) to establish behaviour).

I think we should do all our IRF tests like this: have super-simple test cases and fill them with data values that is easy and generic to write when making the test case, and also to know what to expect in evaluate.

assert data.unit == u.Unit('s-1 MeV-1 sr-1')
def test_background2d_evaluate():

This comment has been minimized.

@cdeil

cdeil Feb 15, 2018

Member

This needs a requires_dependency('scipy').
Generally I would suggest you follow the coding pattern from the 3D case, i.e. return the test case as a pytest fixture and inject it into tests.

return table
def to_hdulist(self, name='BACKGROUND'):

This comment has been minimized.

@cdeil

cdeil Feb 15, 2018

Member

Given that our IRFs serialise into a single HDU, having a method that returns a HDU list isn't very useful, no?
Also it's bad to have different methods for Background2D / Background3D.

This boilerplate code will probably anyways go to a base class (see #1157), but for now, I'd suggest to make Background2D and Background3D the same.

@cdeil cdeil self-assigned this Feb 15, 2018

@cdeil cdeil added the feature label Feb 15, 2018

@cdeil cdeil added this to the 0.7 milestone Feb 15, 2018

Lea Jouvin
prim_hdu = fits.PrimaryHDU()
hdu_bkg = bkg_2d_1.to_fits()
hdulist = fits.HDUList([prim_hdu, hdu_bkg])
hdulist.writeto(filename)

This comment has been minimized.

@JouvinLea

JouvinLea Feb 15, 2018

Contributor

@cdeil
Still get the same issue that if I am not writing the file, with only using from_hdulist(), I lost the dimensions quantity... I checked for example for the effective area, they are writting the file and using the function read then.

Lea Jouvin
res = bkg_2d.evaluate(fov_offset=off, energy_reco=e_reco)
assert_quantity_allclose(res, bkg_2d.data.data)
#Test linear interpolation for offset

This comment has been minimized.

@JouvinLea

JouvinLea Feb 15, 2018

Contributor

@cdeil
This is easy to to a easy test example regarding the offset. But since the nodes will be the log center for the energy then I can not use just integer anymore

This comment has been minimized.

@cdeil

cdeil Feb 15, 2018

Member

I think I would manually compute sqrt(e_1 * e_2), i.e. log center, and then use that in the evaluate call, see if it's giving the mean as value between the IRF value at the two nodes in one case.

Lea Jouvin
@JouvinLea

This comment has been minimized.

Contributor

JouvinLea commented Feb 15, 2018

@cdeil If the final tests passed, it's ready to merge!

@cdeil

This comment has been minimized.

Member

cdeil commented Feb 16, 2018

This looks good, thanks!
I've simplified the tests a little bit in b9bef8d .

Removing the boilerplate code for the I/O and changing NDDataArray coordinate broadcasting behaviour as well as new features will be a topic for future PRs.

@cdeil

cdeil approved these changes Feb 16, 2018

@cdeil cdeil merged commit f48b22d into gammapy:master Feb 16, 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
@cdeil

This comment has been minimized.

Member

cdeil commented Feb 18, 2018

Follow-up commit in master with small docstring fix: 5771201

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