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 parameters correlation matrix property #1918

merged 1 commit into from Nov 12, 2018


Copy link

@cdeil cdeil commented Nov 12, 2018

This PR moves the two-line method to compute the correlation matrix from the covariance matrix to the Parameters class, which currently holds the covariance matrix.

@adonath and I discussed this morning how to continue with the Parameter, Parameters and Model class. We don't have a full plan / PIG, but I really would like to improve and evolve what we have this week, and then only with some experience aim to get something solid and API stable in early 2019. Many things are unclear, but there are also other things that are just missing or badly done at the moment, e.g. we need an API for confidence intervals, @adonath needs this already this week to implement the flux point upper limits. So I strongly feel that we shouldn't default to do nothing in the face of not having a complete design.

The biggest question at the moment is where the parameter error information is stored and how it is exposed. Sherpa has it separately from the model here:
At the moment we are going in the direction of storing it in Parameters and to have parameter error information there optionally filled. We could add errn and errp attributes in Parameter or add them as vector in Parameters, or we could change course and rename ParameterstoErrorEstResultsand changeModel` back to only holding a Python list of parameters, or a very simple object without error information.

There's also many API questions how to expose error information on the Parameter and / or Parameters object.

@registerrier @adonath - Thoughts? Maybe we could have a call tomorrow or Wednesday latest to discuss a bit?

Is this PR OK or already controversial?

@cdeil cdeil added the cleanup label Nov 12, 2018
@cdeil cdeil added this to the 0.9 milestone Nov 12, 2018
@cdeil cdeil added this to In progress in gammapy.modeling via automation Nov 12, 2018
Copy link

@adonath adonath left a comment

As we already have Parameters.covariance this change is not controversial to me. I have no further comments.

Copy link

@registerrier registerrier left a comment

Having the covariance and correlation matrices living on Parameters seems the most natural thing to me. Having it on a Model would prove difficult when fitting inhomogeneous datasets simultaneously because they won't share exactly the same model.

@cdeil cdeil merged commit 52b2d07 into gammapy:master Nov 12, 2018
4 of 5 checks passed
4 of 5 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Codacy/PR Quality Review Up to standards. A positive pull request.
Scrutinizer Analysis: 2762 new issues, 1 updated code elements – Tests: passed
continuous-integration/appveyor/pr AppVeyor build succeeded
gammapy.gammapy Build #20181112.6 succeeded
gammapy.modeling automation moved this from In progress to Done Nov 12, 2018
@cdeil cdeil changed the title Add parameter correlation matrix from covar method Add parameters correlation matrix property Nov 23, 2018
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

3 participants