Repository navigation
Fix covariance weighting across ensembles and Covobs sources - #301
Open
s-kuberski wants to merge 2 commits into
Open
s-kuberski wants to merge 2 commits into
s-kuberski wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Same-name Covobs matrices need consistency validation, alongside the documented performance and coverage concerns.
Review effort: Balanced
Findings: 1
Open (4)
What changed in this PR
Fixes covariance weighting across Monte Carlo ensembles, unequal configuration support, and shared Covobs sources.
Changes:
- Computes and rescales zero-lag covariance per ensemble.
- Adds
Covobscovariance in variance units. - Expands regression coverage for mixed sources and sample support.
| File | Description |
|---|---|
pyerrors/obs.py |
Reworks covariance construction and documentation. |
tests/obs_test.py |
Adds and updates covariance regression tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Collaborator
Author
|
I've addressed all of copilot's suggestions. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



This pull request fixes a bug in the current implementation of the computation of a covariance matrix. It keeps intact the current behavior of the construction regarding the treatment of autocorrelation.
We had written
in the docstring of the function, but I think that this was not sufficient, given that the covariance was basically wrong when having multiple ensembles.
Problem
covariance()currently combines pairwise zero-lag correlations across ensembles before rescaling them by the total errors. This loses the relative size of the ensemble contributions. It also mixesCovobscovariances, which have units of variance, into that correlation calculation.There is a related issue when observables have support on different configurations of the same ensemble. The current pairwise normalization uses only configurations shared by each pair. Those pairwise correlations need not form a positive-semidefinite matrix and do not describe the covariance of averages computed from the full available samples. I always had in mind that the old choice was better, because the correlation was estimated only on the common set and thus nicely resolved. However, the violation of positive semidefiniteness is something that we don't want in this function. This is an edge case anyways...
Both effects can be seen with small examples:
Fix
Construct the zero-lag covariance matrix separately for each ensemble. Its$(i,j)$ element sums products of fluctuations on configurations shared by observables $i$ and $j$ , while its normalization uses each observable’s full sample count on that ensemble. This gives a single zero-lag matrix for the ensemble, rather than separately normalized correlations for each pair.
Normalize that matrix to obtain$R_e^{(0)}$ , then rescale it with the ensemble-specific gamma-method errors:
$$C_{\mathrm{MC}}=\sum_e D_e R_e^{(0)}D_e.$$ $D_e$ is diagonal, with entries equal to the contribution of ensemble $e$ to each observable’s error. Add shared
$$C=C_{\mathrm{MC}}+\sum_c G_c\Sigma_cG_c^{\mathsf T},$$ $\Sigma_c$ is the input covariance and $G_c$ contains the propagated gradients.
Here
Covobsinputs in variance units:where
Each zero-lag ensemble matrix is a Gram matrix when fluctuations are placed on the common configuration space, with zero entries where an observable was not measured. Normalization and diagonal rescaling preserve positive semidefiniteness; so does adding the
Covobscovariance matrices.The routine retains its existing approximation for cross-autocorrelations: it uses zero-lag correlations and the gamma-method errors of the individual observables. It does not estimate correlations at nonzero lags. For one ensemble and one common configuration set, the result agrees with the previous construction. Results can change for unequal configuration support, differing replica contributions, or multiple error sources, as illustrated above.
The tests cover these cases, the covariance of identical observables, and linear propagation across independent ensembles and shared
Covobsinputs.Future
A more detailed estimator of cross-autocorrelations should be considered separately. I have seen cases where autocorrelation in the cross-covariance is not negligible and there might be better ways to include this effect without ending up with non-positive matrices all of the time. It could be an idea to have a switch in order to support several different constructions.