SasView / sasview

Code for the SasView application.
BSD 3-Clause "New" or "Revised" License
49 stars 41 forks source link

5.x: Parameter limits in the Fit Page are not respected. #1999

Closed smk78 closed 1 year ago

smk78 commented 2 years ago

This defect was originally reported by User AdrianR using 5.0.3 on 28 Dec 2020 but I cannot find a corresponding issue (@krzywon might...). However, User GregS has now reported the same issue using 5.0.4 which suggests no fix was implemented. So am logging the issue here.

Adrian said:

I have been using SasView 5.03 under Windows 10 and noticed an anomaly when fitting and applying constraints (simply minimum or maximum values of parameters) that these are not always applied after some iterations of fitting. The attached screen shot shows 'thickness1' exceeding the maximum but my impression is that this can happen with various models/parameters. I am not certain from the various web pages whether this corresponds to other comments for which tickets have been generated or whether this may arise from unusual values.

ScreenShot_Parameters

Greg says:

I was again trying to fit SESANS data, but the fit value of some SLDs went beyond my pre-defined limits. For models with fewer parameters, it seemed fine, but for this highly parameterised model, it didn’t restrict the fit to these limits.

Screenshot 2021-12-09 at 12 16 55
krzywon commented 2 years ago

This seems related to #965, though, that ticket does say this issue is resolved in 5.x.

butlerpd commented 2 years ago

This seems to pop up not infrenquently from what I recollect but not sure we've ever succeeded in tracking it down. It seems however that it must be either in the way the fitting object is prepared and sent to bumps or in bumps itself? Perhaps @pkienzle has more recollection of this?

pkienzle commented 2 years ago

Bumps is using a penalty function for values outside the bounds in Levenberg-Marquardt which does not guarantee that values will stay in range. The simple solution is to use to use "mp" instead of "lm" for least squares optimization: https://github.com/SasView/sasview/blob/410cffdb9918fdbaa737ef5e4f5694a59dfb797e/src/sas/qtgui/Perspectives/Fitting/FittingOptions.py#L21 and remove "lm" from the available fitters:

fitters.FITTERS.pop("lm", None)
smk78 commented 2 years ago

Forgive my ignorance, but what is "mp"?

pkienzle commented 2 years ago

MPFit ("mp") is an alternative L-M fitter that supports bounds. I don't know if it is in version of bumps used by sasview. I see that it is missing from the bumps docs.

pkienzle commented 2 years ago

I propose modifying bumps to use MPFit instead of scipy.leastsq for L-M fitting. The results will be slightly different due to differences in calculation of the Jacobian and general numerical noise, but it should not impact results significantly. A test of a simple fit gives a difference on the order of 1e-8 in the resulting parameter values.

pkienzle commented 2 years ago

Test with https://github.com/bumps/bumps/pull/89 to see if it fixes the problem.

butlerpd commented 2 years ago

Thanks @pkienzle I was about to ask about LMFIT, which seems to be an extension of sipy, vs MPFIT which is based on MINPACK but I see you already have it? mp is not listed in the docs for bumps 0.8.1 as an option? was that one of those "hidden" options?

butlerpd commented 2 years ago

FYI from scipy v1.7.1 manual for scipy.optimize.least_squares method parameter list (for parameter: method):

method{‘trf’, ‘dogbox’, ‘lm’}, optional Algorithm to perform minimization.

‘trf’ : Trust Region Reflective algorithm, particularly suitable for large sparse problems with bounds. Generally robust method.

‘dogbox’ : dogleg algorithm with rectangular trust regions, typical use case is small problems with bounds. Not recommended for problems with rank-deficient Jacobian.

‘lm’ : Levenberg-Marquardt algorithm as implemented in MINPACK. Doesn’t handle bounds and sparse Jacobians. Usually the most efficient method for small unconstrained problems.

Default is ‘trf’. See Notes for more information.

So the manual explicitly states that lm does NOT handle bounds. So that would explain it. I guess bumps was trying to account for that by implementing a penalty function outside the lm call itself?

pkienzle commented 2 years ago

Bumps quickly hacked in a L-M fitter so that sasview only needed to support one optimizer infrastructure. The author (me) didn't necessarily do a good job of it. IIRC, I was using arctan to map [-∞, ∞] into [-1, 1] to handle bounds prior to Bumps; I don't know why I decided that simple penalty function would be good enough.

butlerpd commented 2 years ago

Fixed in 5.0.5 by upgrading bumps version

smk78 commented 1 year ago

I've reopened this issue because I'm seeing some concerning behaviour in Release 5.0.5 using the (new) L-M optimizer.

Run 5.0.5 Load some data; send it to fitting; select a model Compute/Plot Now change a Min limit that was by default 0.0 or -inf to a non-zero value Check some parameters, including the one you changed the Min limit on Click Fit

I'm getting this:

12:07:04 - ERROR: Fitting failed: Traceback (most recent call last):
  File "sas\qtgui\Perspectives\Fitting\FitThread.py", line 79, in compute
  File "sas\qtgui\Perspectives\Fitting\FitThread.py", line 19, in map_apply
  File "sas\qtgui\Perspectives\Fitting\FitThread.py", line 16, in map_getattr
  File "sas\sascalc\fit\BumpsFitting.py", line 291, in fit
AssertionError
12:07:04 - ERROR:  
 None
12:07:04 - ERROR: Traceback (most recent call last):
  File "sas\qtgui\Perspectives\Fitting\FittingWidget.py", line 1839, in fitComplete
IndexError: string index out of range

We should fix this for 5.0.6.

smk78 commented 1 year ago

You do not get these errors if you only change the Max limit of a parameter.

And you do not get these errors if you switch to a different optimizer. It is specific to the L-M.

krzywon commented 1 year ago

I just tested this and was able to reproduce the error @smk78 is noting. If you set a range with the starting value outside the fit range, this error happens. This happens when either the Min or the Max is changed to set the starting value outside the range. This does not happen in 5.0.4, suggesting the new L-M/MPFit optimizer is the culprit.

smk78 commented 1 year ago

Latest bug now fixed by PR https://github.com/SasView/sasview/pull/2422 Closing this issue