Skip to content

Fix random fails in test_commondata#2474

Merged
scarlehoff merged 3 commits into
masterfrom
fix_art_unc
Jun 3, 2026
Merged

Fix random fails in test_commondata#2474
scarlehoff merged 3 commits into
masterfrom
fix_art_unc

Conversation

@andrpie

@andrpie andrpie commented May 19, 2026

Copy link
Copy Markdown
Contributor

As per #2444, test_commondata sometimes randomly fails due to a worker with a different version of some package.

Example list of failing datasets ATLAS_2JET_7TEV_R06/uncertainties.yaml ATLAS_2JET_7TEV_R06/uncertainties_stronger.yaml ATLAS_2JET_7TEV_R06/uncertainties_weaker.yaml ATLAS_TTBAR_13TEV_HADR_DIF/uncertainties_dSig_dyttBar_norm.yaml ATLAS_TTBAR_13TEV_LJ_DIF/uncertainties_dSig_dmttBar_interspectra.yaml ATLAS_TTBAR_13TEV_LJ_DIF/uncertainties_dSig_dmttBar_norm.yaml ATLAS_TTBAR_13TEV_LJ_DIF/uncertainties_dSig_dpTt_interspectra.yaml ATLAS_TTBAR_13TEV_LJ_DIF/uncertainties_dSig_dyt_interspectra.yaml ATLAS_TTBAR_13TEV_LJ_DIF/uncertainties_dSig_dyttBar_interspectra.yaml ATLAS_TTBAR_8TEV_2L_DIF/uncertainties_dSig_dyttBar_norm.yaml ATLAS_TTBAR_8TEV_LJ_DIF/uncertainties_dSig_dmttBar.yaml ATLAS_TTBAR_8TEV_LJ_DIF/uncertainties_dSig_dmttBar_norm.yaml ATLAS_TTBAR_8TEV_LJ_DIF/uncertainties_dSig_dpTt.yaml ATLAS_TTBAR_8TEV_LJ_DIF/uncertainties_dSig_dpTt_norm.yaml ATLAS_TTBAR_8TEV_LJ_DIF/uncertainties_dSig_dyt.yaml ATLAS_TTBAR_8TEV_LJ_DIF/uncertainties_dSig_dyt_norm.yaml ATLAS_TTBAR_8TEV_LJ_DIF/uncertainties_dSig_dyttBar.yaml ATLAS_TTBAR_8TEV_LJ_DIF/uncertainties_dSig_dyttBar_norm.yaml CMS_1JET_8TEV/uncertainties.yaml CMS_2JET_7TEV/uncertainties.yaml CMS_2JET_8TEV/uncertainties.yaml CMS_TTBAR_13TEV_LJ_DIF/uncertainties_d2Sig_dyttBar_dmttBar.yaml CMS_TTBAR_13TEV_LJ_DIF/uncertainties_d2Sig_dyttBar_dmttBar_norm.yaml CMS_TTBAR_13TEV_LJ_DIF/uncertainties_dSig_dmttBar.yaml CMS_TTBAR_13TEV_LJ_DIF/uncertainties_dSig_dmttBar_norm.yaml CMS_TTBAR_13TEV_LJ_DIF/uncertainties_dSig_dpTt_norm.yaml CMS_TTBAR_8TEV_LJ_DIF/uncertainties_dSig_dyttBar_norm.yaml CMS_WPWM_13TEV_ETA/uncertainties_WM.yaml CMS_WPWM_13TEV_ETA/uncertainties_WP.yaml CMS_WPWM_8TEV_MUON/uncertainties.yaml CMS_Z0J_8TEV/uncertainties_PT-Y.yaml CMS_Z0J_8TEV/uncertainties_PT-Y_sys_10.yaml CMS_Z0_13TEV/uncertainties_AFB.yaml CMS_Z0_7TEV_DIMUON/uncertainties.yaml H1_1JET_319GEV_290PB-1_DIF/uncertainties_norm.yaml H1_1JET_319GEV_290PB-1_DIF/uncertainties_wo-lumi.yaml H1_1JET_319GEV_351PB-1_DIF/uncertainties_wo-lumi.yaml H1_2JET_319GEV_290PB-1_DIF/uncertainties_norm.yaml H1_2JET_319GEV_290PB-1_DIF/uncertainties_wo-lumi.yaml H1_2JET_319GEV_351PB-1_DIF/uncertainties_wo-lumi.yaml HERMES_NC_7GEV_ED/uncertainties.yaml LHCB_DY_8TEV_MUON/uncertainties.yaml LHCB_WPWM_8TEV_MUON/uncertainties.yaml LHCB_Z0J_13TEV_2022/uncertainties_dimuon_pT.yaml LHCB_Z0_13TEV/uncertainties_dielectron.yaml LHCB_Z0_13TEV_2022/uncertainties_dimuon_y.yaml LHCB_Z0_8TEV_DIELECTRON/uncertainties.yaml LHCB_Z0_8TEV_MUON/uncertainties.yaml STAR-2009_1JET_200GEV/uncertainties_CC.yaml STAR-2009_1JET_200GEV/uncertainties_CF.yaml STAR-2009_2JET_200GEV/uncertainties_A.yaml STAR-2009_2JET_200GEV/uncertainties_B.yaml STAR-2009_2JET_200GEV/uncertainties_C.yaml STAR-2009_2JET_200GEV_MIDRAP/uncertainties_OS.yaml STAR-2009_2JET_200GEV_MIDRAP/uncertainties_SS.yaml STAR-2012_1JET_510GEV/uncertainties.yaml STAR-2012_2JET_510GEV/uncertainties_A.yaml STAR-2012_2JET_510GEV/uncertainties_B.yaml STAR-2012_2JET_510GEV/uncertainties_C.yaml STAR-2012_2JET_510GEV/uncertainties_D.yaml STAR-2013_1JET_510GEV/uncertainties.yaml STAR-2013_2JET_510GEV/uncertainties_A.yaml STAR-2013_2JET_510GEV/uncertainties_B.yaml STAR-2013_2JET_510GEV/uncertainties_C.yaml STAR-2013_2JET_510GEV/uncertainties_D.yaml STAR-2015_2JET_200GEV_MIDRAP/uncertainties_SS.yaml

