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 elongated gaussian model #2313

Merged
merged 5 commits into from Aug 23, 2019

Conversation

@luca-giunti
Copy link
Contributor

commented Jul 29, 2019

This PR introduces an asymmetric Gaussian spatial model. Some remarks:

  • as discussed in #1663, the model is defined on the sphere but normalized in small angle approximation
  • this addition is accompanied by a test notebook that will be uploaded to https://github.com/gammapy/gammapy-extra/tree/master/checks after the PR approval
  • for the sake of consistency, I renamed the "theta" in the SkyGaussian docstring to "r(lon, lat)" and adopted the same notation for the SkyGaussianElongated. Indeed, for the elongated models theta refers to the rotation angle of the model (as in the case of the SkyEllipse)
  • the example produces the figure here attached
    el_g
@luca-giunti luca-giunti requested a review from adonath Jul 29, 2019
@registerrier registerrier added this to the 0.14 milestone Jul 29, 2019
@registerrier registerrier added this to In progress in Modeling via automation Jul 29, 2019
@cdeil cdeil self-assigned this Aug 2, 2019
Copy link
Member

left a comment

@luca-giunti - Thanks!

See two inline comments.

We have agreed on the following naming conventions last year:
https://docs.gammapy.org/0.13/development/howto.html#coordinate-and-axis-names

Please keep "sigma" or "theta" for radial offsets, and use "phi" or "position_angle" for position angle. I would suggest "phi", especially if you keep the short "e" instead of choosing "ellipticity".
We can / will re-dicuss all model and parameter names in ~ a month when Axel and Regis are back, so what we put here likely won't be the final v1.0 names.

gammapy/image/models/core.py Show resolved Hide resolved
gammapy/image/models/core.py Outdated Show resolved Hide resolved
gammapy/image/models/tests/test_core.py Outdated Show resolved Hide resolved
@luca-giunti luca-giunti force-pushed the luca-giunti:Add-elongated-Gaussian-model branch from 12ae3e9 to ccafdba Aug 23, 2019
@luca-giunti

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2019

@cdeil Thanks for the review. I tried to build the docs, and noticed that the example doesn't display the plot as it should. Can you see why?

@cdeil

This comment has been minimized.

Copy link
Member

commented Aug 23, 2019

The plot script was missing the right import at the top. I got this error from the Sphinx build log:

/Users/deil/work/code/gammapy2/build/lib.macosx-10.7-x86_64-3.7/gammapy/image/models/core.py:docstring of gammapy.image.models.SkyGaussianElongated:62: WARNING: Exception occurred in plotting gammapy-image-models-SkyGaussianElongated-1
 from /Users/deil/work/code/gammapy2/docs/api/gammapy.image.models.SkyGaussianElongated.rst:
Traceback (most recent call last):
  File "/Users/deil/software/anaconda3/envs/gammapy-dev/lib/python3.7/site-packages/matplotlib/sphinxext/plot_directive.py", line 515, in run_code
    exec(code, ns)
  File "<string>", line 10, in <module>
NameError: name 'SkyGaussianElongated' is not defined

I'll push a fix for this and some cleanup now, and merge this in.

@cdeil
cdeil approved these changes Aug 23, 2019
Copy link
Member

left a comment

@luca-giunti - Thanks!

@cdeil cdeil merged commit 7c77e21 into gammapy:master Aug 23, 2019
6 of 9 checks passed
6 of 9 checks passed
Codacy/PR Quality Review Hang in there, Codacy is reviewing your Pull request.
Details
Scrutinizer Running
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
gammapy.gammapy Build #20190823.7 succeeded
Details
gammapy.gammapy (DevDocs) DevDocs succeeded
Details
gammapy.gammapy (Lint) Lint succeeded
Details
gammapy.gammapy (Test Python36) Test Python36 succeeded
Details
gammapy.gammapy (Test Windows36) Test Windows36 succeeded
Details
gammapy.gammapy (Test Windows37) Test Windows37 succeeded
Details
Modeling automation moved this from In progress to Done Aug 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Modeling
  
Done
3 participants
You can’t perform that action at this time.