search

scikit-hep / cabinetry

design and steer profile likelihood fits
https://cabinetry.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
26 stars 19 forks source link

Support scipy optimiser #414

Open MarcelHoh opened 1 year ago

MarcelHoh commented 1 year ago

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.

alexander-held commented 1 year ago

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.

MarcelHoh commented 1 year ago

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.

alexander-held commented 1 year ago

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.