Skip to content

Commit

Permalink
Propagate covariance matrix to the spatial and spectral nodel in SkyM…
Browse files Browse the repository at this point in the history
…odel
  • Loading branch information
adonath committed Sep 21, 2018
1 parent 6cb0919 commit b541b45
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 0 deletions.
8 changes: 8 additions & 0 deletions gammapy/cube/models.py
Expand Up @@ -152,11 +152,19 @@ def __init__(self, spatial_model, spectral_model, name="SkyModel"):
@property
def spatial_model(self):
"""`~gammapy.image.models.SkySpatialModel`"""
# propagate sub-covariance
if self.parameters.covariance is not None:
idx = len(self._spatial_model.parameters.parameters)
self._spatial_model.parameters.covariance = self.parameters.covariance[:idx, :idx]

This comment has been minimized.

Copy link
@cdeil

cdeil Sep 21, 2018

Contributor

@adonath - I'm not sure if this is a good idea.
It is very uncommon in Python to have a property access change internal state.
I think users will be confused to sometimes have covar set on the spatial and spectral part, depending on whether they accessed that model component directly or from the total sky model.
(and even to have the sub-model state change just becomes somewhere someone did a property access on the total model).

I can see how it is desirable to have this back propagation, but I doubt this is the way to go.

This comment has been minimized.

Copy link
@adonath

adonath Sep 22, 2018

Author Member

I'm not sure whether it was a good idea either...but for now it works:-) Note that the model typically only has a covariance defined, when it has been modified by MapFit, where a deep copy is returned anyway. So the input models are not modified. I agree that it is very uncommon to modify the internal state on property access. So one could make a copy again here...I'll leave a TODO comment in the code now and open an issue, that we do not forget about this...

return self._spatial_model

@property
def spectral_model(self):
"""`~gammapy.spectrum.models.SpectralModel`"""
# propagate sub-covariance
if self.parameters.covariance is not None:
idx = len(self._spatial_model.parameters.parameters)
self._spectral_model.parameters.covariance = self.parameters.covariance[idx:, idx:]
return self._spectral_model

@property
Expand Down
3 changes: 3 additions & 0 deletions gammapy/cube/tests/test_fit.py
Expand Up @@ -125,3 +125,6 @@ def test_cube_fit(sky_model, counts, exposure, psf, background, mask, edisp):

assert_allclose(pars["amplitude"].value, 1e-11, rtol=1e-2)
assert_allclose(pars.error("amplitude"), 3.936e-13, rtol=1e-2)

assert result.model.spectral_model.covariance is not None
assert result.model.spatial_model.covariance is not None
1 change: 1 addition & 0 deletions gammapy/utils/fitting/parameter.py
Expand Up @@ -309,6 +309,7 @@ def covariance_to_table(self):
for idx, par in enumerate(self.parameters):
vals = self.covariance[idx]
table[par.name] = vals
table[par.name].format = ".3e"
return table

@property
Expand Down

0 comments on commit b541b45

Please sign in to comment.