Crash when combining point prescriptions with a user covmat

[achiefa]

I was tryin to add the user covmat generated for the PC uncertainties on top of the point prescription. vp-setupfit crashed during reportengine graph resolution:

File ".../reportengine/utils.py", line 56, in saturate
    kwargs = {name: d[name] for name, param in s.parameters.items()
              ~^^^^^^
KeyError: 'group_dataset_inputs_by_metadata'

This goes away if I remove the user covmat from the runcard.

Possible explanation

When both covmats are provided, total_theory_covmat is the provider which carries a check: @check_fit_dataset_order_matches_grouped and the related @make_argcheck. Now, my understanding is that group_dataset_inputs_by_metadata is a production rule that is not resolved in the namespec at the time of the argcheck, which datacuts::theory::theorycovmatconfig (argcheck does not trigger produce_ rules). I think this chain used to be correct by accident before @jacoterh and I modified this part of the code for the bug with the diagonal basis. Before the *_fitting providers (including total_theory_covmat_fitting) used

procs_index_matched(groups_index, procs_index),

taking groups_index and forcing the production of group_dataset_inputs_by_metadata. Now procs_index_matched is replaced by data_input_matched_procs_index(data_input, procs_index), which doesn't call groups_index.

At the same time, this was obselete and wasn't captured by the tests. In total_theory_covmat_fitting, the covmat is already reindexed using data_input_matched_procs_index. Therefore, it can be removed.

[Copilot]

Pull request overview

Fixes a vp-setupfit crash during reportengine graph resolution when both point-prescription theory covmats and a user-provided covmat are enabled, by removing an argcheck dependency that required an unresolved production rule (group_dataset_inputs_by_metadata) at argcheck time.

Changes:

• Remove check_fit_dataset_order_matches_grouped argcheck from theory covariance utilities.
• Stop applying that argcheck to total_theory_covmat, eliminating the KeyError during dependency resolution.
• Remove the now-unused procs_index_matched helper.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
validphys2/src/validphys/theorycovariance/theorycovarianceutils.py	Removes the argcheck function that depended on group_dataset_inputs_by_metadata.
validphys2/src/validphys/theorycovariance/construction.py	Removes the argcheck decorator from total_theory_covmat and deletes unused index-matching helper code.

Comments suppressed due to low confidence (1)

validphys2/src/validphys/theorycovariance/construction.py:549

• This change removes the only runtime guard that caught (and now triggered) the dataset-ordering mismatch scenario when combining point prescriptions + user covmat. Since this PR is specifically fixing a real crash in vp-setupfit/reportengine graph resolution, it would be good to add a regression test that exercises nnfit_theory_covmat with both point_prescriptions and user_covmat_path and asserts it resolves without error (and returns a correctly indexed/typed covmat). That will prevent reintroducing the KeyError and ensure the intended dependency graph remains resolvable.

@table
def total_theory_covmat(theory_covmat_custom, user_covmat):
    """
    Sum of scale variation and user covmat, where both are used.
    """
    return theory_covmat_custom + user_covmat

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

[scarlehoff]

why not a temporary name??

[achiefa]

How would you then restructure the parametrization. Note that tmp_path_factory is already a tmp directory, and the test already goes into it. Would you build the config dict inside the test?

[scarlehoff]

I would not restructure anything. The comment says the name is to avoid clashes. An unique temporary name (with uuid for instance) would for sure avoid clashes.

[scarlehoff]

I see this check is from the time the fitting code was c++ so I'm ready to believe it is obsolete.
However, I also know that two different PhD students three year apart spent many hours on a misordered sum of covmats.
Now it would be about 3 years later from last time... are you absolutely sure this is safe to remove?

Btw, a way to check whether the hypothesis is correct from what you say would be to simply add groups_index to data_input_matched_procs_index as a dummy argument. That should be enough to trigger the production.

[achiefa]

Btw, a way to check whether the hypothesis is correct from what you say would be to simply add groups_index to data_input_matched_procs_index as a dummy argument. That should be enough to trigger the production.

Yes, I tried to add groups_index as a dummy argument and it worked. However, I'm not in favour of this solution for two reasons. First, it's very hard to understand the reason why the dummy variable has been included. However, a comment may help in this case. Second, as you said we'd have an important part of the code (checking ordering of covmats) that rely on obselete code. Moreover, if we were to keep this obselete code, we'd have two functions checking the same thing in two not-so-different branches of the code.

[scarlehoff]

No, I don't think it should be added as an argument of course.
But it is a way to check whether it was a possible issue or the actual issue.

[achiefa]

Good. I reach these conclusions:

• check_fit_dataset_order_matches_grouped checks if group_dataset_inputs_by_metadata follows or not runcard order.
• However, group_dataset_inputs_by_metadata is never used to construct an index in the theory covmat pipeline.
• The method itself is called in

  nnpdf/validphys2/src/validphys/config.py

  Lines 1928 to 1929 in abee4f0

  	def produce_group_dataset_inputs_by_process(self, data_input):
  	return self.produce_group_dataset_inputs_by_metadata(data_input, "nnpdf31_process")

  
  but it's a different thing.
• Anyway, this checked was a guard and didn't fix the order of the code: it just refused to run. Now the code fixes the ordering by reindexing. On the writing side, theory_covmat_custom_fitting / total_theory_covmat_fitting / user_covmat_fitting push the covmat through _reindex_covmat_to_fitting_order with data_input_matched_procs_index. This latter matches with the order of the runcard, which is the same as the exp. covmat. On the reading side, produce_loaded_theory_covmat always reindexes.
• The critical part is

  nnpdf/validphys2/src/validphys/theorycovariance/construction.py

  Lines 544 to 549 in abee4f0

  	@table
  	def total_theory_covmat(theory_covmat_custom, user_covmat):
  	"""
  	Sum of scale variation and user covmat, where both are used.
  	"""
  	return theory_covmat_custom + user_covmat

  
  where both matrices are added. However, fromfile_covmat uses the same index as in theory_covmat_custom_per_prescription, namely procs_index. If their index mismatches, then the sum would complain rasing an error.

Given these points, I think it's safe to remove this function.