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

Pk1d covariance improvements #1003

Merged
merged 20 commits into from
Jun 1, 2023
Merged

Pk1d covariance improvements #1003

merged 20 commits into from
Jun 1, 2023

Conversation

corentinravoux
Copy link
Contributor

Improvements of the covariance matrix calculation.
In particular, this branch can now compute the covariance matrix with the fit_snr method used to compute mean P1D.
This branch will be used to build the first covariance matrix of the EDR+M2 measurement.

I suggest using this branch to make all the improvements discussed in previous covariance matrix calculations. Do not hesitate to push on this branch if you have any changes.

I perform tests on Ohio EDR-mocks to assess the quality of the covariance matrix. In particular, I compared the error computed in the average calculation (standard deviation) with the diagonal of the covariance matrix:

  • For the no_weights method, the diagonal is exactly the same by construction. I also verified on paper that the two estimates are theoretically equal.

  • For the fit_snr method, the error on P1D is not computed with an estimator but with a fitting. It means that theoretically, with the covariance matrix calculation implemented, the error and the diagonal of the matrix are not equal. You will see an attachment showing the difference between those two. I choose to renormalize the diagonal by the error to have something consistent between the error and the diagonal covariance. This choice can be discussed (for example, do we extend this normalization to the full covariance matrix?).

I will also attach the small theoretical calculations I made.

@corentinravoux
Copy link
Contributor Author

Here are the tests I made on the Ohio-mocks:
tests_covariance_branch

@corentinravoux
Copy link
Contributor Author

Here are my calculations, I hope it is readable:
Covariance matrix.pdf

@Waelthus
Copy link
Contributor

Ok, from your write up I think you're doing the right thing and keep correlations the same when doing covariance rescaling. I wouldn't just scale the diagonals...

Also the non-weighted (i.e. all modes have same weight, forests have different weight depending on how many modes fall in a bin for that particular forest) looks like it's right.
Note that the calculation could be split by treating each forest seperately (performing the sums over ik, ik2 for the Cov, Np, mean contributions for that forest) allowing to later perform the sum over forests for those individual forest properties...

It's weird that the weighted covariance wouldn't reproduce the variance (which it should by definition even in the weighted case). Are you sure there's no bug in the calculation? I think normally one would multiply each occurance of P with a weight (for simplicity I'm using sum w=1, here), i.e. var = sum P2 w2, mean = sum P w, Cij = sum wi Pi wj Pj. But you've put additional sqrts for your weights in the cov case (and just insert the final solution for sigma=var**0.5...)

I'll have a closer look later...

weight2 = 1 / selected_variance_estimated[ipk2]
covariance_array[index] = covariance_array[index] + selected_pk[
ipk
] * selected_pk[ipk2] * np.sqrt(weight * weight2)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is the weighting done with a sqrt(weight *weight2) instead of with the weights directly, see other comment

@Waelthus
Copy link
Contributor

Waelthus commented Jun 1, 2023

I'm merging this anyway to allow usage of the existing improvements. @corentinravoux: we'll need to think about exact definitions again at a later time and update the estimators accordingly. The need to rescale does look very weird to me...

@Waelthus Waelthus merged commit 5028f9c into master Jun 1, 2023
@Waelthus Waelthus deleted the pk1d_covariance_improvements branch June 2, 2023 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants