Tolerances for snowstorm additional params#936
Conversation
thehrh
left a comment
thehrh
left a comment
There was a problem hiding this comment.
See the handful of inline comments.
NB: From a design standpoint, attributes such as grads, tol, additional_params_values etc. should better be declared as non-public (self._tol etc.) IMO, to indicate that they are only meant for internal use, in particular because this service has a custom caching mechanism. I'm aware that other services don't adhere to such a design, so approval won't depend on this.
| self.tol = tolerances | ||
| assert isinstance(self.tol, list) | ||
| assert len(self.tol) == len(self.additional_params) | ||
| self.tol = np.array(self.tol) |
There was a problem hiding this comment.
- instantiate array with appropriate data type:
self.tol = np.array(self.tol, dtype=FTYPE) from pisa import ureg, FTYPEat top
| # We need to calculate if an additional params value or the apply_mode changed | ||
| additional_params_values = [self.params[p].m for p in self.additional_params] | ||
| if additional_params_values != self.additional_params_values: | ||
| additional_params_values = np.array([self.params[p].m for p in self.additional_params]) |
There was a problem hiding this comment.
instantiate with dtype=FTYPE
| additional_params : list of str (default None) | ||
| Parameters that are no detector systematics but if changed require a | ||
| re-calculation of the gradients (e.g. osc params). | ||
| tolerances : list of float (default None) |
There was a problem hiding this comment.
This description is a bit brief for my liking. I would reformulate this for clarity, for instance like so (adapt if necessary):
Numerical tolerances for `additional_params`, to be given in the same order.
If none are given (default), all gradients will be updated each time at least
one parameter value changes (most accurate but slowest option). If tolerances
*are* given, though, and *all* parameters move away from their initial values
by *less* than the specified tolerances over the course of a single fit, gradients
will *never* be re-calculated. Only once at least one parameter's overall delta
exceeds its corresponding tolerance will all gradients be updated (and a new
reference point set). Subsequent modfications of the parameter vector will
then be compared to this updated referenced point.
| if isinstance(tolerances, str): | ||
| self.tol = eval(tolerances) | ||
| elif tolerances is None: | ||
| self.tol = [FTYPE_PREC] * len(self.additional_params) |
There was a problem hiding this comment.
Having considered this a bit more, instantiating self.tol = np.zeros_like(self.additional_params, dtype=FTYPE) is probably more prudent. We want to err on the side of caution here. You are also not checking parameters' rescaled values in compute_function, so all values could have vastly different magnitudes. I'd say just update gradients and the reference point once any change is detected, however small it may be and irrespective of FTYPE_PREC.
2 participants
Allows a user to set tolerances for the parameters of the snowstorm stage that require a re-calculation of the systematics gradients (e.g. osc params). Setting no tolerance results in a re-calculation every time the parameter is changed.