Change SkyGaussian to spherical representation #2034
Conversation
…alization on the celestial sphere
…metric-gaussian-spatial-model-on-the-sphere
There was a problem hiding this comment.
Thanks @luca-giunti! This looks very good to me. I've left two inline comments about the quantity
handling in .evaluate() once those are addressed the PR is ready to be merged.
gammapy/image/models/core.py
Outdated
| exponent = -0.5 * (sep / sigma) ** 2 | ||
| return norm * np.exp(exponent) | ||
| a = 1.0 - np.cos(sigma) | ||
| norm = (1 / (4 * np.pi * a * (1.0 - np.exp(-1.0 / a)))) * u.sr ** -1 |
There was a problem hiding this comment.
The term (...) * u,sr ** -1 creates a new quantity object every time the method is evaluated. By default all the data will be copied, which slows down the computation a lot. Please remove it here. Here is some info about this http://docs.astropy.org/en/stable/units/index.html#astropy-units-performance
gammapy/image/models/core.py
Outdated
| a = 1.0 - np.cos(sigma) | ||
| norm = (1 / (4 * np.pi * a * (1.0 - np.exp(-1.0 / a)))) * u.sr ** -1 | ||
| exponent = -0.5 * ((1 - np.cos(sep)) / a) | ||
| return (norm * np.exp(exponent)).to("deg-2") |
There was a problem hiding this comment.
Instead creating the quantity and doing the unit conversion here could you just create a Quantity object like so:
return u.Quantity(norm * np.exp(exponent), unit="sr1", copy=False)
The same as is done for the SkyDisk model...
|
Thank you @adonath for your comment! I agree, now the |
adonath
changed the title
adonath
changed the title
Redefined the
SkyGaussianspatial model. The new function is correctly normalized on the celestial sphere, and reduces to an ordinary Gaussian definition in small angle approximation.