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 King profile PSF class #447

Merged
merged 1 commit into from Feb 24, 2016

Conversation

Projects
None yet
2 participants
@cdeil
Member

cdeil commented Feb 11, 2016

This PR adds a first version of the King profile PSF class:
http://gamma-astro-data-formats.readthedocs.org/en/latest/irfs/psf/psf_king/index.html

The goal is to make this work and print useful info and plot R68 and R95 / R68 as a function of energy and offset:

gammapy-data-show $GAMMAPY_EXTRA/test_datasets/irf/hess/pa/hess_psf_king_023523.fits.gz psf_king -p

@cdeil cdeil added the feature label Feb 11, 2016

@cdeil cdeil self-assigned this Feb 11, 2016

@cdeil cdeil added this to the 0.4 milestone Feb 11, 2016

@cdeil cdeil modified the milestones: 0.5, 0.4 Feb 19, 2016

@cdeil cdeil referenced this pull request Feb 24, 2016

Merged

Pacman #461

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Feb 24, 2016

Member

@joleroi - Have a look at gammapy/irf/tests/test_psf_king.py.

Something similar, but more extensive and using the datastore, could be useful to make sure Gammapy works with HESS data from all three chains.

Member

cdeil commented Feb 24, 2016

@joleroi - Have a look at gammapy/irf/tests/test_psf_king.py.

Something similar, but more extensive and using the datastore, could be useful to make sure Gammapy works with HESS data from all three chains.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Feb 24, 2016

Member

Merging this now.
Will continue in a new branch later today.

Member

cdeil commented Feb 24, 2016

Merging this now.
Will continue in a new branch later today.

cdeil added a commit that referenced this pull request Feb 24, 2016

Merge pull request #447 from cdeil/psf-king
Add King profile PSF class

@cdeil cdeil merged commit a87e78d into gammapy:master Feb 24, 2016

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Feb 24, 2016

Contributor

Hi,
I think the idea to parametrize the test for all chains is very good. However, changing all the reference values for each iteration of the FITS prod manually is quite a lot of work. I started doing the comparison to reference value by comparing the output of the info function, see e.g.

https://github.com/gammapy/gammapy/blob/master/gammapy/irf/tests/test_effective_area.py#L39

Of course there you can have problems with floating point numbers, but if you restrict the info output to a certain number of digit (which makes sense anyway to provide usefull information for the user) it should be fine.

What do you think?

Contributor

joleroi commented Feb 24, 2016

Hi,
I think the idea to parametrize the test for all chains is very good. However, changing all the reference values for each iteration of the FITS prod manually is quite a lot of work. I started doing the comparison to reference value by comparing the output of the info function, see e.g.

https://github.com/gammapy/gammapy/blob/master/gammapy/irf/tests/test_effective_area.py#L39

Of course there you can have problems with floating point numbers, but if you restrict the info output to a certain number of digit (which makes sense anyway to provide usefull information for the user) it should be fine.

What do you think?

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Feb 24, 2016

Member

I think the test comparison is a nice idea.

But I think it would be better to have normal assert and assert_allclose based tests.
Note that the numbers to assert on can be stored in the same dict or YAML file as the input parameters and filenames or datastore names.
Even the precision could be stored there.
Then updating to a new prod would just mean updating the input and output numbers in one file,
the test code can remain unchanged.

Member

cdeil commented Feb 24, 2016

I think the test comparison is a nice idea.

But I think it would be better to have normal assert and assert_allclose based tests.
Note that the numbers to assert on can be stored in the same dict or YAML file as the input parameters and filenames or datastore names.
Even the precision could be stored there.
Then updating to a new prod would just mean updating the input and output numbers in one file,
the test code can remain unchanged.

@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Feb 24, 2016

Contributor

I don't understand your proposal. How is it different from what is currently done in gammapy/irf/tests/test_psf_king.py

The idea of the file comparison is that it's easy to generate by just running
object.info() and copying the output to a file. This way we can also keep track of all changes including binning and head keywords. E.g. for the AEFF2D the reference file looks like this at the moment.

Summary EffectiveArea2D info
----------------
energy         : size =    74, min =  0.020 TeV, max = 89.337 TeV
offset         : size =     6, min =  0.000 deg, max =  2.500 deg
effective area : size =   438, min =  0.000 m2, max = 806261.375 m2
Safe energy threshold lo:  0.603 TeV
Safe energy threshold hi: 99.083 TeV

This is compared as string using an assert. If there is a difference, the output will tell you which number is different.

If you have to update all these number manually for 5 IRF classes + high level results, this seem quite annoying to me

Contributor

joleroi commented Feb 24, 2016

I don't understand your proposal. How is it different from what is currently done in gammapy/irf/tests/test_psf_king.py

The idea of the file comparison is that it's easy to generate by just running
object.info() and copying the output to a file. This way we can also keep track of all changes including binning and head keywords. E.g. for the AEFF2D the reference file looks like this at the moment.

Summary EffectiveArea2D info
----------------
energy         : size =    74, min =  0.020 TeV, max = 89.337 TeV
offset         : size =     6, min =  0.000 deg, max =  2.500 deg
effective area : size =   438, min =  0.000 m2, max = 806261.375 m2
Safe energy threshold lo:  0.603 TeV
Safe energy threshold hi: 99.083 TeV

This is compared as string using an assert. If there is a difference, the output will tell you which number is different.

If you have to update all these number manually for 5 IRF classes + high level results, this seem quite annoying to me

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Feb 24, 2016

Member

I'm OK with using the info string assert where it works.

But I'm not sure it will work well for high-level results such as flux and spectral index.
There depending on the analysis some fine-tuning of rtol and atol is often needed, and I'm not sure one generic info output with given number of digits would work well.
You've already experienced this issue recently, right?

Member

cdeil commented Feb 24, 2016

I'm OK with using the info string assert where it works.

But I'm not sure it will work well for high-level results such as flux and spectral index.
There depending on the analysis some fine-tuning of rtol and atol is often needed, and I'm not sure one generic info output with given number of digits would work well.
You've already experienced this issue recently, right?

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Feb 24, 2016

Member

And I'm not sure adding and updating a ton of info.txt files is more or less pleasant than editing a YAML file or Python dict / list. It's about the same, no?

Member

cdeil commented Feb 24, 2016

And I'm not sure adding and updating a ton of info.txt files is more or less pleasant than editing a YAML file or Python dict / list. It's about the same, no?

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Feb 24, 2016

Member

For reference: we discussed offline --- @joleroi convinced me that the info text comparison would be better, because it's less effort to add lots of tests for many different analyses, and to update the reference results when the test dataset updates.

Member

cdeil commented Feb 24, 2016

For reference: we discussed offline --- @joleroi convinced me that the info text comparison would be better, because it's less effort to add lots of tests for many different analyses, and to update the reference results when the test dataset updates.

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