Provide AD gradient for MLE/MAP #1369
Conversation
src/modes/ModeEstimation.jl
Outdated
| @@ -369,6 +395,11 @@ function _optimize( | |||
| args...; | |||
| kwargs... | |||
| ) | |||
| # Throw an error if we received a second-order optimizer. | |||
There was a problem hiding this comment.
Do we have to do that? Doesn't Optim just use ForwardDiff (or FD?) to compute the Hessian in this case? If that's the case, then we shouldn't throw an error IMO. It might not be the most efficient approach and would not adhere to the user-provided AD settings but as long as it works we could only print a warning.
There was a problem hiding this comment.
In FD yes; but (for example in the project that I am working on) it could be that users only define custom adjoints for the gradients but not the Hessian. Therefore even the user provides an AD backend, it might not be a great idea if it by default take that for Hessian function.
There was a problem hiding this comment.
You mean we shouldn't even print a warning? Would be fine with me as well.
There was a problem hiding this comment.
Oh, I think throwing an error when some Hessian-required optimizer is received is a great idea, just like what Cameron did here.
There was a problem hiding this comment.
If we want to throw an error if the Hessian is evaluated, I suggest using only_fgh!(f) and implementing f(F, G, H, x) that contains the check
if H !== nothing
error("second order methods are not supported at the moment")
endIn general, this approach is more flexible, avoids baking in a hardcoded check for a special type of a different package in our implementation, and avoids incorrect and unexpected behaviour for second-order optimization algorithms that don't subtype this specific type (since multiple inheritance is not possible in Julia, that's not an impossible scenario per se).
There was a problem hiding this comment.
It's a bug. I think there might be an issue for it.
There was a problem hiding this comment.
I just found JuliaNLSolvers/Optim.jl#718, I guess that's the related issue.
There was a problem hiding this comment.
Yeah, I have a fix. Sorry to cossio for waiting a year and a half 😬
There was a problem hiding this comment.
I mean, I will tag a fix in an hour or so, so please don't special case with a branch.
Project.toml
Outdated
| @@ -1,6 +1,6 @@ | |||
| name = "Turing" | |||
| uuid = "fce5fe82-541a-59a6-adf8-730c64b5f9a0" | |||
| version = "0.13.0" | |||
| version = "0.13.1" | |||
There was a problem hiding this comment.
We should make sure that we haven't introduced any breaking changes since 0.13.0. (IMO we should adopt the ColPrac practice of making patch releases for every PR).
There was a problem hiding this comment.
I'll just bump it up to 0.14.0. Honestly at this point we should consider moving to 1.0 as well.
There was a problem hiding this comment.
More importantly, current 0.13.0 is by default failing so ] add Turing and using Turing will fail. I think maybe you guys want to bump up the version really soon...
There was a problem hiding this comment.
We first have to fix the bug introduced by the changes in PDMats 0.10 on master before releasing 0.14.0. What was your package setup that failed, i.e. can you post the output of ] st?
There was a problem hiding this comment.
Oh, I have mine on Turing#master, but there are more than one guys on slack that faces an issue: ] add Turing installs an older version and using Turing somehow fails.
There was a problem hiding this comment.
Usually these problems are caused by unbounded compatibilities of old Turing versions (there are many closed issues in the repo here). These issues should be fixed by running ] add [email protected] and possibly adjusting conflicting packages (by users) and adding correct bounds in the registry (by us). I fixed some bounds a while ago, but it seems the old version are still missing some compatibilty bounds.
src/modes/ModeEstimation.jl
Outdated
| @@ -147,6 +147,50 @@ function (f::OptimLogDensity)(z) | |||
| return -DynamicPPL.getlogp(varinfo) | |||
| end | |||
|
|
|||
| function (f::OptimLogDensity)(F, G, z) | |||
There was a problem hiding this comment.
I'm not sure if it's useful to keep this separate definition? It seems we only need f(F, G, H, z), so the implementation could just be included there directly.
|
It seems that there is nothing blocking this PR, I think? |
|
Tests fail currently. |
|
I'll take a look when I'm done teaching today, looks like I've done something strange to the methods. |
Codecov Report
@@ Coverage Diff @@
## master #1369 +/- ##
==========================================
+ Coverage 66.79% 66.95% +0.16%
==========================================
Files 25 25
Lines 1605 1619 +14
==========================================
+ Hits 1072 1084 +12
- Misses 533 535 +2
Continue to review full report at Codecov.
|
Currently, MLE/MAP is not using the AD gradients calculated with
gradient_logp, as noted in #1365. This PR modifies the MLE/MAP code to use the AD-generated gradient.I incremented the version number to 0.13.1, but this might have to be 0.14.0 since we are technically dropping support for second-order methods with this PR. I intend to follow up in a separate PR with code that adds a
hessian_logpmethod so people can useOptim.Newtonor whatever -- it'll also make the information matrix calculations much better to use the AD-Hessian.