Detector class bug fixes #743
Conversation
|
Was it that the parameters weren't updating at all for one of the distribution makers? I might've been running into the same bug! I can look over this |
| if isinstance(params,Param): params = ParamSet(params) | ||
|
|
||
| for distribution_maker in self.distribution_makers: | ||
| ps = deepcopy(params) |
There was a problem hiding this comment.
Does deepcopy need to be used here? As I understand, it's probably to protect the internal Params whenever what's passed to this method is changed later on, but it messes with DerivedParams in a weird little way where they lose references to the original Params they depend on.
I'm confident I could set it up to update those references when this copy happens, but I thought I'd check to see if there was a specific reason this was implemented as-is!
There was a problem hiding this comment.
I think deepcopy is needed here because the params set is changed afterwards to pass it to the distribution maker but we don't want the detectors class param set to be changed
| @@ -2518,6 +2566,8 @@ def _minimizer_callable(self, scaled_param_vals, hypo_maker, data_dist, | |||
| scaled_param_vals = np.where(flip_x0, 1 - scaled_param_vals, scaled_param_vals) | |||
| # Set param values from the scaled versions the minimizer works with | |||
| hypo_maker._set_rescaled_free_params(scaled_param_vals) # pylint: disable=protected-access | |||
| if hypo_maker.__class__.__name__ == "Detectors": | |||
| update_param_values_detector(hypo_maker, hypo_maker.params.free) #updates values for ALL detectors | |||
There was a problem hiding this comment.
One more question - does update_param_values_detector need to be called every minimizer step? It looks like that could be rather expensive since it also makes calls back to init_params, and the free parameters should already be updated here.
There was a problem hiding this comment.
It is needed to update the parameters for all distribution makers
|
Everyone happy with this or are more eyes on this needed? |
|
i think this is ready |
Updating the detectors class to PISA4 didn't work as I thought. So here are some bug fixes that hopefully will resolve the issues. It is mostly about updating parameter values for the different DistributionMakers that represent the detectors.