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 EffectiveAreaTable and EnergyDependentMultiGaussPSF classes #142

Merged
merged 4 commits into from Jul 22, 2014

Conversation

Projects
None yet
2 participants
@adonath
Member

adonath commented Jul 16, 2014

This implements a first version of EnergyDependentMultiGaussPSF and EnergyDependentTableARF classes. Test data and a corresponding gp-irf-info script is also contained.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jul 17, 2014

Member

I fixed the datasets/tev_spectra/crab issue in master ... could you please rebase this branch?

For python setup.py build_sphinx I get this error ... can you reproduce?
https://gist.github.com/cdeil/842c9b13278a264306d3

Member

cdeil commented Jul 17, 2014

I fixed the datasets/tev_spectra/crab issue in master ... could you please rebase this branch?

For python setup.py build_sphinx I get this error ... can you reproduce?
https://gist.github.com/cdeil/842c9b13278a264306d3

Show outdated Hide outdated gammapy/irf/effective_area.py Outdated

@cdeil cdeil referenced this pull request Jul 17, 2014

Closed

Integrate PyFACT functionality in gammapy #78

2 of 10 tasks complete
@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jul 17, 2014

Member

How about we rename the EnergyDependentTableARF class to TableEffectiveArea?

ARF and RMF are the rare cases where a specification and name exists for the file format, but most IRFs in gammapy (i.e. the ones CTA will use) do not have a name and maybe we could try to name all classes according to what they represent?

(we should definitely re-visit the question of IRF class names in a year when we have more in gammapy and it's more clear what CTA will do)

Member

cdeil commented Jul 17, 2014

How about we rename the EnergyDependentTableARF class to TableEffectiveArea?

ARF and RMF are the rare cases where a specification and name exists for the file format, but most IRFs in gammapy (i.e. the ones CTA will use) do not have a name and maybe we could try to name all classes according to what they represent?

(we should definitely re-visit the question of IRF class names in a year when we have more in gammapy and it's more clear what CTA will do)

Show outdated Hide outdated gammapy/irf/effective_area.py Outdated
Show outdated Hide outdated gammapy/irf/effective_area.py Outdated
Show outdated Hide outdated gammapy/irf/effective_area.py Outdated
Show outdated Hide outdated gammapy/irf/effective_area.py Outdated
Show outdated Hide outdated gammapy/irf/psf_analytical.py Outdated
"""
import matplotlib.pyplot as plt
# Set up and compute data

This comment has been minimized.

@cdeil

cdeil Jul 17, 2014

Member

Put this code that computes the containment radius array into a separate function.

It can be a private function for now (i.e. start with underscore) so you don't have to think about API and write a docstring, but it's always good to split computation from plotting so that it's more re-usable.

@cdeil

cdeil Jul 17, 2014

Member

Put this code that computes the containment radius array into a separate function.

It can be a private function for now (i.e. start with underscore) so you don't have to think about API and write a docstring, but it's always good to split computation from plotting so that it's more re-usable.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jul 17, 2014

Member

The two FITS files are small (10 kB), but still, why not gzip them (will be < 2 kB)?
CFITSIO and astropy.io.fits always decompress on the fly with no extra effort for the user.

Also I see that you have no FOV offset ("theta") dependence in the PSF ... this will be useful to have when we add interpolation ... could you please change the example (e.g. use a parabola with the PSF worse by 30% at 2 deg offset).
And please add the script you use to produce the test FITS files in the dev folder to the repo ... this will make it easier for us to change / extend / reproduce test datasets in the future.

Member

cdeil commented Jul 17, 2014

The two FITS files are small (10 kB), but still, why not gzip them (will be < 2 kB)?
CFITSIO and astropy.io.fits always decompress on the fly with no extra effort for the user.

Also I see that you have no FOV offset ("theta") dependence in the PSF ... this will be useful to have when we add interpolation ... could you please change the example (e.g. use a parabola with the PSF worse by 30% at 2 deg offset).
And please add the script you use to produce the test FITS files in the dev folder to the repo ... this will make it easier for us to change / extend / reproduce test datasets in the future.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jul 17, 2014

Member

I did a not of nitpicking here ... just wanted to say that overall this is a great contribution!

Member

cdeil commented Jul 17, 2014

I did a not of nitpicking here ... just wanted to say that overall this is a great contribution!

@cdeil cdeil added this to the 0.1 milestone Jul 17, 2014

@cdeil cdeil changed the title from Psf analytical class to Add PSF and ARF class Jul 17, 2014

@cdeil cdeil changed the title from Add PSF and ARF class to Add PSF and ARF class and gp-irf-info script Jul 17, 2014

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jul 18, 2014

Member

Is this ready to be merged or was there something you still wanted to change in this PR?

Member

cdeil commented Jul 18, 2014

Is this ready to be merged or was there something you still wanted to change in this PR?

@adonath

This comment has been minimized.

Show comment
Hide comment
@adonath

adonath Jul 21, 2014

Member

The Travis build failed, because get_pkg_data_filename tries to get the file remotely from the internet. The test passes for me locally. Any ideas? @cdeil

Member

adonath commented Jul 21, 2014

The Travis build failed, because get_pkg_data_filename tries to get the file remotely from the internet. The test passes for me locally. Any ideas? @cdeil

@adonath

This comment has been minimized.

Show comment
Hide comment
@adonath

adonath Jul 22, 2014

Member

All Travis builds have now passed. Any objections to merge this?

Member

adonath commented Jul 22, 2014

All Travis builds have now passed. Any objections to merge this?

adonath added a commit that referenced this pull request Jul 22, 2014

Merge pull request #142 from adonath/psf_analytical_class
Add PSF and ARF class and gp-irf-info script

@adonath adonath merged commit 4bd158a into gammapy:master Jul 22, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jul 22, 2014

Member

👍 to merge this. Thanks!

Member

cdeil commented Jul 22, 2014

👍 to merge this. Thanks!

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jul 22, 2014

Member

I'm seeing this error on Python 3 which is because you don't use a relative import:
https://gist.github.com/cdeil/f8847ad217dcc8c2b8e1#file-gistfile1-txt-L70

No idea why this didn't show up on travis-ci.
I'm fixing it directly in master now.

Member

cdeil commented Jul 22, 2014

I'm seeing this error on Python 3 which is because you don't use a relative import:
https://gist.github.com/cdeil/f8847ad217dcc8c2b8e1#file-gistfile1-txt-L70

No idea why this didn't show up on travis-ci.
I'm fixing it directly in master now.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jul 22, 2014

Member

@adonath The fix (together with some cosmetic changes) is here: 7a0b42e#diff-cfb4e8b67c434385b17b6cd254d4c5b0L277

Member

cdeil commented Jul 22, 2014

@adonath The fix (together with some cosmetic changes) is here: 7a0b42e#diff-cfb4e8b67c434385b17b6cd254d4c5b0L277

@cdeil cdeil changed the title from Add PSF and ARF class and gp-irf-info script to Add EffectiveAreaTable and EnergyDependentMultiGaussPSF classes Apr 8, 2015

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