Excluded dataset page for vp-comparefits

[jekoorn]

PR for the excluded (positivity) datasets in a vp-comparefits report.
This page is added if and only if the theory id, datacuts, and theory covmat (if present) are identical between fits.

[scarlehoff]

Suggested change

	same_theoryid = current_runcard.get("theory").get("theoryid") == reference_runcard.get("theory").get("theoryid")
	same_theoryid = current_runcard.get("theory", {}).get("theoryid") == reference_runcard.get("theory", {}).get("theoryid")

[scarlehoff]

Suggested change

	same_thcovmat = (current_thcovmat == reference_thcovmat or (current_thcovmat is None and reference_thcovmat is None))
	same_thcovmat = (current_thcovmat == reference_thcovmat )

If they are both None they are equal.

That said, if the theory covmat adds any kind of difficulty, I think we can skip the theory covmat check and just make sure that in the page it is stated clearly that this is only the experimental covmat.
Because otherwise you will need to decide from which fit you are getting the covmat (even if the settings are the same, the theory covmat itself will be different because the datasets are different).

[scarlehoff]

Thanks. Seems to work fine (see img below)

The two comments are 1) the necessary change to vp_comparefits to make it work, and 2) a comment on what we meant by excluded. Other than that this is good ^^

(btw, I understand the frustration when dealing with reportengine, I was there many moons ago, but you are navegating it quite well so don't get discouraged! After a while you learn to appreciate the guardrails and tools it offers, I promise!)

[scarlehoff]

This was almost done!

You just need to put the conditional here (well, at the end of this function)

are_the_same = self.check_identical_theory_cuts_covmat()
if are_the_same:
    log.info("Using excluded comparecard: identical theory cuts/covmat detected.")
    autosettings["template"] = "report_with_excluded.md"

These autosettings of complete_mapping is what is used to complete the template afterwards.
In this particular case (in which you are just changing the template but would be using the exact same runcard for everything else) I would not add a new comparecard_excluded.yaml but just the report_with_excluded.md

fwiw, what you were trying to do you could do instead at get_config, but I think it is better to override the template .md and not the entire runcard.
After we merge this we can think, if you want, about creating a report.md dynamic instead of having several similar ones.

[scarlehoff]

I don't think we want every dataset excluded, because in general we cannot get predictions for every dataset (to the very least, we have datasets that are polarized and some that are not and those live in different theories).

What we want is the excluded from dataspec 1 that are also in dataspec 2 (and, well, if you have 10 dataspecs, we want those that are not in all 10).

So, very close to what you have, just don't use all the implemented ones as baseline ^^U

[jekoorn]

Thanks for the comments, it feels like we are making some nice progress. I thought the whole idea was to also include datasets not used in both fits, bu tit obviously increases the complexity. I think with these suggestions I can make some final changes and make it work.

After a while you learn to appreciate the guardrails and tools it offers, I promise!
Even though some of its features have been working against me, I definitely appreciate its strengths! It is a powerful framework, and I hope I will come to bless more often than curse it ;-)

[scarlehoff]

I guess once and only once we have the 4.1 dataset defined we can have a canonical set of data, but at the moment this would be a nightmare because any one dataset or variant or fktables that one has locally and is not online would create problems for everyone else.

[jekoorn]

Indeed. We have to make sure it doesn't do anything crazy.
I was also considering the inclusion of a commandline argument for doing an excluded dataset report, which would perhaps protect against these problems. But the code needs to be sound in any case.

[scarlehoff]

But in any case that we cannot do without a canonical set of data (and theory) because no theory contains fktables for all datasets.

It would also be extremely slow ^^U We might want another script for vp-check-all-data or something. comparefits is supposed to be comparing fits, this is something else.

[jekoorn]

Hi Juan, I still don't think I understand how to go from "returning a list of (pos)dataset names to actually generating plots in the report.
With a rule like:

Positivity excluded from fit
--------------------------
{@with matched_excluded_positivity_from_dataspecs@}
{@posdataset_name@}
{@endwith@}

I can generate a list of dataset names to be included, as before. But I just don't understand why replacing posdataset_name with plot_positivity doesn't give me a plot. What exactly is required for this plot_positivity? Or should I use another function to plot? I tried also plot_dataspecs_positivity, but since an excluded dataset is only in one of both datasets, copying the methodology for the "included" plots will not work.
I wonder what you did to get an actual plot in your screenshot above^^

[scarlehoff]

I did not try with the positivity ones. Does it work for you with the "normal" datasets? I think I used plot_fancy for those.

If you don't manage we can have a meeting next week to finish up the missing pieces. This week is my last of one of the courses and I don't think I can ^^U

[jekoorn]

