Support scipy optimiser #414
Conversation
|
Thanks for getting this started and sorry for the slow feedback! I would like to keep There is no need to implement a scipy-equivalent of As mentioned on the issue, I would also prefer to stick with Minuit by default due to having more confidence in the MIGRAD algorithm. That makes it a conscious choice for the user to go with a faster (but possibly slightly less safe) scipy choice. The pre-commit CI failure is unrelated and fixed by #416. |
| elif minimizer == "scipy": | ||
| bestfit = pyhf.tensorlib.to_numpy(result) | ||
| # scipy does not return uncertainty or correlation results | ||
| uncertainty = np.zeros(bestfit.shape) |
There was a problem hiding this comment.
I think it would be safer to go with something like np.nan here for the uncertainties. Zero is also used to indicate that a parameter is constant (see discussion in scikit-hep/iminuit#762), so it is a bit of an overloaded value already.
| # scipy does not return uncertainty or correlation results | ||
| uncertainty = np.zeros(bestfit.shape) | ||
| corr_mat = np.diag(np.ones(bestfit.shape)) | ||
| minos_results = None |
There was a problem hiding this comment.
So far the code has used an empty dict {} in this case, I don't mind None necessarily but is there a difference in functionality between them for this case or can we stick with the dict?
| bestfit = pyhf.tensorlib.to_numpy(result) | ||
| # scipy does not return uncertainty or correlation results | ||
| uncertainty = np.zeros(bestfit.shape) | ||
| corr_mat = np.diag(np.ones(bestfit.shape)) |
There was a problem hiding this comment.
Super minor, the rest of the code base uses np.diagflat instead of np.diag and I think the reasoning behind this is that np.diag does both extraction and creation while diagflat is only for creating and thus perhaps a bit more concise.
I am unsure how I feel about returning a diagonal matrix here. On one hand, it is the best guess we still have (we know nothing about correlations in this setup), on the other hand it may suggest that we know things that we really do not know. Filling everything with np.nan also doesn't seem ideal as clearly the diagonal will all be ones regardless. Just wondering whether making this a nice diagonal might cause issues with e.g. people using the results to do post-fit plots that do not make any sense.
|
|
||
| Returns: | ||
| FitResults: object storing relevant fit results | ||
| model (pyhf.pdf.Model): the model to use in the fit |
There was a problem hiding this comment.
some accidental whitespace additions in this docstring
|
Thanks for the feedback! Sorry for the delay on my side as well, I've been quite busy the last week. |
|
Hi @MarcelHoh, no worries! Feel free to get back to this when you have time, I don't think it is going to collide with anything in the short term. |
This PR implements the suggestions on #413.
minimizeroption tolimit,ranking,scan,significance,_fit_modelandfit_modelthat allows for switching betweenminuitandscipy. Set defaults to use scipy when no uncertainties are needed.custom_fitoption entirely. This goes beyond your suggestion. Should this still be supported?I haven't looked into tests yet, I thought I would first make the PR to start the discussion.