feat: limit parameter variations in rankings to stay within parameter bounds #490
Conversation
|
Hi @alexander-held, I think it is sensible to print a warning to the user, and also add a visual hint (a double-dash of some sort on the side that was bound?). Would you like to finish working on it or should I have a go ? |
|
I think I got stuck figuring out how we would visually indicate the bound and also how we best would transport that information through. You're welcome to give this a try! |
| @@ -588,6 +588,10 @@ def ranking( | |||
| init_pars = init_pars or model.config.suggested_init() | |||
| fix_pars = fix_pars or model.config.suggested_fixed() | |||
|
|
|||
| par_bounds = par_bounds or [ | |||
| tuple(bound) for bound in model.config.suggested_bounds() | |||
There was a problem hiding this comment.
would this behave correctly if some parameters are bound but not others? I am guessing then the bound will be None for unboun ones and we have the same issue in variations?
There was a problem hiding this comment.
All parameters are meant to have bounds as far as I am aware so a None should not happen here. I am however unsure if this is technically a guarantee from pyhf, if so at least I do not see it explicitly mentioned. I do not remember ever running into an example where the bounds are not set (other than scikit-hep/pyhf#1516 which I would call a bug). I thought iminuit also requires bounds but that turns out to be wrong, it only requires starting values.
There was a problem hiding this comment.
what I meant here was that If par_bounds is not None or a "falsy"-valued object, it will be chosen instead of the suggested bounds list. So if par_bounds is for example (None, (0,10), None), we still have two unbound parameters. Am I missing something?
There was a problem hiding this comment.
Now I understand your point I think, if a user provides an invalid value then yes this will cause problems. Such an example would not follow the expected format Optional[List[Tuple[float, float]]].
There was a problem hiding this comment.
Yeah so something like:
model, data = model_utils.model_and_data(spec)
fit_results = fit.fit(model,data)
ranking_results = fit.ranking(model, data, fit_results=fit_results, par_bounds=[None, None, (-2,2)])
fails with the current implementation. I guess ideally this user input should be fine, and we should be checking be updating individual bounds which are not set. I don't see why we should completely stop user from specifying only one parameter bound. A simple insertion of elements from suggested_bounds where there is a None entry can fix this issue.
There was a problem hiding this comment.
[None, None, (-2,2)] is an invalid input at the moment, it has to be either None or a list of tuples of floats according to the desired type. This is mostly done to align with the pyhf API. I'm not opposed to supporting what you suggest, though it does feel a bit complicated to me to have a user create a list of None and then update it at the right index. In that case they might as well get the suggested_bounds themselves as a starting point and update that index?
There was a problem hiding this comment.
But either way the addition doesn't hurt so I'm fine with it.
As identified by @malin-horstmann, we can run into scenarios where the ranking will attempt to vary parameters beyond their bounds. This can happen with parameters that have asymmetric uncertainties when only a symmetric estimate was done from the Hessian (for
shapesysin particular this is likely to happen).This PR limits the variations to stick within bounds. Things to think about before finishing this:
The CI fail is currently expected (bounds kwarg changes).