Skip to content
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 sky cube computation for IACT data #826

Merged
merged 5 commits into from Dec 20, 2016
Merged

Conversation

@JouvinLea
Copy link

@JouvinLea JouvinLea commented Dec 19, 2016

@cdeil @adonath
Hi,
This PR is for cube analysis with two new class StackedObsCubeMakerand SingleObsCubeMaker. It allow to compute counts, bkg, significance, excess and exposure cube for a set of observations. In addition, you can compute the exposure cube in true energy what you need for the 3D spectral analysis if you want to take into account the energy resolution.

The test called all the methods of these two classe and build a background model from acceptance curve as for the test of image_pipe from the events of the 4 crab runs outside the exclusion region. I made also this kind of Cube analysis for the Crab with HAP-Fr data and a real bkg model base on the AGN runs. Then I apply the spectral 3D analysis taking into account the energy resolution (I will do soon a PR for that) and I give really the same result than for the 1D spectral analysis with pointlike IRF.

Cheers,
Lea

@adonath adonath self-assigned this Dec 19, 2016
@adonath adonath added this to the 0.6 milestone Dec 19, 2016
@JouvinLea
Copy link
Author

@JouvinLea JouvinLea commented Dec 19, 2016

@adonath @cdeil
And all the test are ok this is great(-: But I know there are maybe a lot to change.

@adonath
Copy link
Member

@adonath adonath commented Dec 19, 2016

@leajouvin Thanks for this PR! I'll take a closer look and review this afternoon.

Copy link
Member

@adonath adonath left a comment

@leajouvin I've left a few inline comments, nothing major...

outdir = tmpdir
outdir2 = outdir + '/background'

cmd = 'mkdir -p {}'.format(outdir2)

This comment has been minimized.

@adonath

adonath Dec 19, 2016
Member

Could you use the method tmpdir.mkdir() and remove the subprocess call and print statement here?


assert_allclose(cube_maker.counts_cube.data.sum(), 4898.0, atol=3)
assert_allclose(cube_maker.bkg_cube.data.sum(), 4259.818621739171, atol=3)
cube_maker.significance_cube.data[np.where(np.isnan(cube_maker.significance_cube.data))] = 0

This comment has been minimized.

@adonath

adonath Dec 19, 2016
Member

There's an option equal_nan=True for assert_allclose, which could be used instead of fixing the values manually.

This comment has been minimized.

@adonath

adonath Dec 19, 2016
Member

I just realized you only test on the sum, in this case you could use np.nansum as well.

if self.save_bkg_scale:
self.table_bkg_scale = Table(names=["OBS_ID", "bkg_scale"])

def make_images(self, make_background_image=False, bkg_norm=True, radius=10):

This comment has been minimized.

@adonath

adonath Dec 19, 2016
Member

This method is kind of a misnomer, because it actually computes cubes not images...maybe rename to make_cubes()?

Parameters
----------
make_background_image : bool

This comment has been minimized.

@adonath

adonath Dec 19, 2016
Member

The same here..I think it's actually a cube...

Lea Jouvin
@cdeil cdeil changed the title Cube analysis Add sky cube computation for IACT data Dec 19, 2016
@cdeil
Copy link
Member

@cdeil cdeil commented Dec 19, 2016

Just to say: I won't have time to review before Christmas. I'm +1 to merge soon (when @adonath is happy) and I'll try it out in January and give feedback based on that or try to improve it myself.

@JouvinLea
Copy link
Author

@JouvinLea JouvinLea commented Dec 20, 2016

@adonath
I think I took all your suggestions into account, the tests passed so maybe we can merge?

@adonath adonath merged commit 94cda4b into gammapy:master Dec 20, 2016
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@adonath
Copy link
Member

@adonath adonath commented Dec 20, 2016

@JouvinLea Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants