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 access CTA point-like responses #816

Merged
merged 2 commits into from Dec 16, 2016

Conversation

Projects
None yet
3 participants
@jjlk
Contributor

jjlk commented Dec 12, 2016

Hi @cdeil ,
I added some classes to handle point-like performance for the offical CTA's point-like IRF. I tried to stay as close as possible to the recommandations of https://gamma-astro-data-formats.readthedocs.io/en/latest/irfs/.
I added:

  • one class to handle the background rate, BgRateTable (following what is done for EffectiveAreaTable)
  • one class to handle the angular resolution, Psf68Table (following what is done for EffectiveAreaTable)
  • one class to store the four response functions, CTAPerf
    Thanks

@cdeil cdeil self-assigned this Dec 12, 2016

@cdeil cdeil added the feature label Dec 12, 2016

@cdeil cdeil added this to the 0.6 milestone Dec 12, 2016

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Dec 12, 2016

Member

@jjlk - Thanks!

All code without tests will break, since both data formats and code evolve quickly.
So can you add one or two high-level tests that execute the functionality, please?

Maybe for now, just one line to execute peek (and mark the test as requires_dependency('matplotlib') would be a start?

Member

cdeil commented Dec 12, 2016

@jjlk - Thanks!

All code without tests will break, since both data formats and code evolve quickly.
So can you add one or two high-level tests that execute the functionality, please?

Maybe for now, just one line to execute peek (and mark the test as requires_dependency('matplotlib') would be a start?

@jjlk

This comment has been minimized.

Show comment
Hide comment
@jjlk

jjlk Dec 12, 2016

Contributor

Hi @cdeil ,
I added the required test. Thanks!

Contributor

jjlk commented Dec 12, 2016

Hi @cdeil ,
I added the required test. Thanks!

@requires_dependency('matplotlib')
@requires_data('gammapy-extra')
def test_point_like_perf():

This comment has been minimized.

@cdeil

cdeil Dec 12, 2016

Member

We always try to name tests as similarly to classes / functions as possible.
In this case, my suggestion would be test_cta_perf.
Or if you want to mention "point like" for some reason, test_cta_perf_point_like.

I'll try this out locally now, to see if I get a useful plot.

@cdeil

cdeil Dec 12, 2016

Member

We always try to name tests as similarly to classes / functions as possible.
In this case, my suggestion would be test_cta_perf.
Or if you want to mention "point like" for some reason, test_cta_perf_point_like.

I'll try this out locally now, to see if I get a useful plot.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Dec 12, 2016

Member

I see this locally:

from gammapy.scripts import CTAPerf
filename = '$GAMMAPY_EXTRA/datasets/cta/perf_prod2/CTA-Performance-South-20150511/CTA-Performance-South-50h_20150511.fits'
cta_perf = CTAPerf.read(filename)
cta_perf.peek()

screen shot 2016-12-12 at 11 48 15

The EDISP looks incorrect, no?
And the other three plots look OK, right?

Member

cdeil commented Dec 12, 2016

I see this locally:

from gammapy.scripts import CTAPerf
filename = '$GAMMAPY_EXTRA/datasets/cta/perf_prod2/CTA-Performance-South-20150511/CTA-Performance-South-50h_20150511.fits'
cta_perf = CTAPerf.read(filename)
cta_perf.peek()

screen shot 2016-12-12 at 11 48 15

The EDISP looks incorrect, no?
And the other three plots look OK, right?

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Dec 12, 2016

Member

@jjlk - This is the EDISP plot you sent me via email a few days ago:

screen shot 2016-12-12 at 11 51 19

Do you still get this, if you use the code snippet from my last comment?

Member

cdeil commented Dec 12, 2016

@jjlk - This is the EDISP plot you sent me via email a few days ago:

screen shot 2016-12-12 at 11 51 19

Do you still get this, if you use the code snippet from my last comment?

hdu_list = fits.open(filename)
edisp = EnergyDispersion.from_hdulist(hdu_list=hdu_list)
# Do not work, can't understand why, see BgRateTable and Psf68Table class

This comment has been minimized.

@joleroi

joleroi Dec 12, 2016

Contributor

The problem is the following:

BgRateTable.read() is implemented here. As you see it calls

fits.open(*args, **kwargs)

However, hdu is not a valid kwarg. fits.open always returns the entire HDUList. For the effective are it works by change because it is stored in the first HDU. This behaviour of NDDataArray.read is obviously very bad.

But for now I propose to use the solution you found here, and change it as soons as the implementation of NDDataArray has improved. There is already an issue about the API of read but I can't find it at the moment.

@joleroi

joleroi Dec 12, 2016

Contributor

The problem is the following:

BgRateTable.read() is implemented here. As you see it calls

fits.open(*args, **kwargs)

However, hdu is not a valid kwarg. fits.open always returns the entire HDUList. For the effective are it works by change because it is stored in the first HDU. This behaviour of NDDataArray.read is obviously very bad.

But for now I propose to use the solution you found here, and change it as soons as the implementation of NDDataArray has improved. There is already an issue about the API of read but I can't find it at the moment.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Dec 12, 2016

Member

When reading and plotting the EDISP like this (i.e. in a way equivalent to what you do in the new class here), I get the EDISP plot with incorrect axis limits:

from gammapy.irf import EnergyDispersion
EnergyDispersion.read('$GAMMAPY_EXTRA/datasets/cta/perf_prod2/CTA-Performance-South-20150511/CTA-Performance-South-50h_20150511.fits')
edisp = EnergyDispersion.read('$GAMMAPY_EXTRA/datasets/cta/perf_prod2/CTA-Performance-South-20150511/CTA-Performance-South-50h_20150511.fits')
print(edisp)
edisp.plot_matrix()

I don't know what's wrong here, i.e. where the bug hides:

  • Is the EDISP filled correctly in the FITS file?
  • Is it read correctly?
  • Is it evaluated correctly?
  • Is it just a plotting issue?

@joleroi - Would you be willing to have a look and generally to take over review of this PR?
Or should we just merge now, and then you look at this and maybe do a follow-up PR in the coming days?

Member

cdeil commented Dec 12, 2016

When reading and plotting the EDISP like this (i.e. in a way equivalent to what you do in the new class here), I get the EDISP plot with incorrect axis limits:

from gammapy.irf import EnergyDispersion
EnergyDispersion.read('$GAMMAPY_EXTRA/datasets/cta/perf_prod2/CTA-Performance-South-20150511/CTA-Performance-South-50h_20150511.fits')
edisp = EnergyDispersion.read('$GAMMAPY_EXTRA/datasets/cta/perf_prod2/CTA-Performance-South-20150511/CTA-Performance-South-50h_20150511.fits')
print(edisp)
edisp.plot_matrix()

I don't know what's wrong here, i.e. where the bug hides:

  • Is the EDISP filled correctly in the FITS file?
  • Is it read correctly?
  • Is it evaluated correctly?
  • Is it just a plotting issue?

@joleroi - Would you be willing to have a look and generally to take over review of this PR?
Or should we just merge now, and then you look at this and maybe do a follow-up PR in the coming days?

@cdeil cdeil assigned joleroi and unassigned cdeil Dec 12, 2016

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Dec 12, 2016

Member

I've re-started travis-ci builds, now that the dataset in gammapy/gammapy-extra#44 has been merged.

Member

cdeil commented Dec 12, 2016

I've re-started travis-ci builds, now that the dataset in gammapy/gammapy-extra#44 has been merged.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Dec 16, 2016

Member

@jjlk - I'm merging this now. Thanks for the contribution!

@joleroi and I don't have time this week to look at the potential EDISP issue mentioned above.
I'll file a reminder issue and assign it to the v0.6 milestone.

Member

cdeil commented Dec 16, 2016

@jjlk - I'm merging this now. Thanks for the contribution!

@joleroi and I don't have time this week to look at the potential EDISP issue mentioned above.
I'll file a reminder issue and assign it to the v0.6 milestone.

@cdeil cdeil merged commit 90ec1e1 into gammapy:master Dec 16, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jjlk

This comment has been minimized.

Show comment
Hide comment
@jjlk

jjlk Dec 16, 2016

Contributor

Great!

Contributor

jjlk commented Dec 16, 2016

Great!

@jjlk jjlk deleted the jjlk:cta_perf branch Dec 16, 2016

@cdeil cdeil changed the title from Add some classes to store point-like CTA performance for convenience to Add class to access CTA point-like responses Dec 20, 2016

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