-
Notifications
You must be signed in to change notification settings - Fork 34
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
Remove gradient storage #48
Conversation
Codecov Report
@@ Coverage Diff @@
## master #48 +/- ##
==========================================
- Coverage 48.63% 45.52% -3.11%
==========================================
Files 8 8
Lines 584 615 +31
==========================================
- Hits 284 280 -4
- Misses 300 335 +35
Continue to review full report at Codecov.
|
@@ -84,7 +82,6 @@ function _hagerzhang!{T}(df, | |||
x::Array{T}, | |||
s::Array, | |||
xtmp::Array, | |||
g::Array, |
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.
If you keep a signature with a warning and g_scratch
, and make sure that copy!(g_scratch, d.g
at the end of this method, like
function _hagerzhang!(df, x, s, xtmp, g, lsr, c, mayterminate, ...)
warn("you no longer have to provide a "g" input")
_hagerzhang!(df, x, s, xtmp, lsr, c, mayterminate, ...)
copy!(g, d.g)
end
then this doesn't have to be a breaking change... don't know if it's worth the trouble though.
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.
Good idea! I'll try to do this.
This can be merged, if nobody objects. Then we can tag a new minor |
I'll merge after the tests run (can't be sure the conflict resolution worked flawlessly) |
Fixes #47
I also updated the README and the tests (can't enable them until Optim 1.0 is tagged)