Introduction of an inconsistency within a Closure Test #1682
Conversation
|
Thank you for this! I am going to review it soon |
… inconSYStencies merge to galileo
validphys2/src/validphys/filters.py
Outdated
| lvl1_inconsistent_fit=False, | ||
| lvl2_inconsistent_fit=False |
There was a problem hiding this comment.
@giovannidecrescenzo I think this names are a bit misleading, I would rather call it type1_inconsistency and type2_inconsistency.
Then, at some point, if we see that they are equivalent we will only keep one.
There was a problem hiding this comment.
I am actually not sure: type1_inconsistency and type2_inconsistency are only clear for us while I believe the name @giovannidecrescenzo suggested are at least a bit more clear for the others
|
@comane I don't know if you addressed the points in my review. If you did, can you please paste the relevant commits as a comment in the relevant sections? (I know it is a pain but it makes reviewing a bit easier :) ) EDIT: same for @giovannidecrescenzo of course |
| @@ -10,6 +10,7 @@ | |||
|
|
|||
| from reportengine import collect | |||
| from reportengine.table import table | |||
| from reportengine.figure import figure, figuregen | |||
There was a problem hiding this comment.
can we move these things to closure_plots.py?
|
|
||
| log = logging.getLogger(__name__) | ||
|
|
||
| l = Loader() |
There was a problem hiding this comment.
The loader should not be used in various modules. Use the existing mechanisms, e.g. in config .py to get data.
| plt_file = ds.commondata.plotfiles[1] | ||
| with open(plt_file, "r") as file: | ||
| card = yaml.safe_load(file) | ||
| prcs = card["nnpdf31_process"] |
There was a problem hiding this comment.
There are easier, correct, ways to get the process. See what is done in config.py.
comane
force-pushed
the
inconSYStencies
branch
from
9d8211f to
e309d4a
Compare
| @@ -0,0 +1,155 @@ | |||
| import dataclasses | |||
There was a problem hiding this comment.
Please add a docstr to this module and move it into the closuretest submodule
| @@ -287,7 +287,10 @@ def level0_commondata_wc(data, fakepdf): | |||
|
|
|||
|
|
|||
| def make_level1_data( | |||
| data, level0_commondata_wc, filterseed, experiments_index, sep_mult | |||
| data, level0_commondata_wc, filterseed, experiments_index, | |||
| sep_mult, MULT = False, ADD = False, CORR = False, UNCORR = False, SPECIAL=False, | |||
There was a problem hiding this comment.
The way I read this function, the data should be already processed before calling this function, so I would remove these arguments and just process the data before calling make_level1_data since these are arguments that are only active when type1_inconsistency is set to True.
|
|
||
| closure_data = make_level1_data(data, closure_data, filterseed, experiments_index, sep_mult) | ||
| #======= Level 1 closure test =======# | ||
| closure_data = make_level1_data( |
There was a problem hiding this comment.
Process the data and call make_level1_data with the data already processed. Passing through this many arguments is asking for trouble down the line (believe me!)
|
|
||
| # for a consistent fit use commondata with modified systematics and also change central value | ||
| else: | ||
| level1_commondata_dict = {c.setname: c for c in level0_commondata_wc} |
There was a problem hiding this comment.
Basically the only (new) information that needs to be passed to this function is whether there exist an original level 0 common data.
So, in my view, the data should be prepared before calling make_level1_pseudodata and the only argument that needs to be added is original_level0_commondata_wc=None.
Then you can do e.g.
if original_level_0_commondata_wc is None:
level1_commondata_dict = {c.setname: c for c in level0_commondata_wc}
else:
level1_commondata_dict = {c.setname: c for c in original_level0_commondata_wc}| inconsistent_datasets=[], sys_rescaling_factor_1=1, sys_rescaling_factor_2=1, | ||
| type1_inconsistency=False, type2_inconsistency=False, reference_fit=True | ||
|
|
||
|
|
There was a problem hiding this comment.
Same as with make_level1_etc, the data should've been processed before entering this function. Specially given how far removed the arguments are from where they are used!
My personal recommendation would be to create another function of the type.
def filter_closure_data_by_experiment_with_inconsistencies(**bla):
<preprocessing of the data>
return filter_closure_data_by_experiment(data_preprocessed, original_data=data)Note that I'm changing the input data and creating a new key for original data.
It would work just the same (maybe it would be even better) to leave the original arguments as they are and add a single argument inconsistent_data=<data already processed>.
This PR has been replaced in favour of #2180. I'll leave it open since the branch may be used for comparison and if we close the PR we'll probably forget to delete it, but once #2180 is merged this PR can be closed and the corresponding branch deleted.
The purpose of this PR is to study the effects of introducing data inconsistencies within a Closure Test.