Changed an error to warning for failure to optimise parameters in curve fit #13442
Conversation
Changed error when too many many iterations have failed to optimise the parameters to a warning and returning the results since they will often be valid.
scipy/optimize/minpack.py
Outdated
| @@ -785,7 +785,10 @@ def curve_fit(f, xdata, ydata, p0=None, sigma=None, absolute_sigma=False, | |||
| popt, pcov, infodict, errmsg, ier = res | |||
| ysize = len(infodict['fvec']) | |||
| cost = np.sum(infodict['fvec'] ** 2) | |||
| if ier not in [1, 2, 3, 4]: | |||
| if ier==5: | |||
| print("Optimal parameters not found: " + errmsg + " This can be increased by passing the parameter maxfev=n.") | |||
There was a problem hiding this comment.
Just a quick note about two things:
-
Understandable that sometimes Python
warningsdon't show up, but perhaps that is something we can help get working. I just grepped through the code a bit and I do seeclass OptimizeWarning(UserWarning)defined and used through the optimize machinery. Perhaps one of the optimize regulars can comment on using that, along with a filter if one is needed. -
Note that this part of the code is not covered by tests (codecov shows 0 % coverage on the diff). May be an opportunity to encode the desired warnings and exceptions situations in actual unit tests.
There was a problem hiding this comment.
Thanks for the help.
When I set warnings.simplefilter('always') just before curve_fit then it shows and another warning about not being able to find the covariance. I tried going through how warnings is set up but couldn't find anywhere where they're filtered yet.
I have also now added a test but don't have much experience of unit testing so not sure its right/tests for the right stuff. I don't know if I need to change something else too to add it properly, I don't really understand the workflow but happy to try and work it out more.
Added test to check that warning is raised when maxfev is exceeded in curve_fit
|
|
Failed tests seem to be unrelated to my changes so trying to prompt them to re run
|
@jagoosw I resolved merge conflicts for you. If you're happy with what this does now, please send an email to the mailing list proposing the change, mentioning the possible concern that users might miss the warning and think that the returned solution is optimal, and asking for opinions about whether the benefits outweigh the risks. What do you think @tylerjereddy? |
…are created from lists" This reverts commit b0ca26d.
|
Oops that push was accidental. In the future, it's best to create a new feature branch rather than working in |
|
I haven't heard back, and I cannot champion this change, so I'll go ahead and close this one. I would support making this change in conjunction with:
But that would be a more substantial PR. |
ENH: adjusted error conditions for too many iterations to optimise parameters in scipy optimise curve fit.
What does this implement/fix?
Often when the error "Optimal parameters not found: Number of calls to function has reached maxfev = ..." is given perfectly usable results have been produced so raising an error is unhelpful so I have changed it to message (I would have preferred to use the warnings module but it appears to just not be showing anything).
Additional information
This was also complained about here, here and others.