Skip to content

Crash when combining point prescriptions with a user covmat #2504

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.

Sign up for GitHub

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

Open
achiefa wants to merge 4 commits into
base: master
Choose a base branch
from
+93 −42
Open

Crash when combining point prescriptions with a user covmat #2504

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a298c58
Fix reported crash
achiefa Jul 27, 2026
b669dad
Add tests
achiefa Jul 27, 2026
abee4f0
Add comment to specify the ordering of produce_group_dataset_inputs_b…
achiefa Jul 28, 2026
d5675e2
Cleaning comments in tests
achiefa Jul 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 0 additions & 17 deletions validphys2/src/validphys/theorycovariance/construction.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
from validphys.theorycovariance.higher_twist_functions import compute_deltas_pc
from validphys.theorycovariance.theorycovarianceutils import (
check_correct_theory_combination,
check_fit_dataset_order_matches_grouped,
process_lookup,
)

Expand Down Expand Up @@ -543,7 +542,6 @@ def user_covmat(


@table
@check_fit_dataset_order_matches_grouped
def total_theory_covmat(theory_covmat_custom, user_covmat):
"""
Sum of scale variation and user covmat, where both are used.
Expand Down Expand Up @@ -595,21 +593,6 @@ def user_covmat_fitting(user_covmat, data_input_matched_procs_index):
return _reindex_covmat_to_fitting_order(user_covmat, data_input_matched_procs_index)


def procs_index_matched(groups_index, procs_index):
"""procs_index but matched to the dataset order given
by groups_index."""
# Making list with exps ordered like in groups_index
groups_ds_order = groups_index.get_level_values(level=1).unique().tolist()
# Tuples to make multiindex, ordered like in groups_index
tups = []
for ds in groups_ds_order:
for orig in procs_index:
if orig[1] == ds:
tups.append(orig)

return pd.MultiIndex.from_tuples(tups, names=("process", "dataset", "id"))


@table
def theory_corrmat_custom(theory_covmat_custom):
"""Calculates the theory correlation matrix for scale variations
Expand Down
24 changes: 0 additions & 24 deletions validphys2/src/validphys/theorycovariance/theorycovarianceutils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,30 +85,6 @@ def check_correct_theory_combination_internal(
check_correct_theory_combination = make_argcheck(check_correct_theory_combination_internal)


@make_argcheck
def check_fit_dataset_order_matches_grouped(
group_dataset_inputs_by_metadata, data_input, processed_metadata_group
):
"""
Check for use with theory covmat generation.

Makes sure that the order of datasets listed in the fit runcard is the same
as that specified by the metadata grouping. Otherwise there can be a
misalignment between the experiment covmat and theory covmat.
"""
data_input_iter = iter(data_input)
for group in group_dataset_inputs_by_metadata:
for dsinput in group["data_input"]:
grouped_ds = dsinput.name
input_ds = next(data_input_iter).name
check(
grouped_ds == input_ds,
"Dataset ordering is changed by grouping, this will cause "
"errors when running fits with theory covmat. Datasets should "
f"be ordered by {processed_metadata_group} in the runcard.",
)

@scarlehoff scarlehoff Jul 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 achiefa Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 scarlehoff Jul 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 achiefa Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.


def process_lookup(name):
"""
Returns the `nnpdf31_process` of the corresponding dataset.
Expand Down
Loading