Fix shifts in plot_fancy_dataspecs

[enocera]

This PR ensures that the shift due to correlated systematic uncertainties is applied to all theoretical predictions when more than one prediction is compared to the data, for example when using plot_fancy_dataspecs. This PR fixes #2481.

Here are the same plots as in #2481, but with the fix applied.

https://vp.nnpdf.science/TCmkXajzSeq8TGsHVLE4Dw==

[scarlehoff]

Added a few comments. The general idea is that alpha is only needed for the data so it doesn't need to be part of the output.
And the data doesn't need the shift so it shouldn't go through the costly part of the equation.

If you check for data beforehand, then you avoid also having to guard against using data/theory relying on the order in which enters the plot, because it is not a given that tomorrow we won't change the order for whatever reason (and the bug would completely escape me as it did when I reviewed these changes, because it didn't occur to me to test with more than one prediction...)

[scarlehoff]

I think alpha should not be an output of this function. The complicated/costly part of this function (inverting a matrix, etc) doesn't need to be done for the data part (which is the only reason you might want to enter this loop with the data), right? I think it would be worth it to compute alpha, given solely lcd_wc as a separate function (shifts_from_systematics can also call that same function).

Btw, one thing that might reduce the report time is to make the result hashable (depends only on commondata and on the theory number -if a theory prediction-) such that the amount of shifts that need to be computed for a report are not duplicated... writing but probably an issue to tackle later.

[enocera]

I have separated the computation of the shifts and the extraction of the uncorrelated and correlated parts part of the uncertainty.

[scarlehoff]

I think it would be cleaner if this were instead:

Suggested change

	if with_shift:
	if do_shift:

where

do_shift = with_shift and not isintance(result, DataResult)

(you might need to import DataResults from results.py)

So that you don't enter the if condition with the Data, so you don't have to check if i>=1, which might not always be reliable if things change in validphys. You still need the alpha of course, so make sure that you add:

if with_shift and isintance(result, DataResult):
    err[mask] = compute_alpha(lcd_wc)

[enocera]

In my opinion, we have to enter the condition with the Data, also to get the uncorrelated part of the uncertainty, because these are Data with cuts, and cuts may differ.

[scarlehoff]

Suggested change

	do_shift = False
	# Shift theory predictions, but not data
	if i >= 1 and do_shift:
	cv[mask] = result.central_value - shifts
	else:
	cv[mask] = result.central_value
	# Retain only the uncorrelated part of the data error if shifting the data
	if i == 0 and do_shift:
	err[mask] = alpha
	else:
	err[mask] = result.std_error
	shifts = 0.0
	cv[mask] = result.central_value - shifts
	err[mask] = result.std_error

Since when you are dealing with the data, you are not entering in the if condition, this is safe.

[enocera]

Done.

[enocera]

@scarlehoff I hope to have correctly addressed your ocmments. perhaps, let me generate a vp comparefit reports to check that everything is in order.

[scarlehoff]

Here the check for data/theory is missing with DataResult, right? Or the same needs to be done in all cases?

(but maybe then alpha will need to be define in both path, in the try and the except)

[enocera]

After some thoughts, I think that here, if try fails, we just don't want to apply the shifts, and we want to fall into the unshifted case by default (which is not the case now). I am thinking that perhaps we should fall in this case also when alpha is zero (that is when there's no uncorrelated part of the uncertainty because e.g. the experimentalists provide us only with a covariance matrix). Right now we just set the uncertainty displayed in data/theory plots to zero and do not shift the data (and say in the title that the data is shifted).

[scarlehoff]

I think the way of making sure there is no shift applied is to do

try:
    shifts = shift_calculation()
    alpha = alpha_calculation()
except:
     shifts = 0.0
     alpha = result.std_err

so that it is equivalent to not applying the shift, no?

[enocera]

Not exactly, because I fear that the label printed on the plot will still be "shifted" because try is in the with_shift if. I'm checking right now.

[enocera]

Simple fix, turn with_shift to False.

[scarlehoff]

Suggested change

	shifts = None
	alpha = None
	do_shift = with_shift
	# Compute shifts due to the correlated part on the experiemntal ucnertainty
	if do_shift:
	# Compute shifts due to the correlated part on the experimental uncertainty
	if with_shift:

[enocera]

Done.

[enocera]

@scarlehoff I have rethought how we apply shifts, based on your comments, that I didn't completely understand in the first place, but that I hope to have understood now. What I hopefully do now is to separate the uncertainty on the data and the shift to the theory and to more clearly identify four cases: with shift; no shift (default); no shift, because even if the user requires a shift, the shift cannot be computed because uncorrelated uncertainties are zero; no shift, because, even if the user requires a shift, the computation of the shift fails.

[scarlehoff]

My comments were ill-informed in that I didn't appreciate the fact that two different predictions might have different cuts.

I'll look at the new version of the changes with that in mind. I like the 4 cases though.