Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
smk78 opened this issue Jan 12, 2022 · 15 comments · Fixed by #2422
Closed

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

smk78 opened this issue Jan 12, 2022 · 15 comments · Fixed by #2422
Labels
Defect Bug or undesirable behaviour
Milestone

Comments

@smk78
Copy link
Contributor

smk78 commented Jan 12, 2022

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

@smk78 smk78 added the Defect Bug or undesirable behaviour label Jan 12, 2022
@smk78 smk78 added this to the SasView 5.0.5 milestone Jan 12, 2022
@krzywon
Copy link
Contributor

krzywon commented Jan 12, 2022

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

@butlerpd
Copy link
Member

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
Copy link
Contributor

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:


and remove "lm" from the available fitters:

fitters.FITTERS.pop("lm", None)

@smk78
Copy link
Contributor Author

smk78 commented Jan 13, 2022

Forgive my ignorance, but what is "mp"?

@pkienzle
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

Test with bumps/bumps#89 to see if it fixes the problem.

@butlerpd
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor

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 butlerpd modified the milestones: SasView 5.0.5, SasView 5.1.0 Jan 27, 2022
@butlerpd
Copy link
Member

butlerpd commented May 7, 2022

Fixed in 5.0.5 by upgrading bumps version

@butlerpd butlerpd closed this as completed May 7, 2022
@smk78 smk78 reopened this Jan 12, 2023
@smk78
Copy link
Contributor Author

smk78 commented Jan 12, 2023

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
Copy link
Contributor Author

smk78 commented Jan 12, 2023

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
Copy link
Contributor

krzywon commented Jan 17, 2023

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
Copy link
Contributor Author

smk78 commented Jan 19, 2023

Latest bug now fixed by PR #2422
Closing this issue

@smk78 smk78 closed this as completed Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Defect Bug or undesirable behaviour
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants