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 King PSF evaluate and to_table_psf methods #526

Merged
merged 13 commits into from May 9, 2016
Merged

Conversation

@JouvinLea
Copy link

@JouvinLea JouvinLea commented May 4, 2016

@adonath
Hey,
I started to implement the evaluate and to_table_psf for the king profile!
I also added a read and write method!!
I'm not sure that we can implement a containment radius, there is no analytical formula for the king profile?

self.norms[2].flatten(), self.sigmas[2].flatten()]
self.norms[0], self.sigmas[0],
self.norms[1], self.sigmas[1],
self.norms[2], self.sigmas[2]]

This comment has been minimized.

@JouvinLea

JouvinLea May 4, 2016
Author

@adonath
If you flatten there is a problem when you read it again ?

This comment has been minimized.

@joleroi

joleroi May 4, 2016
Contributor

What do you mean? Is there a problem when reading a fits file?

This comment has been minimized.

@JouvinLea

JouvinLea May 4, 2016
Author

@joleroi
yeah because since this is flatten there is only one dimension whereas the tab norm and sigmas are 2D (Energy*Offset)

"""
r2 = r * r
sigma2 = sigma * sigma

term1 = 1 / (2 * np.pi * sigma2)
term2 = 1 - 1 / gamma
term3 = (1 + r2 / 2 * gamma * sigma2) ** (-gamma)
term3 = (1 + r2 / (2 * gamma * sigma2)) ** (-gamma)

This comment has been minimized.

@JouvinLea

JouvinLea May 4, 2016
Author

There was a big mistake for the parenthesis here I think

param_king = self.evaluate(energy, theta)
psf_value[i] = Quantity(self.evaluate_direct(offset, param_king["gamma"], param_king["sigma"]), 'deg^-2')

return EnergyDependentTablePSF(energy=energies, offset=offset,

This comment has been minimized.

@JouvinLea

JouvinLea May 4, 2016
Author

@adonath
Since there is no analytical formula for the containment radius, I don't know exactly what could be a good test for the to_table_psf?

This comment has been minimized.

@joleroi

joleroi May 4, 2016
Contributor

Could you create a table PSF instance and check that the value in a certain bin is the same as when you evaluate the KingPSF class at that bin?

Instance of `EnergyDependentTablePSF`.
"""
# self.energy is already the logcenter
energies = self.energy

This comment has been minimized.

@JouvinLea

JouvinLea May 4, 2016
Author

@adonath
This self.energy was already implemented in king. Do you think that it will be a good idea to use the same for the Multigauss or do you want me to use as you did energy_lo and energy_hi ?

This comment has been minimized.

@joleroi

joleroi May 4, 2016
Contributor

I think in general the IRF module will change in the future, to be more consistent. For now just do what works for you, even if it's done differently somewhere else

@adonath adonath added the feature label May 4, 2016
@adonath adonath assigned joleroi and adonath and unassigned joleroi and adonath May 4, 2016
self.offset = Angle(offset)
self.energy = Energy(energy)
self.energy = np.sqrt(self.energy_lo * self.energy_hi)

This comment has been minimized.

@joleroi

joleroi May 4, 2016
Contributor

Just in case you didn't know, there is an EnergyBounds class in gammapy
https://github.com/gammapy/gammapy/blob/master/gammapy/utils/energy.py#L163

I would prefer all classes to accept and EnergyBounds instance as input in the constructor. If you want the log bin center as in this case you can just call EnergyBounds.log_centers. But it's up to you.

see also http://gammapy.readthedocs.io/en/latest/utils/index.html#energy-handling-in-gammapy

energy = np.sqrt(energy_lo * energy_hi)
energy = Energy(energy, unit=table['ENERG_LO'].unit)
energy_lo = Energy(energy_lo, unit=table['ENERG_LO'].unit)
energy_hi = Energy(energy_hi, unit=table['ENERG_HI'].unit)

This comment has been minimized.

@joleroi

joleroi May 4, 2016
Contributor

here you can use EnergyBounds.from_upper_and_lower_bounds



@requires_data('gammapy-extra')
def test_psf_king_to_table():

This comment has been minimized.

@joleroi

joleroi May 4, 2016
Contributor

what's the difference between this test and the one above?

data = [self.energy_lo, self.energy_hi, self.offset, self.offset,
self.sigma, self.gamma]

table = Table()

This comment has been minimized.

@joleroi

joleroi May 4, 2016
Contributor

if you want to you can split this out into a to_table method, but I don't think it's necessary at the moment

@joleroi
Copy link
Contributor

@joleroi joleroi commented May 4, 2016

Hi, I've left some comments, but they're all minor, if you don't want to adress them, let's me know and I'll merge

@JouvinLea
Copy link
Author

@JouvinLea JouvinLea commented May 4, 2016

@joleroi
Normally I took them into account! so if the tests passes on Travis we can merge(-:

assert_quantity_allclose(psf_king_table_off1.psf_value[8, 200], value_off1)
assert_quantity_allclose(psf_king_table_off2.psf_value[8, 200], value_off2)

# Test that the integral value is close to one

This comment has been minimized.

@JouvinLea

JouvinLea May 4, 2016
Author

@joleroi
Add this test to check that the histrogram is normalized

@JouvinLea
Copy link
Author

@JouvinLea JouvinLea commented May 4, 2016

@joleroi
Not sure to understand why the test failed..

@bsipocz
Copy link
Member

@bsipocz bsipocz commented May 4, 2016

@JouvinLea - random fail, one with appveyor rights should restart the build.

@JouvinLea
Copy link
Author

@JouvinLea JouvinLea commented May 5, 2016

@joleroi @bsipocz
Hi,
All the test passed now, can we merge?

@joleroi
Copy link
Contributor

@joleroi joleroi commented May 9, 2016

@JouvinLea sorry for the delay, there was a public holiday in Germany last thursday. Merging this now

@joleroi joleroi changed the title Psf king Implement evaluate and to_table_psf for King PSF May 9, 2016
@joleroi joleroi added this to the 0.5 milestone May 9, 2016
@joleroi joleroi merged commit 80ed90e into gammapy:master May 9, 2016
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@cdeil cdeil changed the title Implement evaluate and to_table_psf for King PSF Add King PSD evaluate and to_table_psf methods May 19, 2016
@cdeil cdeil changed the title Add King PSD evaluate and to_table_psf methods Add King PSF evaluate and to_table_psf methods Jun 9, 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

4 participants