cabinetry icon indicating copy to clipboard operation
cabinetry copied to clipboard

Support scipy optimiser

Open MarcelHoh opened this issue 2 years ago • 3 comments

This PR implements the suggestions on #413.

  1. Add a minimizer option to limit,ranking,scan,significance , _fit_model and fit_model that allows for switching between minuit and scipy. Set defaults to use scipy when no uncertainties are needed.
  2. Removes the custom_fit option 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.

MarcelHoh avatar Jun 24 '23 12:06 MarcelHoh

Thanks for getting this started and sorry for the slow feedback!

I would like to keep custom_fit around for fit.fit() (fine to remove from ranking and scan) as it is sometimes convenient to switch to it for debugging and adjust the Minuit functions being called within _fit_model_custom, which gives convenient full access. This is certainly more of a power user feature, I suspect most people would not need that, but it is the reason I've kept around this function.

There is no need to implement a scipy-equivalent of _fit_model_custom though I would say, so if fit() is called with both custom_fit and minimizer=scipy then we could just raise a NotImplementedError.

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.

alexander-held avatar Jul 01 '23 09:07 alexander-held

Thanks for the feedback! Sorry for the delay on my side as well, I've been quite busy the last week. I will probably not have time to get back to this for a few more weeks, but it's high on my to-do list.

MarcelHoh avatar Jul 14 '23 12:07 MarcelHoh

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.

alexander-held avatar Jul 15 '23 20:07 alexander-held