-
Notifications
You must be signed in to change notification settings - Fork 17
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
Allow lbfgs on auglag, expose more kwargs and fix max_eval #44
Allow lbfgs on auglag, expose more kwargs and fix max_eval #44
Conversation
Codecov Report
@@ Coverage Diff @@
## master #44 +/- ##
==========================================
+ Coverage 90.20% 90.41% +0.20%
==========================================
Files 3 3
Lines 143 146 +3
==========================================
+ Hits 129 132 +3
Misses 14 14
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. I suggested changes for the names:
modifier
->subproblem_modifier
tron_max_eval
->subsolver_max_eval
And I'm thinking that maybemax_cgiter
could be passed as a Dictsubsolver_kwargs
because its splats nicely and when/if we add new subsolvers, it won't need much change.
Co-authored-by: Abel Siqueira <nepper271@gmail.com>
Co-authored-by: Abel Siqueira <nepper271@gmail.com>
Co-authored-by: Abel Siqueira <nepper271@gmail.com>
Co-authored-by: Abel Siqueira <nepper271@gmail.com>
Let me know when it's time for a new review with the "request re-review" button, please. |
Thanks |
This PR does 3 things. Let me know if you prefer I split them.
modifier
kwarg, one can now pass a function that changes the augmented Lagrangian model to an LBFGS modeltron
topercival
to give more options to set.max_eval
referred to the total number of function evaluations not for each loop iteration. Let me know if you think a different kwarg would be better here.