No I am now using @dataset_report report@, by using the same ChainMap construction as the produce_matched_datasets_from_dataspecs. I indeed get some plots for excluded sets. However, by definition (because it needs to take some stuff from dataspecs) this means that only one of the two fits' predictions for this datasets can be plotted. As opposed to seeing BOTH fits' predictions for this dataset.

I assume that we would prefer the latter, in which case we cannot simply use @dataset_report report@

Then I tried also plot_fancy, but since I do not understand what reportengine is doing, I do not know what I need to return from my produce-function. From dataplots it appears it needs

def _plot_fancy_impl(
        results,
        commondata,
        cutlist,
        normalize_to: (int, type(None)) = None,
        labellist=None,
        with_shift: bool = True,
):
    """Implementation of the data-theory comparison plots. Providers are
    supposed to call (yield from) this.
    Parameters
    -----------
    results : list
        A list of results, where the first one is a data result and the
        subsequent ones are theory predictions.
    commondata : ``CommonDataSpec``
        The specification corresponfing to the commondata to be plotted.
    cutlist : list
        The list of ``CutSpecs`` or ``None`` corresponding to the cuts for each
        result.
    normalize_to : int or None
        The index of the result to which ratios will be computed. If ``None``,
        plot absolute values.
    labellist : list or None
        The labels that will appear in the plot. They will be deduced
        (from the PDF names) if None is given.
    with_shift: bool
        This option specifies wheter one wants (True) or not (False) to shift
        the theoretical predictions by a shift due to the correlated part of
        the experimental uncertainty. The default is True.
    Returns
    -------
    A generator over figures.
    """

results for both theory predictions. The problem is that when I use plot_fancy (even with this produce rule), it looks for a dataset_input in the runcard. But this is only available by definition for actual fitted datasets, and we are back at the problem where I was a while back.

[scarlehoff]

So, some pointers, but probably better to discuss next week with a bit of calm, I think you are overcomplicating your own life a bit too much here.

For now just do the changes necessary to get the list of excluded datasets from the combinations of fits (and not all datasets ) like right now.

To do a plot you only need:

1. Cuts
2. Theory ID
3. PDFs
4. dataset_input (and not dataset_inputs)

Since 1) and 2) you have checked they are the same by this point, you can take them from just one of them and that's perfectly fine. We want that information to come only from one because the other is exactly the same.

3), PDFs, it is set in the comparefit runcard to be a list of the PDFs corresponding to the two fits.

And finally, what you need to provide with your function, a dataset_input:

---

With that out of the way, in order to get the matched_dataset_etc... you indeed need to sets of dataset_inputs to match (or to antimatch), for that you need to add

dataset_inputs:
  from_: fit 

(to each of current and reference)

That's perfectly fine, since you are matching them you need to have them. And then, from those two lists of dataset_inputs that you are going to get (one for each one of the two or more dataspecs that you are matching), you are going to return a subset of dataset_inputs that will take care of 4) above :)

---

so, taking all this in consideration, this should work even if not the most beautiful:

    def produce_matched_excluded_datasets_by_name(self, dataspecs):
        dinputs_a = dataspecs[0]["dataset_inputs"]
        dinputs_b = dataspecs[1]["dataset_inputs"]
        # some fancy logic
        more_info = {
                "pdfs": [i["pdf"] for i in dataspecs],
                "theoryid": dataspecs[0]["theoryid"],
                "fit": dataspecs[0]["fit"] # The fancy logic will put here the _right_ fit since this is used for the cuts
        }
        return [{"dataset_input": i, **more_info} for i in dinputs_b[1:3]]

And in the validphys runcard

  {@ with matched_excluded_datasets_by_name @}
  {@ plot_fancy @}
  {@ endwith @}

This is very not-fancy-at-all since I'm returning a list with a lot of information repeated each time. You can probably play with the report and get the information out that way (something like joined_dataspecs: theory: from current, and then use that joined_dataspecs there... but no way I can do that from memory...)

---

(also, please, rebase on top of master... if you need to merge so be it, but a rebase would be much better!)

[jekoorn]

Thanks for the pointers. Reportengine is a beast, and with some slight changes it now finally produces a report!

There is one caveat. Reportengine looks for a filtered dataset in the fit directory of associated dataspecs, so we will have to either restrict the usage to something like "compare [fit with only AB] to [fit with ABC]". (and not the other way around!!) or do some clever check so that it works both ways around.
I will play around a bit with this to make it more stable.

See
https://vp.nnpdf.science/TMqqwoUZTaOb-HCk2Wf3XQ==/

[scarlehoff]

Reportengine looks for a filtered dataset in the fit directory of associated dataspecs, so we will have to either restrict the usage to something like "compare [fit with only AB] to [fit with ABC]". (and not the other way around!!) or do some clever check so that it works both ways around.

