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 SkyModel.position and frame attribute #2066

merged 13 commits into from Mar 4, 2019


Copy link

@adonath adonath commented Mar 3, 2019

This PR contains the following changes:

  • Add a .frame and .position attribute to SkySpatialModel and SkyModel
  • Add frame handling to MapEvaluator, such that the spatial model is always evaluated with the right coordinate system defined by the model.
  • Change the default wrapping angle of the lon_0 parameter of spatial models to 180 deg. I noticed that in the analysis_3d.ipynb notebook the fit did not converge, when the start value was set to to lon_0 = -0.05, because by default it was wrapped at 360 deg. The optimiser cannot deal with this jump.
  • Introduce default min and max values for lon_0 and lat_0 to constrain the models position to the sphere. With the failing fit I noticed that the optimiser tried values lat_0 > 90 or lat_0 < -90.
  • Introduce a BackgroundModel.from_skymodel() method, which allows to create a background model from a SkyDiffuseCube or SkyDiffuseConstant model. The motivation is that this vastly improves the performance of the fit (~factor 20-30), because the model is not interpolated and convolved anymore in every fit iteration, but just scaled and tilted.
  • Introduce a BackgroundModels class to handle the sum of multiple background models. The use case here was the fermi_lat.ipynb notebook, where two independent components for the galactic and constant diffuse model are used.
@adonath adonath self-assigned this Mar 3, 2019
@adonath adonath added the feature label Mar 3, 2019
@adonath adonath added this to In progress in gammapy.modeling via automation Mar 3, 2019
@adonath adonath added this to the 0.11 milestone Mar 3, 2019
@adonath adonath force-pushed the skymodel_position_frame branch from b261d2e to f5a51f8 Mar 3, 2019
@adonath adonath requested a review from registerrier Mar 4, 2019
Copy link

@registerrier registerrier left a comment

Thanks @adonath .
While I understand the need to add a coordinate system to the SkyModel, I don't really see the point of adding some IRF convolved SkyModel to BackgroundModel. What is the driver to have this here?

@@ -395,3 +426,77 @@ def evaluate(self):
tilt_factor = np.power((self.energy_center / reference).to(""), -tilt)
back_values = norm * * tilt_factor.value

def from_skymodel(cls, skymodel, exposure, edisp=None, psf=None, **kwargs):
Copy link

@registerrier registerrier Mar 4, 2019

I am not sure I understand the use case here. Why do you introduce this mix between BackgroundModel and SkyModel?

Copy link
Member Author

@adonath adonath Mar 4, 2019

This is definitely something we have to re-discuss. Currently we support a constant or galactic diffuse background component via SkyDiffuseCube or SkyDiffuseConstant. Unfortunately this is super inefficient with the MapEvaluator, because the model is re-evaluated (i.e interpolated, PSF-convolved and energy migrated) in every iteration of the fit. The effect here is not minor, it slows down the fitting by a factor of ~50 depending on the image size and becomes the bottleneck of the model evalutaion. Currently we have the fermi_lat.ipynb and analysis_3d.ipynb notebook which are affected by this. The general question is probably more, how to handle galactic diffuse emission. Is it handled as a background or source component? I guess it depends on your analysis interest. In the use cases listed above its probably part of the background...

The solution here is of course to precompute the models on the analysis grid and then just scale and tilt them. Which is exactly, what the BackgroundModel class does. So this is the idea behind the .from_skymodel() method.

I guess the better solution would be to have an internal caching of the SkyDiffuseCube or SkyDiffuseConstant models, but then we need some mechanism to couple it with the spectral parameters. I'm happy for any better idea / suggestion how to handle this...

Copy link
Member Author

@adonath adonath Mar 4, 2019

A compromise for now could be to just use the MapEvaluator directly in the notebooks and pre-computed the diffuse background model and adding a comment that it's needed because of performance. Then I would delete the .from_skymodel() method again.

Copy link

@registerrier registerrier Mar 4, 2019

OK I see the point now.

Technically, changing the spectral index on a SkyModel and applying a tilt on the BackgroundModel should not be identical since the edisp should redistribute in some non-trivial manner.

That said I have no clear idea on how to tell the MapEvaluator to cache the result of the evaluation of apply_psf when the model is a SkyDiffuseCube or SkyDiffuseMap.

I guess the .from_skymodel() method is OK for now, as there might be other uses cases for it.

@adonath adonath force-pushed the skymodel_position_frame branch from 4a00124 to 2cd0ca3 Mar 4, 2019
@adonath adonath merged commit 0298bd7 into gammapy:master Mar 4, 2019
4 of 8 checks passed
gammapy.modeling automation moved this from In progress to Done Mar 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants