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

Improve analysis 3D tutorial #1821

Merged
merged 3 commits into from Sep 21, 2018
Merged

Conversation

@adonath
Copy link
Member

@adonath adonath commented Sep 21, 2018

This PR improves the 3D analysis tutorial, by adding a few more explanations and a residual map at the end.

@adonath adonath self-assigned this Sep 21, 2018
@adonath adonath added the docs label Sep 21, 2018
@adonath adonath added this to To Do in DOCUMENTATION via automation Sep 21, 2018
@adonath adonath added this to the 0.8 milestone Sep 21, 2018
@adonath adonath merged commit 045980e into gammapy:master Sep 21, 2018
0 of 3 checks passed
0 of 3 checks passed
Scrutinizer Created
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
DOCUMENTATION automation moved this from To Do to Done Sep 21, 2018
@cdeil

This comment has been minimized.

Copy link
Member

@cdeil cdeil commented on gammapy/cube/models.py in b541b45 Sep 21, 2018

@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
Member Author

@adonath adonath replied Sep 22, 2018

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
DOCUMENTATION
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.