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 SpatialModel.position_error and SpatialModel.to_region #2491

Merged
merged 26 commits into from Oct 30, 2019

Conversation

@QRemy
Copy link
Contributor

@QRemy QRemy commented Oct 27, 2019

This PR implements #2460 and #2422 .

  • add SpatialModel.position_error attribute as DiskSpatialModel (or None)
  • set position_error for supported catalogs
  • add SpatialModel.getcontour() method, return a matplotlib path object and a ds9 .reg string
  • add models_to_reg() in serialize in order to export a list of SpatialModel to ds9 .reg file,
    so is can be used to generate position error and extended sources contours in .reg,
    Use polygons to export contours as in ds9, except for DiskSpatialModel where an ellipse is used (whatever the contour threshold selected).

For example:
4fgl_extended_20percentmax.reg.txt
4fgl_extended_50percentmax.reg.txt
4fgl_position_error.reg.txt

@QRemy QRemy requested a review from cdeil Oct 27, 2019
@QRemy QRemy added the feature label Oct 27, 2019
@adonath adonath added this to the 0.15 milestone Oct 28, 2019
@cdeil
Copy link
Member

@cdeil cdeil commented Oct 28, 2019

@QRemy - Please use sky region objects in the API (see #2460 and #2422 and https://astropy-regions.readthedocs.io), both for the position error, and for the model extent.

I think spatial_model.to_region should just return the appropriate region object, i.e. often point or circle or ellipse. E.g. rasterising and contouring a point or small source won't yield good results, users will have to fiddle with binsz and other paramters too much. My suggestion would be to delete the contouring code completely. We could add it in the future, either on TemplateSpatialModel, but probably better as a method on gammapy.maps.WCSNdMap or in astropy-regions.
Note that there is contouring code already available in http://docs.astropy.org/en/stable/visualization/wcsaxes/images_contours.html - that could be used for now, and we could document or add an API to transform back and forth between MPL contour points and polygon region objects.

Concerning the code how to transform back and forth between covariance matrix and ellipse parameters, see https://docs.astropy.org/en/stable/_modules/astropy/modeling/functional_models.html#Gaussian2D or https://multinorm.readthedocs.io/en/latest/_modules/multinorm.html#MultiNorm.error_ellipse
If you figure out the other direction (ellipse to covariance), let me know and I'll add it as a helper classmethod also on MultiNorm (and then soon move that functionality to Gammapy to the Parameters class).

@QRemy QRemy force-pushed the spatial_model_contours branch from eddf9b2 to 28ed4a0 Oct 28, 2019
@cdeil cdeil added this to In progress in gammapy.catalog Oct 29, 2019
@QRemy QRemy force-pushed the spatial_model_contours branch from 45f6f4c to 8861f00 Oct 29, 2019
@QRemy QRemy changed the title Add SpatialModel.position_error and SpatialModel_getcontour() Add SpatialModel.position_error and SpatialModel.to_region Oct 29, 2019
Copy link
Member

@cdeil cdeil left a comment

@QRemy - Looks pretty good now, thanks!

Some more suggestions inline.

I only had a quick look. You still have the model._position_error and set that instead of going through covar? This is already a 300 line diff, so OK to merge in like this, and to leave that to a future PR.

gammapy/modeling/serialize.py Outdated Show resolved Hide resolved
gammapy/modeling/tests/test_serialize_yaml.py Outdated Show resolved Hide resolved
gammapy/modeling/models/spatial.py Show resolved Hide resolved
gammapy/modeling/models/spatial.py Outdated Show resolved Hide resolved
gammapy/modeling/models/spatial.py Outdated Show resolved Hide resolved
gammapy/modeling/models/spatial.py Show resolved Hide resolved
gammapy/catalog/hess.py Outdated Show resolved Hide resolved
@QRemy
Copy link
Contributor Author

@QRemy QRemy commented Oct 29, 2019

If the errors are given in the catalog, like in the HGPS, I go through covar (but only the diagonal terms are set), otherwise I set directly the position error given in the catalog through this _position_error.

@QRemy QRemy force-pushed the spatial_model_contours branch from 3c8fed5 to 2c164f9 Oct 29, 2019
Copy link
Member

@cdeil cdeil left a comment

@QRemy - See comments inline.

gammapy/modeling/models/spatial.py Outdated Show resolved Hide resolved
gammapy/modeling/models/spatial.py Outdated Show resolved Hide resolved
gammapy/modeling/models/spatial.py Outdated Show resolved Hide resolved
gammapy/catalog/gammacat.py Outdated Show resolved Hide resolved
gammapy/catalog/fermi.py Outdated Show resolved Hide resolved
gammapy/modeling/serialize.py Outdated Show resolved Hide resolved
@QRemy QRemy force-pushed the spatial_model_contours branch from b5ef9d7 to 6d84e7c Oct 30, 2019
cdeil
cdeil approved these changes Oct 30, 2019
Copy link
Member

@cdeil cdeil left a comment

@QRemy - Thanks!

I'm merging this in now.

There's some things left to do - especially going covar <-> error ellipse.
Let's leave #2460 open for now.

@cdeil cdeil merged commit 6d9c775 into gammapy:master Oct 30, 2019
6 of 8 checks passed
@cdeil cdeil moved this from In progress to Done in gammapy.catalog Nov 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants