Update for new line search functionality. #613
Conversation
There was a problem hiding this comment.
Great :D
Shall we tag a new version of Optim with the last two months changes, or are there more things to address?
I guess that at some point we could possibly use the make_\phi functions from within Optim instead of passing df to the line search. That sounds like a non-breaking change, however, so it can possibly wait?
| @@ -40,10 +40,8 @@ function perform_linesearch!(state, method, d) | |||
| state.alpha, ϕalpha = | |||
| method.linesearch!(d, state.x, state.s, state.alpha, | |||
| state.x_ls, phi_0, dphi_0) | |||
| state.dphi_0_previous = dphi_0 | |||
There was a problem hiding this comment.
Should you not update dphi_0_previous if is is of type InitialConstantChange?
There was a problem hiding this comment.
There was a problem hiding this comment.
Yes, I don't see the value of dϕ_0_previous being assigned or updated anywhere in either Optim or LineSearches?
There was a problem hiding this comment.
That's an oversight. That's supposed to happen in LineSearches! Doesn't seem to cause any errors, so LineSearches probably needs a test and a bug fix :)
| return true # lssuccess = true | ||
| catch ex | ||
| state.dphi_0_previous = dphi_0 |
| state.dphi_0_previous = state.nlpreconstate.dphi_0_previous # assumes precon is a linesearch based method. TODO: Deal with trust region based methods | ||
| if typeof(method.alphaguess!) <: LineSearches.InitialConstantChange | ||
| if typeof(state.nlpreconstate.alphaguess!) <: LineSearches.InitialConstantChange | ||
| method.alphaguess!.dphi_0_previous = state.nlpreconstate.alphaguess!.dphi_0_previous # assumes precon is a linesearch based method. TODO: Deal with trust region based methods |
There was a problem hiding this comment.
Isn't this property now called dϕ_0_previous?
There was a problem hiding this comment.
@pkofod this will crash because the property in alphaguess! Is no longer called dphi_0_previous; it uses the symbolic version of phi.
There was a problem hiding this comment.
Right. Since GMRES uses a specific type it should probably have tests with combinations of these line searches, right?
| @@ -40,10 +40,8 @@ function perform_linesearch!(state, method, d) | |||
| state.alpha, ϕalpha = | |||
| method.linesearch!(d, state.x, state.s, state.alpha, | |||
| state.x_ls, phi_0, dphi_0) | |||
| state.dphi_0_previous = dphi_0 | |||
There was a problem hiding this comment.
Yes, I don't see the value of dϕ_0_previous being assigned or updated anywhere in either Optim or LineSearches?
@anriseth this should sync Optim with LineSearches.