All of these uncertainty files are built using covmat_to_artunc or decompose_covmat. These in turn rely on `numpy.linalg.eig' which leaves the evals and evecs unordered, most likely causing random fails.

Changes

This PR introduces ordering of eigenvalues and eigenvectors in util functions covmat_to_artunc and decompose_covmat. It is as follows: eigenvalues are given in decreasing order, and the first non-zero entry of each eigenvector is positive.
I also fixed filters that used numpy.linalg.eig rather than our utils.

Potential to do:

  • check that the covmats for these datasets are unaffected by the changes
  • are both covmat_to_artunc and decompose_covmat necessary? They seem to do the same thing.
  • why is covmat_to_artunc defined twice (in utils.py and correlations.py)?
  • edge case: what if eigenvalues have multiplicities greater than 1?

@scarlehoff

scarlehoff commented May 19, 2026

Copy link
Copy Markdown
Member

check that the covmats for these datasets are unaffected by the changes

Yes, please do! This is important

are both covmat_to_artunc and decompose_covmat necessary? They seem to do the same thing.

If they seem to do the same thing (and given that you will most likely have a script to check whether the covmat is the same given the point above) just remove one and check.

why is covmat_to_artunc defined twice (in utils.py and correlations.py)?

Because we are evil and deserve divine punishment. Please remove one for salvation.

edge case: what if eigenvalues have multiplicities greater than 1?

How often does it happen? My naive guess is that float precision + ordering might take care of this edge case as well (up to updates of numpy/scipy that might change the order of operations)

@felixhekhorn felixhekhorn removed their request for review May 19, 2026 12:06
webwizardg99

This comment was marked as spam.

Sign in to view

@scarlehoff

scarlehoff commented May 25, 2026

Copy link
Copy Markdown
Member

What is the status of this?

https://github.com/NNPDF/nnpdf/actions/runs/26394014558/job/77690240148

The ratio of fail/non fail is getting close to 50% now 😢

@andrpie

andrpie commented May 26, 2026

Copy link
Copy Markdown
Contributor Author

I have been very busy with my annual report the last few days, I'll get back to this PR soon!

@scarlehoff

scarlehoff commented Jun 1, 2026

Copy link
Copy Markdown
Member

Do you think you'll be able to tackle this in the coming days?

@andrpie

andrpie commented Jun 1, 2026

Copy link
Copy Markdown
Contributor Author

Hi @scarlehoff I've found some time to work on this today. This is the current status:

  • I've compared covmat(s) from before changes were made to filters with covmat(s) obtained upon regenerating commondata with the new implementation of covmat_to_artunc. Perfect agreement is found, confirming that ordering of eigenvectors doesn't impact the phenomenology (phew).
  • I've deleted the unnecessary instances of covmat_to_artunc, so that now all the functions that use this function, call to the same place

In this sense, this PR can be considered finalised -- filters now shouldn't be subject to random permutations of artificial uncertainties.

The only thing hanging is that there are three utils functions that (I've confirmed) do the same thing: covmat_to_artunc, compute_covmat, and decompose_covmat. I'll work on this extra clean-up in the coming days.

@scarlehoff scarlehoff mentioned this pull request Jun 2, 2026
Open
@scarlehoff

scarlehoff commented Jun 2, 2026

Copy link
Copy Markdown
Member

I'll rebase this again (in part to force the tests to rerun) and then I think it can be merged. Thanks!

@scarlehoff scarlehoff merged commit bbebfa8 into master Jun 3, 2026
48 of 52 checks passed
@scarlehoff scarlehoff deleted the fix_art_unc branch June 3, 2026 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@scarlehoff scarlehoff scarlehoff approved these changes
+1 more reviewer
@webwizardg99 webwizardg99 webwizardg99 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

regenerate-data

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@andrpie @scarlehoff @webwizardg99