Remove Optim.jl interface + minor tidying up of src/optimisation/Optimisation #2708
Conversation
penelopeysm
force-pushed
the
py/no-optim
branch
from
6409018 to
02b1f48
Compare
|
Turing.jl documentation for PR #2708 is available at: |
There was a problem hiding this comment.
I checked and the base optimisation tests are a superset of these tests, so no need to incorporate any of these deleted tests into the main suite.
| function OptimLogDensity( | ||
| model::DynamicPPL.Model, | ||
| getlogdensity::Function, | ||
| vi::DynamicPPL.AbstractVarInfo; | ||
| adtype::ADTypes.AbstractADType=Turing.DEFAULT_ADTYPE, | ||
| ) | ||
| ldf = DynamicPPL.LogDensityFunction(model, getlogdensity, vi; adtype=adtype) | ||
| return new{typeof(ldf)}(ldf) | ||
| end | ||
| function OptimLogDensity( | ||
| model::DynamicPPL.Model, | ||
| getlogdensity::Function; | ||
| adtype::ADTypes.AbstractADType=Turing.DEFAULT_ADTYPE, | ||
| ) | ||
| # No varinfo | ||
| return OptimLogDensity( | ||
| model, | ||
| getlogdensity, | ||
| DynamicPPL.ldf_default_varinfo(model, getlogdensity); | ||
| adtype=adtype, | ||
| ) | ||
| end |
There was a problem hiding this comment.
this is just duplicating LogDensityFunction code. I thought it simpler to just construct the LDF and then wrap it.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## breaking #2708 +/- ##
=============================================
- Coverage 86.45% 56.79% -29.67%
=============================================
Files 21 20 -1
Lines 1418 1340 -78
=============================================
- Hits 1226 761 -465
- Misses 192 579 +387 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| ## Optimisation interface | ||
|
|
||
| The Optim.jl interface has been removed (so you cannot call `Optim.optimize` directly on Turing models). |
There was a problem hiding this comment.
I suggest that we define our own optimize function on top of DynamicPPL.LogDensityFunction to avoid the breaking change. The optimize function provides a natural alternative to AbstractMCMC.sample.
There was a problem hiding this comment.
I don't really agree:
-
What is the purpose of avoiding the breaking change? The functionality is already present in
maximum_likelihood(model)ormaximum_a_posteriori(model)so this is just duplicate functionality. -
A
Turing.optimizefunction would still be a breaking change because this is not the same asOptim.optimize.
There was a problem hiding this comment.
Thanks, Penny, for clarifying.
My main point is not breaking change, instead optimise mirrors the Turing.sample API well, and provides a generic interface for optimisation, and VI algorithms. Sorry for the confusion. Though I agree we ought to remove the Optim interface and define our own optimize method.
There was a problem hiding this comment.
Sure, in that case we could start by renaming estimate_mode to optimize, and exporting it. That would give us out of the box
optimize(model, MLE(); kwargs...)
optimize(model, MAP(); kwargs...)and then the VI arguments could be bundled into a struct that would be passed as the second argument.
notes in comments.
Further refactoring will happen, but in separate PRs.
Closes #2635.