Move histTupleFlavor handling to Setup#282
Conversation
|
@cms-flaf-bot please test
|
|
pipeline#15282022 started |
|
pipeline#15282022 failed |
Refactor logic for handling histogram tuple variables to improve readability.
Refactor access to histTuple flavor variables for better readability.
|
@cms-flaf-bot please test
|
|
pipeline#15282141 started |
|
pipeline#15282141 failed |
Refactor list comprehensions for clarity in Setup.py.
|
@cms-flaf-bot please test
|
|
pipeline#15282331 started |
|
pipeline#15282331 failed |
Refactor variable handling to ensure uniqueness in histTuple variables.
Replaced histTuple_vars with unique_list to ensure uniqueness.
|
@cms-flaf-bot please test
|
|
pipeline#15282512 started |
|
pipeline#15282512 failed |
|
@cms-flaf-bot please test
|
|
pipeline#15282562 started |
|
pipeline#15282562 failed |
|
pipeline#15282751 failed |
|
pipeline#15282817 passed |
|
@kandrosov CI passed with robin_cache option to fix Hmm analysis. Ready for review |
| variables = setup.global_params["variables"] | ||
| elif type(setup.global_params["variables"]) == dict: | ||
| variables = setup.global_params["variables"].keys() | ||
| variables = setup.histTuple_plot_vars |
There was a problem hiding this comment.
How is it that HistTupleProducer uses histTuple_plot_vars but not histTuple_fullres_vars?
There was a problem hiding this comment.
In the stupidest way in the world -- the original fullres vars were a patch done on top of existing histTupleProducer. Meaning the Analysis/HistTupleProducer.py "variables" is only things that need to be binned.
The "fullres" vars are actually used in HH_bbWW/Analysis/histTupleDef.py like here:
https://github.com/cms-flaf/HH_bbWW/pull/100/changes#diff-0be58a80ddb2156d834672742929e8b5d6188c495528150fcc3ce49660014f82
There was a problem hiding this comment.
So, can you move it to HistTupleProducer for consistency?
| unique_list = [] | ||
| seen_hashable = set() | ||
| seen_unhashable = [] | ||
| for var in self.histTuple_plot_vars + self.histTuple_fullres_vars: | ||
| if isinstance(var, dict): | ||
| if var not in seen_unhashable: | ||
| seen_unhashable.append(var) | ||
| unique_list.append(var) | ||
| else: | ||
| if var not in seen_hashable: | ||
| seen_hashable.add(var) | ||
| unique_list.append(var) |
There was a problem hiding this comment.
Here is a good place to check the unique names of the variables (and raise an error if they are not). So I propose something like this:
var_dict = {}
for var in self.histTuple_plot_vars + self.histTuple_fullres_vars:
var_name = var["name"] if isinstance(var, dict) else var
if var_name in var_dict:
raise RuntimeError(f"Duplicated variable name {var_name}")
var_dict[var_name] = var
self.histTuple_vars = list(var_dict.values())There was a problem hiding this comment.
After going back through this, I disagree about the duplicated variables thing.
I don't have a 100% clear reason for it, but I can imagine we will sometimes want both a plot and a fullres version of a variable. I think the duplicate business would need to happen on plot vars and fullres vars separately.
Refactored variable uniqueness check to use a dictionary for improved efficiency and clarity.
|
@cms-flaf-bot please test (Cache rebuild magic?)
|
|
pipeline#15303956 started |
|
pipeline#15303956 passed |
| variables = setup.global_params["variables"] | ||
| elif type(setup.global_params["variables"]) == dict: | ||
| variables = setup.global_params["variables"].keys() | ||
| variables = setup.histTuple_plot_vars |
There was a problem hiding this comment.
So, can you move it to HistTupleProducer for consistency?
|
@cms-flaf-bot please test (Cache rebuild magic?)
|
|
pipeline#15314824 started |
|
@cms-flaf-bot please test
|
|
pipeline#15314861 started |
|
pipeline#15314824 failed |
|
@cms-flaf-bot please test
|
|
pipeline#15314861 failed |
|
pipeline#15315060 started |
|
@cms-flaf-bot please test
|
|
pipeline#15315207 started |
|
pipeline#15315060 failed |
|
pipeline#15315207 passed |
|
@kandrosov ready for review again. Everything looks good. bbWW has a new default histTupleFlavors that includes lep1_pt as both a binned and a fullres variable that is correctly handled in the final artifact from the test. Full resolution has also been moved from HH_bbWW/Analysis/histTupleDef to the standard FLAF/Analysis/histTupleProducer |
3 participants
Moving the histTuple_flavor logic into the Setup class.
histTuple_flavor was a key used in the global.yaml that can point to different histTuple setups. This contains 2 parts:
"variables" -- list of variables to plot into histograms
"fullResolution_variables" -- list of variables to save at full resolution (NOT PLOT)
Previously, through some quirks, law tasks were created using the default variables list always, even when histTuple_flavor was set to something else (DNN_training in bbWW example). This created weird dependency and branch creation.
This new method is set to instead handle both variable lists through Setup, creating
setup.histTuple_plot_varsandsetup.histTuple_fullres_vars. Through these and the unionsetup.histTuple_vars, the law tasks are correctly created now.The side effect is now all
global.yamlneed to move their defaultvariablesand old namedhistTuple_fullResolution_variablesinto adefaultkey of theirhistTuple_flavorsdict.