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 offset-dependent effective area IRF class #260

Merged
merged 5 commits into from May 19, 2015

Conversation

Projects
None yet
3 participants
@joleroi
Contributor

joleroi commented May 6, 2015

basic implementation of the EffArea2D class

joleroi added some commits Apr 30, 2015

Added aeff2D class to read the HAP output file produces by hap-to-irf…
… (follows some standard)

Tested functionality so far: Get effarea vector for spectral analysis

@cdeil cdeil added the feature label May 6, 2015

@cdeil cdeil added this to the 0.3 milestone May 6, 2015

@cdeil cdeil self-assigned this May 6, 2015

hdu_list : `~astropy.io.fits.HDUList`
aeff2D file contents.
"""
filename = get_path('irfs/aeff2D.fits')

This comment has been minimized.

@cdeil

cdeil May 6, 2015

Member

Commit the example FITS file to the repo?
(It's small, right?)

This comment has been minimized.

@joleroi

joleroi May 6, 2015

Contributor

20K, git says however
The following paths are ignored by one of your .gitignore files:
gammapy/datasets/data/irfs/aeff2D.fits
should I force it?

This comment has been minimized.

@cdeil

cdeil May 8, 2015

Member

Yes, use git add -f to add this file and fix this test error on travis-ci:
https://travis-ci.org/gammapy/gammapy/jobs/61775130#L1033

class OffsetDependentTableEffectiveArea(object):
"""Offset-dependent radially-symmetric table effective area (``GCTAAeff2D FITS`` format).
The table is read by interpolated by triangulating the input data with Qhull (http://www.qhull.org/R42).

This comment has been minimized.

@cdeil

cdeil May 6, 2015

Member

Refer to the scipy classes you are using from the docstring instead of mentioning Qhull here. E.g.

Interpolation is done with `~scipy.interpolate.RectBivariateSpline` or ...
eff_area : `OffsetDependentTableEffectiveArea`
Offset dependent Effective Area object.
"""
energ_lo = Quantity(hdu_list['EFFECTIVE AREA'].data['ENERG_LO'][0,:], 'TeV')

This comment has been minimized.

@cdeil

cdeil May 6, 2015

Member

Create this temp variable to save characters?

data = hdu_list['EFFECTIVE AREA'].data
energ_lo = Quantity(data['ENERG_LO'][0,:], 'TeV')
...
from astropy.table import Table
from ..extern.validator import validate_physical_type
from ..utils.array import array_stats_str
from scipy.interpolate import LinearNDInterpolator, RectBivariateSpline

This comment has been minimized.

@cdeil

cdeil May 6, 2015

Member

At least at the moment scipy is an optional dependency and you have to move this import into the method where you use it.

This comment has been minimized.

@joleroi

joleroi May 6, 2015

Contributor

Ah, I was wondering why you have imports in methods sometimes!

@@ -1,14 +1,19 @@
# Licensed under a 3-clause BSD style license - see LICENSE.rst
from __future__ import print_function, division
import numpy as np
import matplotlib.pyplot as plt

This comment has been minimized.

@cdeil

cdeil May 6, 2015

Member

Same here, matplotlib is an optional dependency ... people should be able to run analyses without it.
So move this import into the methods where you use it – yes, this is duplicating the import a few times, there's not better way.

@pytest.mark.skipif('not HAS_SCIPY')
def test_OffsetDependentTableEffectiveArea():
print("Executing tests ...)")

This comment has been minimized.

@cdeil

cdeil May 6, 2015

Member

Remove print statement.

@cdeil cdeil changed the title from Effarea to Add offset-dependent effective area IRF class May 8, 2015

Offset dependent Effective Area object.
"""
e_lo = Quantity(
hdu_list['EFFECTIVE AREA'].data['ENERG_LO'][0, :], 'TeV')

This comment has been minimized.

@cdeil

cdeil May 8, 2015

Member

Use temp variable data = hdu_list['EFFECTIVE AREA'].data so that these statements fit on one line.

Parameters
----------
offset : Angle

This comment has been minimized.

@cdeil

cdeil May 8, 2015

Member

Use this:

offset : `~astropy.coordinates.Angle`

Then if you do python setup.py build_sphinx the data type will be a link to the corresponding class in the Astropy docs, which I think is convenient and warrants the extra typing.

----------
offset : Angle
offset
energy : Quantity

This comment has been minimized.

@cdeil

cdeil May 8, 2015

Member

Same here ... make this a Sphinx link.

y = np.log10(self.energy.value)
coord = [(xx, yy) for xx in x for yy in y]
self._linear = LinearNDInterpolator(

This comment has been minimized.

@cdeil

cdeil May 8, 2015

Member

I'm not a big fan of these line breaks ... probably they were introduced by autopep8.
Can you use temp variables instead, i.e. in this case something like:

vals = self.eff_area.value.flatten())
self._linear = LinearNDInterpolator(coord, vals)
interpolation_methods = ('linear', 'spline')
for method in interpolation_methods:

This comment has been minimized.

@cdeil

cdeil May 8, 2015

Member

Can you please use pytest parametrize instead of looping in the test function?

Docs: https://pytest.org/latest/parametrize.html
Examples: https://github.com/astropy/astropy/search?utf8=%E2%9C%93&q=parametrize

It's a bit complicated to learn how to use this, but pretty powerful and worth it in the long run.

@cdeil

This comment has been minimized.

Member

cdeil commented May 8, 2015

I've left a few inline comments.

FYI : The main criterion before a pull request can be merged is that the travis-ci tests pass. Otherwise the testing in master would be broken and tests in all other pull requests would fail.

@joleroi joleroi force-pushed the joleroi:effarea branch 3 times, most recently from e38bbc2 to 3c0456d May 11, 2015

Removed loop in tests -> pytest.parametrize
Unified evaluate functions to only one function self.evaluate
Adjusted plot functions
Did pep8 stuff

@joleroi joleroi force-pushed the joleroi:effarea branch from 3c0456d to 698720e May 12, 2015

@coveralls

This comment has been minimized.

coveralls commented May 12, 2015

Coverage Status

Coverage increased (+0.21%) to 46.09% when pulling 3e926bc on kingj90:effarea into b56a56a on gammapy:master.

"""
data = hdu_list['EFFECTIVE AREA'].data
e_lo = Quantity(data['ENERG_LO'][0, :], 'TeV')

This comment has been minimized.

@joleroi

joleroi May 12, 2015

Contributor

use np.squeeze?

@coveralls

This comment has been minimized.

coveralls commented May 12, 2015

Coverage Status

Coverage increased (+0.21%) to 46.09% when pulling 81babb0 on kingj90:effarea into b56a56a on gammapy:master.

@cdeil

This comment has been minimized.

Member

cdeil commented May 13, 2015

@kingj90 – Looks good to me. Should I merge this? Or was there something else you wanted to do in this PR?

@joleroi

This comment has been minimized.

Contributor

joleroi commented May 19, 2015

@cdeil : Yes, please merge it, I am not planning to change anything for now. I will instead turn towards HSPEC in gammapy

@cdeil

This comment has been minimized.

Member

cdeil commented May 19, 2015

Thanks!

I think this is a great addition ... it's important we can process the same inputs as ctools, and it's very convenient to only have to exports IRFs once and select the FOV position later from a Python script.

cdeil added a commit that referenced this pull request May 19, 2015

Merge pull request #260 from kingj90/effarea
Add offset-dependent effective area IRF class

@cdeil cdeil merged commit 6c0ad10 into gammapy:master May 19, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.21%) to 46.09%
Details

@joleroi joleroi deleted the joleroi:effarea branch Jul 15, 2015

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