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 class to load CTA IRFs #648

Merged
merged 3 commits into from Jul 30, 2016
Merged

Add class to load CTA IRFs #648

merged 3 commits into from Jul 30, 2016

Conversation

@jjlk
Copy link
Contributor

@jjlk jjlk commented Jul 25, 2016

Hi,
Add a small piece of code to scripts/cta_irf.py with a new class CTAIrf (CTAIRF is ugly, right?) and a test (plus some minor fixes in the doc). For now there is a small bug i need to deal with before asking for a review

@jjlk jjlk added the feature label Jul 25, 2016
@cdeil cdeil added this to the 0.5 milestone Jul 25, 2016
@cdeil cdeil self-assigned this Jul 25, 2016
Position-dependent multi-Gauss PSF
"""

def __init__(self, area_2d=None, bkg_cube=None, e_disp=None, psf_mgauss=None):

This comment has been minimized.

@cdeil

cdeil Jul 26, 2016
Member

I get this error:

E       TypeError: __init__() got an unexpected keyword argument 'e_disp_2d'

You have to make the parameter names consistent, it's called e_disp once and then e_disp_2d later and edisp_2d in the docstring.

self.psf_mgauss = psf_mgauss

@classmethod
def from_fits(cls, irf_path):

This comment has been minimized.

@cdeil

cdeil Jul 26, 2016
Member

Rename from_fits -> read.
We always use read for the method that takes a filename.
from_fits is used for constructor methods that take astropy.io.fits objects if present.

Parameter is called irf_path and then path in the docstring. Usually we put filename.

table_hdu_bkg.header['TTYPE7'] = 'Bgd'
table_hdu_bkg.header['TUNIT7'] = '1 / (MeV s sr)'

bkg_cube = Cube.from_fits_table(table_hdu_bkg, scheme='bg_cube')

This comment has been minimized.

@cdeil

cdeil Jul 26, 2016
Member

This is failing with a TypeError on some travis-ci builds:
https://travis-ci.org/gammapy/gammapy/jobs/147251046#L940

I'm not getting this error locally, not sure yet what's causing it ...

!?

# to 0 and 1, respectively
table_hdu_psf = get_hdu(irf_path + '[POINT SPREAD FUNCTION]')

zero_array = np.zeros(shape=(2, 21), dtype='float32')

This comment has been minimized.

@cdeil

cdeil Jul 26, 2016
Member

Hard-coding a shape here is brittle, this will break with other IRFs.

Can't you just assign a scalar and let numpy broadcasting take care of it?

table_hdu_psf.data[0]['AMPL_2'] = 0

Alternatively, you could get the shape from an existing array like table_hdu_psf.data[0]['AMPL_2'].shape, no?

@cdeil
Copy link
Member

@cdeil cdeil commented Jul 26, 2016

I left some inline comments.

Did you want to add other things like obs time computation in this PR?
Or is this mostly done and the main thing left to do is figure out the TypeError for background?

Cube container class with scheme fixed to `bg_cube`
edisp_2d : `~gammapy.irf.EnergyDispersion2D`
Offset-dependent energy dispersion matrix
psf_mgauss : `~gammapy.irf.EnergyDispersion2D`

This comment has been minimized.

@cdeil

cdeil Jul 26, 2016
Member

Copy & paste error: type should be a PSF class for psf_mgauss

(https://portal.cta-observatory.org/Pages/CTA-Performance.aspx)
adapted from the ctools
(http://cta.irap.omp.eu/ctools/user_manual/getting_started/response.html).
The IRF format should be complient compliant with the one discussed

This comment has been minimized.

@cdeil

cdeil Jul 26, 2016
Member

Remove complient

at http://gamma-astro-data-formats.readthedocs.io/en/latest/irfs/.
Waiting for a new public production of the CTA IRF,
we'll fix the missing pieces.

This comment has been minimized.

@cdeil

cdeil Jul 26, 2016
Member

You could leave as a note or TODO a link to
https://gammapy.readthedocs.io/en/latest/api/gammapy.data.DataStoreObservation.html

Maybe something like this:

This class is similar to `~gammapy.data.DataStoreObservation`,
but only contains IRFs (no event data or livetime info).
TODO: maybe re-factor code somehow to avoid code duplication.
@jjlk
Copy link
Contributor Author

@jjlk jjlk commented Jul 26, 2016

Hi @cdeil,
I took your comments into account. The bug is still here and the error is pretty similar to the one you previsouly discussed in astropy/astropy#646 and in astropy/astropy#4804. Do you think it is related? The weird thing is that it works when putting the handling of the bkg cube outside the class.

I left some inline comments.
Did you want to add other things like obs time computation in this PR?
Or is this mostly done and the main thing left to do is figure out the TypeError for background?

No I guess I could open an other PR to transform the 2D IRF to 1D IRF and to work on the prediction stuff when the bug is exterminated.

@jjlk jjlk force-pushed the jjlk:cta_irf branch from 2c63088 to 1f48205 Jul 26, 2016
@jjlk jjlk force-pushed the jjlk:cta_irf branch from 1f48205 to 9dd7f1a Jul 26, 2016
@cdeil
Copy link
Member

@cdeil cdeil commented Jul 28, 2016

@cdeil
Copy link
Member

@cdeil cdeil commented Jul 28, 2016

Concerning the filename string error, I'm getting the same issue in Fermipy:
fermiPy/fermipy#63 (comment)
I'll have to think a bit about how to best handle this...

@cdeil
Copy link
Member

@cdeil cdeil commented Jul 28, 2016

I think I found a solution to the error:

TypeError: data type not understood

Replace

table_hdu_bkg.header['TTYPE7'] = 'Bgd'

with

table_hdu_bkg.columns.change_name(str('BGD'), str('Bgd'))

Note to future self: for debugging I've been using this script test.py:

from __future__ import unicode_literals
import os
from gammapy.utils.fits import get_hdu
from gammapy.background import Cube
filename = os.getenv('GAMMAPY_EXTRA') + '/datasets/cta/perf_prod2/South_5h/irf_file.fits.gz'
table_hdu_bkg = get_hdu(filename + '[BACKGROUND]')
# import IPython; IPython.embed()
table_hdu_bkg.columns.change_name(str('BGD'), str('Bgd'))
# table_hdu_bkg.header['TTYPE7'] = str('Bgd')
table_hdu_bkg.header['TUNIT7'] = '1 / (MeV s sr)'
table_hdu_bkg.data
Cube.from_fits_table(table_hdu_bkg, scheme='bg_cube')
@cdeil cdeil merged commit 1d8e09f into gammapy:master Jul 30, 2016
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@cdeil
Copy link
Member

@cdeil cdeil commented Jul 30, 2016

@jjlk - I added a commit (428b4c6) to apply the unicode fix I mentioned above and extend the tests a bit and merged this.

Thanks!

@cdeil cdeil changed the title Add a new class storing CTA IRF Add class to load CTA IRFs Jul 30, 2016
@jjlk jjlk deleted the jjlk:cta_irf branch Oct 3, 2016
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

2 participants