Only initialize runner once in NeymanConstructor #103
Conversation
Pull Request Test Coverage Report for Build 6765927051
💛 - Coveralls |
| # then the poi_expectation is not in the nominal_expectation_values | ||
| component = self.poi.replace("_rate_multiplier", "") | ||
| poi_expectation = expectation_values.get(component, None) | ||
| if runner.input_poi_expectation: |
There was a problem hiding this comment.
Why not just "if poi_expectation" in generate_values?
There was a problem hiding this comment.
Should also be fine. What is the benefit?
| generate_values = runner.update_poi( | ||
| runner.model, self.poi, generate_values, nominal_values |
There was a problem hiding this comment.
I think this won't work with static parameters. That was the entire point of the previous PR
There was a problem hiding this comment.
I ran it quickly and yes, this crashes if you have static parameters (as it should).
There was a problem hiding this comment.
The recent commit 7b4f38f makes the PR compatible with static parameters.
| fittable: false | ||
| blueice_anchors: | ||
| - 50 | ||
| description: WIMP mass in GeV/c^2 |
There was a problem hiding this comment.
This modification is to be compatible with the requirement of ptype.
|
|
||
| def _define_parameters(self, parameter_definition, nominal_values=None): | ||
| def _define_parameters(self, parameter_definition): |
There was a problem hiding this comment.
I think _define_parameters should be as simple as just initializing Parameters.
| @@ -239,6 +236,18 @@ def store_data( | |||
| raise ValueError("The number of data sets and data names must be the same") | |||
| toydata_to_file(file_name, _data_list, data_name_list, **kw) | |||
|
|
|||
| def set_nominal_values(self, overwrite_static=False, **nominal_values): | |||
There was a problem hiding this comment.
Recover set_nominal_values, with an option to overwrite static parameters. And in principle I think we do need a method to overwrite static parameters(bypass the check).
| @@ -54,6 +54,11 @@ def __init__( | |||
| self.name = name | |||
| self._nominal_value = nominal_value | |||
| self.fittable = fittable | |||
| if ptype not in ["rate", "shape", "efficiency", "livetime"]: | |||
There was a problem hiding this comment.
Here the requirement of ptype is more strict.
| @@ -226,7 +238,7 @@ def submit( | |||
| "threshold": [], | |||
| "poi_expectation": [], | |||
| } | |||
| threshold[threshold_key] = threshold_value | |||
| threshold[threshold_key] = deepcopy(threshold_value) | |||
There was a problem hiding this comment.
Very important because the shallow copy will convey the modification of nominal_values and generate_values to threshold_value and threshold.
| @@ -449,7 +449,7 @@ def convert_variations(variations: dict, iteration) -> list: | |||
| if not isinstance(v, list): | |||
| raise ValueError(f"variations {k} must be a list, not {v} with {type(v)}") | |||
| variations[k] = expand_grid_dict(v) | |||
| result = [dict(zip(variations, t)) for t in iteration(*variations.values())] | |||
| result = [dict(zip(variations, deepcopy(t))) for t in iteration(*variations.values())] | |||
There was a problem hiding this comment.
Very important because the shallow copy will convey the modification of items in t in one entry of result, to another entry of result.
dachengx
changed the title
|
|
||
| self._check_ll_and_generate_data_signature() | ||
| self.set_nominal_values(overwrite_static=True, **kwargs.get("nominal_values", {})) |
There was a problem hiding this comment.
@hammannr Can we keep set_nominal_values in the future so that _define_parameters only handle the definition of Parameters? And for the need_reinit parameters, I think they can be defined correctly just by set_nominal_values.
|
This PR can be closed now, right @dachengx ? |
|
Suspended by #108 |
This is a further fix of #100. We do not need to initialize runner for each
**{**nominal_values, **generate_values}, but we can passgenerate_values, nominal_valuestorunner.update_poito pass correctnominal_valuesto get the expectations. Additionally, I made modifications:ptypeto be in["rate", "shape", "efficiency", "livetime"], the defaultptypeis"shape".StatisticalModel.set_nominal_valuesandParameters.set_nominal_values, add option to modify the static parameter so that user is aware of the modification.