-
Notifications
You must be signed in to change notification settings - Fork 233
Add iterator interface #745
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
base: master
Are you sure you want to change the base?
Conversation
| @with_kw struct IteratorState{IT <: OptimIterator, TR <: OptimizationTrace} | ||
| # Put `OptimIterator` in iterator state so that `OptimizationResults` can | ||
| # be constructed from `IteratorState`. | ||
| iter::IT |
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.
We can drop iter field from IteratorState if we change the API to:
let istate
iter = optimizing(args...; kwargs...)
for istate′ in iter
istate = istate′
end
OptimizationResults(iter, istate) # need to pass `iter` here
endThere 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.
Accessor functions like iteration_limit_reached(istate) and (f|g|h)_calls(istate) need .iter, too.
|
Interesting, thanks. That took surprisingly few changes, but it was mostly written as an iterator already, just wasn't an actual iterator :laugh . Will have to have a more thorough look! |
Codecov Report
@@ Coverage Diff @@
## master #745 +/- ##
==========================================
- Coverage 81.46% 81.41% -0.05%
==========================================
Files 43 43
Lines 2422 2438 +16
==========================================
+ Hits 1973 1985 +12
- Misses 449 453 +4
Continue to review full report at Codecov.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #745 +/- ##
==========================================
- Coverage 85.73% 85.01% -0.73%
==========================================
Files 46 45 -1
Lines 3604 3630 +26
==========================================
- Hits 3090 3086 -4
- Misses 514 544 +30 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @unpack_IteratorState istate | ||
| @unpack d, initial_x, method, options, state = iter | ||
|
|
||
| after_while!(d, state, method, options) |
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.
Executing a mutating-function after_while! inside non-bang function OptimizationResults is not super great. But it looks like after_while! is a no-op mostly except for NelderMead so maybe it's OK? From a quick look, after_while! for NelderMead seems to be idempotent (so that, e.g., calling OptimizationResults(istate) inside a loop multiple times is OK). If that's the case, OptimizationResults(istate) practically has no side-effect?
But using a function like result! sounds good to me as well.
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.
I got rid of after_while! so no more side effects
| if !isa(r.method, NelderMead) | ||
| throw(ArgumentError("There is no centroid involved in optimization using $(r.method). Please use x_trace(...) to grab the points from the trace.")) | ||
| function centroid_trace(r::Union{MultivariateOptimizationResults, IteratorState}) | ||
| tr = trace(r) |
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.
I suppose tr = trace(r) should be added here? I think it'll throw UndefVarError otherwise. There are two more places I did this change.
I'm including these changes (adding tr = trace(r)) although they are not directly related to PR.
|
@pkofod After posted the PR, I realized that the first version didn't provide the way to access iteration state (e.g., getting "best minimizer so far") so I added accessor functions for iterator state by reusing the function names/code already defined for the result type. So the diff may not be as small as you remember now. Anyway, I think I've implemented the APIs I need. I'm looking forward to hear your thoughts on this. |
|
Aside: Using this API a few times, I realized that function Base.foreach(f, iter::OptimIterator)
local istate
for istate′ in iter
istate = istate′
f(istate)
end
return istate
endWe can then use it as result = foreach(optimizing(args...; kwargs...)) do istate
@info "Minimizing..." minimizer(istate) minimum(istate)
end |> Optim.OptimizationResultsalthough this does not let us use (Or we can implement transducer-compatible |
|
Friendly ping :) Is there anything I can do to make it easier to merge? |
|
Sorry, I've had too much on my plate. I'll take a look soon. |
|
Thanks! |
|
🛎️ Can we have this? I am still interested in plotting the evolution of the optimization : https://discourse.julialang.org/t/plotting-the-evolution-of-the-optimization-using-optim-jl/41978 |
Benchmark Results
Benchmark PlotsA plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR. |
…test to succeed locally
…se the method is not in the state
| Base.summary(r::OptimizationResults) = summary(r.method) # might want to do more here than just return summary of the method used | ||
| _method(r::OptimizationResults) = r.method | ||
|
|
||
| Base.summary(r::Union{OptimizationResults, OptimIterator}) = |
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.
Base.summary should be overloaded as Base.summary(::IO, x)
| # We can safely assume that `istate` is defined at this point. That is to say, | ||
| # `OptimIterator` guarantees that `iterate(::OptimIterator) !== nothing`. |
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.
I think JET won't agree with this comment 😄
Generally, the code above seems a bit unfortunate... Maybe optimizing should return the iterator AND the initial state?
I also wonder, is there no utility in Julia for directly obtaining the last state of an iterator?
|
|
||
| function Base.iterate(iter::OptimIterator, istate = nothing) | ||
| (; d, initial_x, method, options, state) = iter | ||
| if istate === nothing |
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.
IMO it would be cleaner to move this code to a separate Base.iterate(iter::OptimIterator) definition, I'm not a fan of "dummy" states such as nothing for the initial iteration as this is not part of the Base.iterate interface.
|
|
||
| Base.IteratorSize(::Type{<:OptimIterator}) = Base.SizeUnknown() | ||
| Base.IteratorEltype(::Type{<:OptimIterator}) = Base.HasEltype() | ||
| Base.eltype(::Type{<:OptimIterator}) = IteratorState |
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.
Is it a problem that the element type is non-concrete here? Could it be defined in a way that the eltype is concrete?
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.
Seems below TR is initialized as OptimizationTrace{typeof(value(d)),M}.
This PR adds an iterator interface for optimizer. I tentatively call it
Optim.optimizing. It provides an iterator-based API such thatis equivalent to
optimize(args...; kwargs...)for multivariate optimizations.close #599