-
Notifications
You must be signed in to change notification settings - Fork 219
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
Add Base.get method for ModeResult #2269
Conversation
using Random: Random | ||
using Optimization | ||
using Optimization: Optimization | ||
using OptimizationBBO: OptimizationBBO | ||
using OptimizationNLopt: OptimizationNLopt | ||
using OptimizationOptimJL: OptimizationOptimJL | ||
using ReverseDiff: ReverseDiff |
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.
This is unrelated to the other changes in this PR. Not having this here before was just a plain bug.
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 @mhauru -- looks very good to me.
src/optimisation/Optimisation.jl
Outdated
Base.get(m::ModeResult, var_symbols::Vector{Symbol}) | ||
|
||
Return the values of all the variables with the symbol(s) `var_symbol` in the mode result | ||
`m`. The return value is a `NamedTuple` with `var_symbols` as the key(s). |
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.
Intuitively I would assume that indexing with a Tuple
might (possibly) return a NamedTuple
. But returning a NamedTuple
for Vector
inputs seems quite surprising, and also means that not even the keys of the NamedTuple
return type can be inferred.
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 point. I changed this so that the second argument can be any iterator type, and made the single symbol version use a singleton tuple. I also stabilised the array types a bit. Let me know if you are happy with this.
As a fun side effect of this PR, I ran into this case of bizarrely slow type inference: JuliaLang/julia#54879 |
Thanks @mhauru and @devmotion! |
Answers the feature request from #1417
The return type here is a bit questionable. It currently mimics the
NamedTuples
that MCMCChains returns forBase.get
, but I also considered returningNamedArrays
like theModeResult.values
field. Opinions on this are welcome.Fix #1417