covmat-by-reference-bug by jacoterh · Pull Request #2463 · NNPDF/nnpdf

jacoterh · 2026-04-29T09:31:00Z

This PR addresses a bug in the construction of the total covariance matrix entering the fit:

nnpdf/validphys2/src/validphys/covmats.py

Lines 479 to 487 in 44a5ddf

    
           def dataset_inputs_t0_total_covmat(dataset_inputs_t0_exp_covmat, loaded_theory_covmat): 
        
               """ 
        
               Function to compute the covmat to be used for the sampling by make_replica and for the chi2 
        
               by fitting_data_dict. In this case the t0 prescription is used for the experimental covmat 
        
               and the multiplicative errors are included in it. Moreover, the theory covmat is added to experimental covmat. 
        
               """ 
        
               covmat = dataset_inputs_t0_exp_covmat 
        
               covmat += loaded_theory_covmat 
        
               return covmat

The line

nnpdf/validphys2/src/validphys/covmats.py

Line 485 in 44a5ddf

covmat = dataset_inputs_t0_exp_covmat

links covmat to dataset_inputs_t0_exp_covmat by reference, i.e. creates an alias, such that any subsequent changes to covmat also mutate the original object dataset_inputs_t0_exp_covmat .

We have checked that n3fit calls/resolves the provider dataset_inputs_t0_total_covmat twice before starting the fit, which means that dataset_inputs_t0_exp_covmat already includes the theory covmat after the first call such that the returned object that enters the fit after the second call is the experimental covmat plus twice the theory covmat.
We have run an agent and it turns out this behaviour shows up in covmats.py in 3 other places. This PR fixes this too. It also added a dedicated test to check whether things are safe now.
The filter utils function has also been modified for safety, because also here we modify the argument directly. I don't think this matters too much because the filter utils are not part of report engine, but better safe than sorry.

Report comparing the impact of the bug before and after the fix: https://vp.nnpdf.science/Gx02F1SqSaKSvTtOLhXFZg==/

…lity of inputs

achiefa · 2026-04-29T09:51:37Z

Hi @jacoterh, thanks for opening this PR. I'll have a look asap!

achiefa

It looks good to me. I just want to hear whether @scarlehoff has something to say on this matter.

achiefa · 2026-04-29T09:56:24Z

    """
-    for i in range(len(list_of_matrices)):
-        list_of_matrices[i] = np.array(list_of_matrices[i])
+    matrices = [np.array(matrix) for matrix in list_of_matrices]


I'm OK with this. I just want to check if this affects any of the datasets. regenerate_commondata should tell us if there's any difference.

achiefa · 2026-04-29T10:07:40Z

    assert ld.ndata == 1
    ld.systematic_errors(t0_predictions)
+
+


The test is fine per se, but I'd like to hear @scarlehoff's opinion. In my view, the test prevents us from repeating the same mistake, although now that we are aware of the issue it might seem a bit superfluous. Still, I think we should keep it as a reminder for the future and as a signal for new members.

Many of our tests are built on the blood of our previous mistakes. I'm happy with superflous tests.

jacoterh · 2026-04-29T10:21:02Z

Why do we get a failing test because of some eko incompatibility?

scarlehoff · 2026-04-29T12:22:06Z

It looks like the macos workers in github have changed (and have python 3.14 now? we should not be testing that)

achiefa · 2026-04-30T07:04:33Z

Does this mean that we can merge this? Is it possible to change the Python version un the MacOS runners?

scarlehoff · 2026-04-30T07:14:33Z

No, it means that we need to look over the github docs to understand what changed (or what are we doing wrong in the workflow for mac os)

…vironment explicitly

scarlehoff · 2026-04-30T10:23:03Z

        shell: bash -l {0}
        run: |
-          conda -n base install conda-build --yes
+          conda install -n base conda-build --yes


If I was both clever and handsome I would be too powerful for this world.
Anyway, this can be merged I guess.

(I've done this change because reading https://github.com/conda-incubator/setup-miniconda#important I thought macos, which has its own flavour of bash due to licence issues, might be messing with the environment activation, but I don't fully know why this fixes it other than "apple bad")

scarlehoff

Thanks for this. Please make sure to put here also the report you did comparing the fit with and without the bugfix.

achiefa

Also green light from me. Thanks @jacoterh !

jacoterh · 2026-04-30T12:43:23Z

Somehow I'm not authorised to merge? I see "Merging is blocked. You're not authorised to push to this branch". Weird.

scarlehoff · 2026-04-30T14:34:24Z

I wanted to put strong protections to master. Perhaps I exaggerated :__ I'll double check

jacoterh added 2 commits April 29, 2026 10:08

removing in-place covariance sums from covmats.py

47f1856

refactor matrix handling in utils.py and add tests to ensure immutabi…

434d573

…lity of inputs

jacoterh mentioned this pull request Apr 29, 2026

Pseudodata in diagonal basis and saving rotation #2455

Open

2 tasks

jacoterh requested a review from achiefa April 29, 2026 09:49

jacoterh changed the title ~~comat-by-reference-bug~~ covmat-by-reference-bug Apr 29, 2026

achiefa added the bug Something isn't working label Apr 29, 2026

achiefa reviewed Apr 29, 2026

View reviewed changes

achiefa added the regenerate-data label Apr 29, 2026

scarlehoff mentioned this pull request Apr 30, 2026

Improve reproducibility of regression tests (and add one with a theory covmat) #2464

Open

scarlehoff added 2 commits April 30, 2026 10:01

install pip explicitly when using conda, install build in the base en…

80e6b81

…vironment explicitly

wrong order of arguments

472d92c

scarlehoff reviewed Apr 30, 2026

View reviewed changes

scarlehoff approved these changes Apr 30, 2026

View reviewed changes

Comment thread nnpdf_data/nnpdf_data/filter_utils/utils.py Outdated

restoring utils.py

74cf8ed

achiefa approved these changes Apr 30, 2026

View reviewed changes

scarlehoff merged commit bb30db4 into master Apr 30, 2026
12 checks passed

scarlehoff deleted the comat-by-reference-bug branch April 30, 2026 14:13

	def dataset_inputs_t0_total_covmat(dataset_inputs_t0_exp_covmat, loaded_theory_covmat):
	"""
	Function to compute the covmat to be used for the sampling by make_replica and for the chi2
	by fitting_data_dict. In this case the t0 prescription is used for the experimental covmat
	and the multiplicative errors are included in it. Moreover, the theory covmat is added to experimental covmat.
	"""
	covmat = dataset_inputs_t0_exp_covmat
	covmat += loaded_theory_covmat
	return covmat

Conversation

jacoterh commented Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achiefa commented Apr 29, 2026

Uh oh!

achiefa left a comment

Choose a reason for hiding this comment

Uh oh!

achiefa Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

achiefa Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

scarlehoff Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

jacoterh commented Apr 29, 2026

Uh oh!

scarlehoff commented Apr 29, 2026

Uh oh!

achiefa commented Apr 30, 2026

Uh oh!

scarlehoff commented Apr 30, 2026

Uh oh!

scarlehoff Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

scarlehoff left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

achiefa left a comment

Choose a reason for hiding this comment

Uh oh!

jacoterh commented Apr 30, 2026

Uh oh!

Uh oh!

scarlehoff commented Apr 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jacoterh commented Apr 29, 2026 •

edited

Loading

scarlehoff left a comment •

edited

Loading