Remove additional t0 flags #1626
Conversation
|
Note this is less flexible than what we used to have as one cannot meaningfully override to |
|
But we might want to do fits without the t0 covmat, right? (for testing, show how important it is, etc) |
|
I think when (if) there is a pressing need to run these tests, whoever does it should re-add the options making sure they are documented and tested properly. It is easy enough to swap for an one off test modifying the source. |
| @@ -158,10 +158,8 @@ def from_yaml(cls, o, *args, **kwargs): | |||
| N3FIT_FIXED_CONFIG['use_scalevar_uncertainties'] = thconfig.get('use_scalevar_uncertainties', True) | |||
| #Sampling flags | |||
| if (sam_t0:=file_content.get('sampling')) is not None: | |||
| N3FIT_FIXED_CONFIG['use_t0_sampling'] = sam_t0.get('use_t0', False) | |||
| N3FIT_FIXED_CONFIG['separate_multiplicative'] = sam_t0.get('separate_multiplicative', True) | |||
| #Fitting flag | |||
There was a problem hiding this comment.
Is the only other flag of sampling separate_multiplicative @andreab1997 ?
If so it might make sense to remove this extra level.
Also, maybe in the same vein as removing the option to not use t0 we might want to remove the option of doing the sampling with the multiplicative uncertainties separated.
There was a problem hiding this comment.
I believe this was the original proposal: it does not really make sense to separate the multiplicative uncertainties. However I think that @Zaharid was against. Are you still?
There was a problem hiding this comment.
Is the only other flag of
samplingseparate_multiplicative@andreab1997 ?
To answer this question, yes so it makes sense to remove completely the sampling level
There was a problem hiding this comment.
I would say let's remove both the sampling level and separate_multiplicative
validphys2/src/validphys/config.py
Outdated
| return covmats.dataset_inputs_total_covmat | ||
| else: | ||
| return covmats.dataset_inputs_exp_covmat | ||
| return covmats.dataset_inputs_t0_exp_covmat |
There was a problem hiding this comment.
I would remove the else given the other changes.
Btw, why are we doing the import inside the method? Circular imports?
There was a problem hiding this comment.
Probable for no good reason anymore: At some point I wanted to make this file import quickly, but that ships has probably sailed.
There was a problem hiding this comment.
What do you mean exactly with removing the else? I would say we want support for enabling and disabling the theorycovmat. Not clear to me why there are two flags.
cc @andreab1997
There was a problem hiding this comment.
Just a stylistic comment, I would do
if this:
return that
return thetbut it's unimportant, if you prefer the else leave it.
The two flags are there because the th covmat can be used in the fit or in the sampling.
Since seeing the difference was relevant at some point I think that should stay.
There was a problem hiding this comment.
The two flags are there because the th covmat can be used in the fit or in the sampling.
Since seeing the difference was relevant at some point I think that should stay.
but the two flags are in the fitting action...
There was a problem hiding this comment.
One to say that the th covmat is active and another two to decide whether it is active in sampling or fitting.
A bit like the use_t0 but in this case there is no the solution of "fitting without t0 is wrong, so let's remove the wrong option".
I guess that's the two flags you are referring to ?
There was a problem hiding this comment.
Indeed you can decide to use the thcovmat in only one of the two places or in both and I believe that one of Stefano's student will actually try to have a look to the different fits you can get with these three options.
|
Greetings from your nice fit 🤖 !
Check the report carefully, and please buy me a ☕ , or better, a GPU 😉! |
The work on covariance matrices added new flags to control to t0 setting, namely `use_t0_sampling` and `use_t0_fitting`. These were undocumented, and caused bugs in validphys since t0pdfset could be set by either of them. Remove both flags and mandate that the fitting covmat always uses t0 while the sampling covmat never uses it. This removes some footguns and its backwards compatible. It has the minor disadvantage that the fitting covmat implicitly requires use_t0=True, which is redundant, but it only really matters in n3fit and we set the flag there anyway.
|
I'd like to merge this as is, and defer discussion of other aspects of the covmat setting. |
There was a problem hiding this comment.
For me it is even okay to merge. I am only confused about the flag use_t0. As far as I understood it should always be True (otherwise there will be an error), right? And, even if it is True we are only using t0 for the fitting covmat and never for the sampling covmat. Is my understanding correct? Because in this case I believe it is a bit confusing and we should probably just remove it and rely on the default behaviour.
Ok, but please update the documentation and runcards. |
|
|
|
@scarlehoff which runcards and which docs? AFAICT the relevant docs are https://docs.nnpdf.science/figuresofmerit/index.html?highlight=use_t0 which are correct now and were wrong before. And the runcards as of 4.0 work as of now. |
|
The example runcards |
This partly reverts 9a1e9f6
|
Should I merge this @Zaharid ? |
The work on covariance matrices added new flags to control to t0 setting, namely
use_t0_samplinganduse_t0_fitting. These were undocumented, and caused bugs in validphys since t0pdfset could be set by either of them.Remove both flags and mandate that the fitting covmat always uses t0 while the sampling covmat never uses it.
This removes some footguns and its backwards compatible.
It has the minor disadvantage that the fitting covmat implicitly requires use_t0=True, which is redundant, but it only really matters in n3fit and we set the flag there anyway.