-
Notifications
You must be signed in to change notification settings - Fork 67
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
Linesearch in Broyden #559
base: main
Are you sure you want to change the base?
Conversation
@fabianp <https://github.com/fabianp> I noticed you deprecated condition in
this commit
<7238f76>,
what was the reason for this?
The idea was that the options passed with "condition" could now be passed
through the stepsize , since this can be a callable now
Message ID: ***@***.***>
… |
I am not sure I understand. |
the idea is that having a callable for a step-size could be used not just for simple pre-defined schedules, but also for line-searches like (say) a custom Armijo line-search (correct me @vroulet if I'm wrong) |
But this would mean having the step size callable take as input not just the iteration number but also all the things that the line search takes as input and change its output to return all the metadata returned by the line search, no? |
ok, scratch that, sorry for the confusion. I think that the custom line-search was meant to be passed as a callable linesearch function (eventually). But clearly we didn't properly finish the API, and "condition" should not have been deprecated so soon |
ok, I might reinstate it here as it's important for Broyden to be able to change the condition of the linesearch to armijo. |
The idea was to avoid making a list of potential linesearches with string options and let the user define his/her own linesearch, which would always be of type IterativeLineSearch. That would have let us avoid adding the arguments of the linesearch inside the solver, but yes it was not finished... |
This PR follows an email thread with @vroulet (sorry for taking this long to address this).
Basically the idea is that at the moment the Broyden tests are passing but with failed line searches.
Ideally, they should pass without the linesearch failing.
This PR makes sure that at least one test is testing that condition, in addition to using the more modern linesearch API for Broyden.
At the moment, the test is failing because we cannot modify the
condition
parameter of the linesearch. Indeed, as pointed out by @vroulet , scipy uses the armijo line search for Broyden, and if we hack our way into using it, the test passes.@fabianp I noticed you deprecated
condition
in this commit, what was the reason for this?This needs to be merged after #529