This is done, afair, only for the cuts (when you have use_cuts: fromfit, actually if you change the use_cuts: internal in the comparison report the fit should be unnecessary). So in principle it should be possible to concatenate two lists in which one you have the datasets that were in A but not B (with the cuts of A) and in the other one the datasets that were in B but not A (with the cuts in B).

The snippet of code I wrote before does that in that you have the whole namespace in each item of the list. Not beautiful but it should work.

[jekoorn]

Hi Juan, this should now be finished. dataspecs unfortunately doesn't have a dataspecs[0]["dataset_inputs"] so I have improved the logic in my function now in such a way that it computes the excluded set in a more general way.
It should now be fully working. See https://vp.nnpdf.science/DX3J6AfhTUiO5VWwWEEJtw==/#datasets-excluded-from-fit

I made an attempt to create an actual extra page wth a @excluded_report report@ as is done for the normal datasets, but since we usually will only look at this page if we are specifically going to look at all excluded datasets, it doesn't make sense to me to hide the individual plots behind a link. Especially since we are already at the bottom of the report, and extra scrolling time isn't an issue.

Please take a look, I think at this point only perhaps some cleaning up is required.

[scarlehoff]

dataspecs unfortunately doesn't have a dataspecs[0]["dataset_inputs"]

No, you need to add it to the dataspec with

dataset_inputs:
  from_: fit

the same that all other information is added (or from current and then you need to add it to current as well etc)

[jekoorn]

Ah yes, you are right. This reproduces now a report for me as well.
So we have the luxury of choosing between two working methods, which do we choose?^^

[scarlehoff]

I haven't looked at the code but if both of them work I would choose either the faster or the one that adds the least amount of lines of code* which usually means it is going to be more maintainable.

You choose!

*being reasonable I mean, maybe the longer one is obviously easier to read

[jekoorn]

Unfortunately for me, your function (where we pre-add dataset_inputs from_ fit to dataspecs), is a good 50 seconds faster.
I will commit it.

[scarlehoff]

@jekoorn please rebase on top of master and fix the failing tests (perhaps rebasing will fix it already)

[scarlehoff]

Remove this. We don't want a graveyard of all functions!

[scarlehoff]

Here you have two docstr one after the other.

[scarlehoff]

What are you exactly doing here? If you are just checking the dataset_input from the dataspecs by name you should not need a lot of this.

Note that you are trying to reproduce produce_matched_datasets_from_dataspecs but what you really want is produce_matched_excluded_dataset_inputs_by_name. And what you are really doing, this can be simplified quite a bit.

[scarlehoff]

No need to create a function here. theoryid is fixed (and should come from the outside tbf) pdfs should come from the outside not be part of the output.

[scarlehoff]

imho this can be simplified.

Note that at this point what you want for each dataset just the dataset_input itself and a the fit from which you want to take the cuts.

Everything else is shared. If you don't know how to deal with reportengine to do so that's fine, but at least make it explicit that everything else is shared.

With what you have I would've put:

dataset_inputs = dataspecs[0].as_input()["dataset_inputs"]

# Check that `dataset_inputs` is in the dataspec, otherwise raise Exception early.
# Restrict it to two dataspecs

exclusive_dinputs = []
for spec in dataspecs:
    for dinput in spec["dataset_inputs"]:
        # Check whether it exists in all of the others
        if any( dinput not in s["dataset_inputs"] for s in dataspecs ):
            exclusive_dinputs.append( (dinput, spec) )

[jekoorn]

I see. It's obviously more efficient and simpler to only construct the required objects after knowing what the mismatched datasets are. I have now with your suggestions simplified the function greatly, and added dataset_inputs to dataspecs. This should speed it up considerably.

I am still building the input for plot_fancy myself:

# prepare output for plot_fancy
        for dsin, spec in mismatched_dinputs:
            res.append(
                {
                    "dataset_input": dsin,
                    "dataset_name": dsin.name,
                    "theoryid": spec["theoryid"],
                    "pdfs": [i["pdf"] for i in dataspecs],
                    "fit": spec["fit"],
                }
                    )
        return res

but in principle it should be clear that everything is shared. I'm not sure what I should do with spec["theoryid"]. From the check in vp_comparefits.py we know that they are the same, but do we want to rely on that check?

In any case I've rebased and pushed some changes. I will try to fix the failing tests asap.

[scarlehoff]

Please remove this.

[scarlehoff]

Add a docstr to explain what this is doing.

[scarlehoff]

I think once you have used loader.check_fit above, you can, instead of using .path/filter.yml use directly as_input() to avoid having to open both runcards yourself.

[scarlehoff]

Instead of changing the whole template (and then needing a whole new report) you can change directly the mismatched_report part.

