fix: respect fixed parameters in n_dof calculation #479
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #479 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 2093 2096 +3
Branches 346 347 +1
=========================================
+ Hits 2093 2096 +3 ☔ View full report in Codecov by Sentry. |
| model: pyhf.pdf.Model, | ||
| data: List[float], | ||
| best_twice_nll: float, | ||
| fix_pars: Optional[List[bool]] = None, |
There was a problem hiding this comment.
Please add a , * between the mandatory and optional arguments to force the optional arguments to be keyword-only (see also other functions in this file for examples). This is done to allow re-ordering keyword arguments in the future without fear of breaking the setup for anyone, as otherwise people could be using fix_pars as a positional argument still and relying on the order.
| """Returns the number of unconstrained parameters in a model. | ||
| def unconstrained_parameter_count( | ||
| model: pyhf.pdf.Model, | ||
| fix_pars: Optional[List[bool]] = None, |
There was a problem hiding this comment.
same comment here about , *
| for parname in model.config.par_order: | ||
| if not model.config.param_set(parname).constrained: | ||
| # only consider non-constant parameters | ||
| n_pars += model.config.param_set(parname).suggested_fixed.count(False) |
There was a problem hiding this comment.
I am trying to remind myself if there was a particular reason I wrote this in this fashion because it felt slightly unusual to me upon revisiting. Looking at the implementation for model.config.suggested_fixed() in https://github.com/scikit-hep/pyhf/blob/1af5ed423cfbdda40dedabf93fed772c729adee3/src/pyhf/pdf.py#L437-L441, that seems to be using the exact same interface though so I think for the fix_pars=None case, the new approach is fully consistent with the old one. I will run with some example workspaces to sanity check this but tests look good too so I think this solution is exactly what we need!
Solves #478 by passing fixed parameters to
model_utils.unconstrained_parameter_count.