add argument line_search_kwargs to BFGS#1070
Conversation
| max_line_search_iterations=50, | ||
| name=None): | ||
| name=None, | ||
| line_search_kwargs={}): |
There was a problem hiding this comment.
We have a pretty strong convention in TFP of letting the name argument be last.
Also please use None as a default value and explicitly set it to {} if None in the body, because {} is not safe as a default argument (see lint error on Travis).
| current_state, value_and_gradients_function, actual_search_direction, | ||
| tolerance, f_relative_tolerance, x_tolerance, stopping_condition, | ||
| max_line_search_iterations) | ||
| max_line_search_iterations, **line_search_kwargs) |
There was a problem hiding this comment.
The purpose is for the user to be able to control the parameters of the actual line search algorithm, namely linesearch.hager_zhang. All of the parameters that line_search_step itself consumes are already being passed in explicitly. Please pass the dictionary through to that depth, and add a unit test that you can, indeed, affect the behavior of the algorithm by controlling at least one of those parameters (e.g., initial_step_size).
|
@rushabh-v will you be following up on the test @axch requested? |
|
I am sorry, I tried at that time but couldn't figure out a way to test that (I could not figure out changing which argument would make what change in the output, because I don't understand the math behind it). And currently, I am really low on time. Should I close this PR? |
5 participants
Fixes: #1043