Include parameter dictionary in optimisation return value #2710
Conversation
penelopeysm
changed the title
|
Turing.jl documentation for PR #2710 is available at: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2710 +/- ##
==========================================
+ Coverage 86.45% 86.47% +0.01%
==========================================
Files 21 21
Lines 1418 1420 +2
==========================================
+ Hits 1226 1228 +2
Misses 192 192 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| """ | ||
| ModeResult{ | ||
| V<:NamedArrays.NamedArray, | ||
| M<:NamedArrays.NamedArray, |
There was a problem hiding this comment.
doesn't look consistent, probably already wrong and for a while, good to fix
There was a problem hiding this comment.
@penelopeysm sorry for review twice 😓
bump this in case you missed
There was a problem hiding this comment.
Oh, I agree the NamedArray stuff should be removed, but that would be breaking, right? I was thinking of leaving it to another time. #2634
There was a problem hiding this comment.
Also, don't ever apologise for double-reviewing!
HISTORY.md
Outdated
| model = f() | ||
| opt_result = maximum_a_posteriori(model) | ||
|
|
||
| sample(model, NUTS(), 1000; initial_params=InitFromParams(opt_result.params)) |
There was a problem hiding this comment.
small nit is that this would somewhat makes params fieldname a public interface, with the warrant just following the code example, not big deal, but I thought it might be worth mentioning
There was a problem hiding this comment.
Ah yeah, I guess we could overload InitFromParams(opt_result) instead so that you don't need to access the params field. I thought we could do that in a separate release, but doing that would avoid having to document the field, so let's bundle it in
The return value of a point optimisation currently includes an array of parameters. Arrays bad.
Dict{VarName}good. (In particular, it currently returns aNamedArraywhich is a really weird data structure!)This is not meant to be a permanent fix:
ModeResultand its associated interface IMO needs a larger overhaul. (There are a few other open issues on the optimisation interface.) But getting this fix in will at least let it be more composable with the rest of Turing, while we figure out how to fix the rest. (See changelog for examples of what I mean by composable)I reckon it's fair to not make this a breaking change because all of
ModeResult's existing fields still behave the same way, the only difference is that there is an extra field. So the only thing it would break is if people were doing something bizarre likefieldnames(...). Also, technically,ModeResultisn't exported (although I think it's effectively public since it is the return value of an exported function).