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

Fix and improve 2HWC catalog source models #2449

Merged
merged 6 commits into from Oct 15, 2019

Conversation

@QRemy
Copy link
Contributor

QRemy commented Oct 10, 2019

add spatial_models and sky_models for 2HAWC catalogue

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

cdeil left a comment

@QRemy - Thanks!

The sky_models method is not covered by a test? - please add it.

Now that we have a sky_models method, I think we can improve the API and tests here:

  • Make the spectral and spatial methods private, just offer the sky model as public API (since spectral and spatial are readily available from sky model and there's no performance concerns here)
  • Keep the current test cases, just rewrite them to go through the sky_models API
  • n_spectra isn't great -- it's just sometimes two models, for the second one both the spatial and spectral part is different? Maybe rename to n_models, or just remove it and users would do len(source.sky_models)?
  • From a quick look at the paper, it seems the spatial extensions are very unreliable? Or they aren't even extension measurements at all, but rather a correlation radius that isn't directly related to extension because of their large PSF? I guess returning a disk model is the best we can do, but maybe leave a comment on the docstring about this issue?

@QRemy - Thoughts?
(If you agree, please apply the changes in this PR)

@QRemy

This comment has been minimized.

Copy link
Contributor Author

QRemy commented Oct 11, 2019

They describe the source model as a point source or a disk of fixed radius of 0.5, 1, 2 degrees by default, but they try to re-define this radius based on residual map:

After the search, a residual map is generated and halo-like structures are visible around several sources modeled as point sources. This halo is used to define a tentative source radius for the secondary source model when fitting the energy spectrum (results presented in Table 3
of the next section). This radius should not be regarded as a definite measurement of the source extent but can nonetheless provide useful information on how much the
spectrum measurement depends on the source region definition.

I have added this comment in the sky_model docstring : The radius of the secondary source model is a rough estimate based on the residual excess above the point source model. This radius should not be regarded as a definite measurement of the source extent.

@QRemy QRemy force-pushed the QRemy:add_2HAWC_SpatialModel branch from 82c9270 to cf8f00b Oct 11, 2019
@adonath

This comment has been minimized.

Copy link
Member

adonath commented Oct 14, 2019

@QRemy and @cdeil For the hgps catalog we also have different spectral models and there we have chosen the pattern, that users can choose which model to return, see here: https://github.com/gammapy/gammapy/blob/master/gammapy/catalog/hess.py#L436

Maybe we just do the same here and add a which argument to spectral_model() and sky_model()? I guess the option would be maybe point-like and extended. If extended is not available, then an error is thrown...

@cdeil I would prefer not to hide .spectral_model() and .spatial_model(), because we don't do it for the other source catalogs either. Let's just go for consistency now and later re-think and change our pattern homogeneously, if we decide it's better...

@cdeil cdeil changed the title add spatial_models and sky_models for 2HAWC catalogue Improve 2HWC catalog source models Oct 14, 2019
@cdeil

This comment has been minimized.

Copy link
Member

cdeil commented Oct 14, 2019

@adonath - OK.
@QRemy - I'll finish this PR up tomorrow.

I noticed that the spectral index sign is filled incorrectly, I'll fix that in this PR also, changed the title to "Fix and improve" just now.

@cdeil cdeil changed the title Improve 2HWC catalog source models Fix and improve 2HWC catalog source models Oct 14, 2019
gammapy/catalog/hawc.py Outdated Show resolved Hide resolved
cdeil added 2 commits Oct 15, 2019
@cdeil

This comment has been minimized.

Copy link
Member

cdeil commented Oct 15, 2019

@QRemy - Thanks!

I'm merging this in now, and open a reminder issue that we should add some API to get and set position errors on spatial models. (At the moment the position error information isn't filled yet)

@cdeil
cdeil approved these changes Oct 15, 2019
@cdeil cdeil merged commit 6821570 into gammapy:master Oct 15, 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 #20191015.6 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.