-
Notifications
You must be signed in to change notification settings - Fork 6
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
Hessian PDF Covariance Matrix for theory predictions #1743
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also add a test, an equivalent of
return API.groups_covmat_no_table(use_pdferr=True, **data_internal_cuts_config) |
Also, could you run black
on your code? (now it should be completely safe to do so :D)
@@ -677,12 +677,15 @@ def groups_corrmat(groups_covmat): | |||
return mat | |||
|
|||
|
|||
@check_pdf_is_montecarlo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should still be some check, for the error member types we don't support.
validphys2/src/validphys/covmats.py
Outdated
X = hessian_eigenvectors - central_predictions.reshape((central_predictions.shape[0],1)) | ||
# need to rescale the Hessian eigenvectors in case the eigenvector confidence interval is not 68% | ||
X = X / rescale_fac | ||
pdf_cov = np.einsum("ij,kj->ik",X, X) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be X@X.T
which is clearer, if less efficient.
validphys2/src/validphys/covmats.py
Outdated
|
||
# need to subtract the central set which is not the same as the average of the | ||
# Hessian eigenvectors. | ||
X = hessian_eigenvectors - central_predictions.reshape((central_predictions.shape[0],1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be something like central_prediction[:, np.newaxis]
(or central_prediction[:, None]
)
Hi @Zaharid , @scarlehoff not sure of why |
You need to rebase / merge master. I think it is due to numpy 1.24.X, you probably have a different (older) version in your computer. |
0dd394b
to
7efa7fd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good afaict.
validphys2/src/validphys/checks.py
Outdated
@@ -43,6 +43,16 @@ def check_pdf_is_montecarlo(ns, **kwargs): | |||
raise CheckError(f"Error type of PDF {pdf} must be 'replicas' and not {etype}") | |||
|
|||
|
|||
@make_check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be using make_argcheck
afaict.
validphys2/src/validphys/checks.py
Outdated
def check_pdf_is_montecarlo_or_symmhessian(ns, **kwargs): | ||
pdf = ns['pdf'] | ||
etype = pdf.error_type | ||
if (etype != 'replicas') and (etype != 'symmhessian'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could be check(etype in {'replicas', 'symhessian'}, f"Error type of PDF {pdf} must be either 'replicas' or 'symmhessian' and not {etype}")
I wonder if it makes sense to move this kind of functionality to the stats classes themselves (clue being that we are using plenty of internal methods). Would be a different pr anyway. |
The idea of this PR is to have the
covmats.pdferr_plus_covmat
function to take into account for the case in which the PDF haserror_type = 'symmhessian'