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

ENH: optimize: add a new method least_squares_lm for curve_fit. #18198

Closed
wants to merge 1 commit into from

Conversation

AtsushiSakai
Copy link
Member

@AtsushiSakai AtsushiSakai commented Mar 26, 2023

Reference issue

Fix #14337

What does this implement/fix?

The current curve_fit has these methods:

  1. lm, which is using leastsq
  2. trf, which is using least_squares with method='trf'.
  3. dogbox, which is using least_squares with method='dogbox'.

So, the current curve_fit is missing a method that is using least_squares with method='lm'.

Both leastsq and least_squares with method='lm' are based on MINPACK’s lmdif and lmder functions,
but as reported in #14337, least_squares with method='lm' has some good optional arguments of least_squares.
(e.g. jac_sparsity for large systems)
So, this PR adds a new method least_squares_lm for curve_fit, which is using least_squares with method='lm' for curve fitting and enhanced unit tests for it.

Additional information

@tupui
Copy link
Member

tupui commented Mar 26, 2023

Thanks @AtsushiSakai! Since you had a look at this closely, is there a reason we don't change lm to not use leastsq but directly least_squares? I guess there are backward compatibility concerns, but how much is this a concern? Results are worse, better, API is too different, other?

@AtsushiSakai
Copy link
Member Author

Thank you for your comment @tupui. Yes, I though we should replace leastsq to least_squares with lm option.
But, least_squares is missing some optional arguments which leastsq has: (e.g. full_output, factor, epsfcn, diag, etc.).
We need to add these to least_squares for replacing. Do you think we should try it?

@andyfaff
Copy link
Contributor

But, least_squares is missing some optional arguments which leastsq has: (e.g. full_output, factor, epsfcn, diag, etc.).

It might be worth looking into the feasibility of this. It may prove quite difficult to do this and get all the bugs ironed out. Even if it's possible one trouble is that the least_squares interface then becomes much more complex. For example the least_squares interface might gain an epsfcn keyword, but how would that be handled if diff_step is also specified.
In other words, there's a whole bunch of keywords that are only be active for one method, and they would interact/overlap to some extent with existing keywords.

@tupui
Copy link
Member

tupui commented Mar 27, 2023

Maybe instead of trying to fit all the API of one into the API of the other, we could find a minimal set of parameters? i.e. that might mean some deprecation being introduced for the lm option (e.g. if we don't want anymore full_output, calling with full_output would call leastsq and raise a deprecation warning.)
(I am sorry I am not really helpful here as I don't work with these and don't have much sense as of now on how these are used in the wild.)

@andyfaff
Copy link
Contributor

My initial thoughts are that least_squares_lm is the least painful route. But it's a good idea for someone to do that feasibility check

@tupui
Copy link
Member

tupui commented Mar 27, 2023

Oh it is for sure. I am just trying, if we can, to avoid a double deprecation loop 😅 Like adding least_squares_lm and later realise we want to remove it in favour of lm.

@dschmitz89
Copy link
Contributor

dschmitz89 commented Mar 31, 2023

I might be misunderstanding but from what i understand, all the advanced feature sof least_squares are not available via MINPACK's Levenberg-Marquardt solver. See the docs here:

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

So I am not sure if this PR would only complicate the API.

@lucascolley lucascolley added the needs-decision Items that need further discussion before they are merged or closed label Dec 23, 2023
@lucascolley lucascolley removed the needs-decision Items that need further discussion before they are merged or closed label Jan 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature or improvement scipy.optimize
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Full functionality of optimize.least_squares for optimize.curve_fit
5 participants