e.g., if are_the_same is True then the template part under mismatched_report stays, otherwise you drop it and change it to `template_text: "Difference settings, mismatched page cannot be added".

[jekoorn]

I am not really sure how to do this. I understand the need, since we don't want 30 different report-xx.md tempalates. You mean take the existing report.md and then add something like the following?

report.update(
                """
                Mismatched datasets
                --------------------------
                {@with mismatched_datasets_by_name@}
                [Plots for {@dataset_name@}]({@mismatched_report report@})
                {@endwith@}
                """
                )

Ideally I wouldn't want to add this page all at the bottom, but somewhere after the other datasets.
Or is there a better way, where we replace only the [Plots for {@dataset_name@}]({@mismatched_report report@}) line with some text like "Different settings, no mismatched datasets can be added". I'm just not sure how to do this :-)

[scarlehoff]

There might be a better / more beautiful way but what I would do is (with minimal modification to what you already did) to add the following to report.md:

[Mismatched report]({@mismatched_information report@})
--------------------

And then to comparecard.yaml

mismatched_information:
  meta: Null
  actions_:
    - report
  
  # Datasets will go to their own page
  mismatched_report:
    meta: Null
    template: mismatched.md

  template_text: |
    Mismatched datasets
    ---------------------
    The following plots corresponds to datasets which are not available in one of the fits.

    {@with mismatched_datasets_by_name@}
    [Plots for {@dataset_name@}]({@mismatched_report report@})
    {@endwith@}

and finally, to vp_comparefits.py

are_the_same = self.check_identical_theory_cuts_covmat()
if are_the_same:
    log.info("Adding mismatched datasets page: identical theory cuts/covmat detected")
else:
    autosettings["mismatched_information"] = {"template_text": "Mismatched datasets cannot be shown due to cuts or theory not being identical"}

In other words, if they are not the same, the template_text gets effectively dropped. You still get a "Mismatched Report" link to nowhere though, but I think that's better than having two reports that are basically the same thing.

Ideally I wouldn't want to add this page all at the bottom

I would actually prefer it very much at the bottom, after the list of datasets that are identical/mismatched so you get "Here's everything that's the same, same cuts and same everything, now click here".
But ok, your report your rules :P

---

There's the alternative of taking the template as a really template and put in the middle

MISMATCHED_PLACEHOLDER

and then do a search and replace of the placeholder with what you already have. That would also be ok for me.

[jekoorn]

Done, waiting for the tests to finish now.

[scarlehoff]

Thanks, did you test it and checked that it worked as expected?

[jekoorn]

Yes, I forgot to attach the report. See below:
https://vp.nnpdf.science/iQYcaNAjQaOsfHIT6Wyl0g==/#mismatched-report

[jekoorn]

Hi, I am now going through the failed checks, that I managed to reproduce locally. In particular, a plot_fancy plot fails because it doesn't look similar enough. See here the baseline, the result and the failed difference. The only difference is a small lack of space below the y=0 axis.
I think that the reason why this happens is because of the fact that I needed to rebuild the limits of plot_fancy https://github.com/NNPDF/nnpdf/pull/2387/files#diff-2257204401799e2ee4b8c681ae348465ff3c7c1f968116e33c3cee332ecefa6b to ensure that some current/reference predictions aren't out of the plot range. See e.g. this plot, that is produced when I do not rebuild the limits. This is obviously not good, since the datapoints do not show up at all.
I'm not sure how to proceed. Should I add a small tolerance around the rebuilt limits that I added in plot_fancy? Or keep as-is, since this is not a change that affects anything imo. Thanks.

[scarlehoff]

Was this needed before or does it happen only for the new page that has been added?

[jekoorn]

Only for the new page.

[scarlehoff]

Then you should either find the reason (why does it happen for the new page but not for the old one?) or at the very least wrap it so that it is done only for the new page.

Otherwise you should consider comparing side by side all ratio plots and look at them one by one... because it already happened in the past that one of this changes looked good for a subset of plots but they were not all checked and quite a few were actually broken (and there are many observables and many plots ^^U, my recommendation is to go with either understanding the problem or leaving it only for the new page)

[github-actions]

Greetings from your nice fit 🤖 !
I have good news for you, I just finished my tasks:

• Fit Name: NNBOT-7733a0908-2025-12-22
• Fit Report wrt master: https://vp.nnpdf.science/FqGDaTeSRXSgc_RpylhnbA==
• Fit Report wrt latest stable reference: https://vp.nnpdf.science/C8cX252XRIW8cucmxk0V2g==
• Fit Data: fit data is kept as an artifact. Please, remember to upload it to the server if the reference is changed.

Check the report carefully, and please buy me a ☕ , or better, a GPU 😉!