New multithreaded API#43
Conversation
tpapp
left a comment
There was a problem hiding this comment.
Thanks for making this contribution. I think it will improve the package a lot but I have a few questions and suggestions.
| s = SobolSeq(lower_bounds, upper_bounds) | ||
| skip(s, N) # better uniformity | ||
| points = Iterators.take(s, N) | ||
| points = collect(Iterators.take(s, N)) |
There was a problem hiding this comment.
Is this the source (and the fix) for the type instability? I am OK with this workaround but please leave a comment, eg
| points = collect(Iterators.take(s, N)) | |
| # temporary workaround for type instability in Sobol.jl, cf | |
| # https://github.com/JuliaMath/Sobol.jl/pull/42 | |
| points = collect(Iterators.take(s, N)) |
There was a problem hiding this comment.
No, this has nothing to do with type instability. I did not introduce any kind of workaround, I just made sure that I don't introduce any new instabilities unrelated to the Sobol issue.
This is related to the way OhMyThreads does multithreading. Previously, the multithreaded iteration was
map(fetch, map(x -> @spawn(_initial(x)), points))so that the iteration over the points is done in the main thread, which then spawns starting from the point that we already got. OMT instead might wants to index (with ranges) into the points, depending on the scheduler. So basically, we need something that
a) supports an AbstractArray interface - which Iterators.take doesn't do, but this could easily be mitigated
b) can be indexed randomly - but the s is an iterator and the next points depends on the previous one. So I don't really see a good way around this pre-allocation of the points. It could be made a bit more efficiently by pre-allocating a matrix and then iterating into its columns instead of having a vector of vectors, but that's the best I can currently imagine.
| minchunksize::Union{Integer,Nothing}=nothing, | ||
| chunking::Union{Bool,Nothing}=nothing, | ||
| nchunks::Union{Integer,Nothing}=nothing, | ||
| split::Union{OhMyThreads.Split,Symbol,Nothing}=nothing) |
There was a problem hiding this comment.
It would be fine to just deprecate use_threads (issue a deprecation warning) so it can be removed in the next major release.
…threaded folding and multi-threaded foreach
|
I now made a couple of changes that should implement some or all of your comments:
1 This could be superficially avoided by also storing the local results in the task-local storage and moving this storage into a Channel, so that in the end we can retrieve the best values for all tasks and take the minimum. Since internally the channels also have to synchronize, so I don't think this would give any kind of benefit. |
…tion Be careful with mutating optimizers, don't let them invalidate previous results
This is my go for implementing a multithreaded API. Initially, I tried what you suggested in #36 and used
Folds.jl; however, this turned out to be very impractical. WithFolds.jl, you have to specify abasesizeif you somehow want to control how many tasks to spawn. But a static division strategy does not really make a lot of sense for optimizations, where the landscape could be wildly different and therefore the optimization times might vary a great deal depending on the initial point. So instead, I moved toOhMyThreads.jland use two different strategies:StaticScheduler.GreedySchedulerwith fixed number of tasks in order to keep the computer busy.Of course, this is just the default strategy, the custom options or scheduler from
OhMyThreadsmay be used to provide an own strategy.What I don't like yet, but don't see a good alternative: The possible keyword arguments are currently named explicitly. This means that once
OhMyThread.jlchanges/adds/removes a keyword argument, the list here has to be synchronized. However, given that we have just one function but need to construct two different objects, what would be the alternative?scheduler_initpointsand ascheduler_localoptinstead?Base.kwarg_declandwhichto dynamically generate the possible keyword arguments and intersect with akwargs..., plus raising an error for unused kwargs? Actually, this would work rather decently when converting themultistart_minimizationto a generated function.How relevant would this be: I hope that my PR to
OhMyThreads.jlwill receive some attention sooner or later, which would add amigrationkeyword argument to theGreedyScheduler, which should also be passed through here.What I discovered while developing this:
MultistartOptimization.jlis terribly type-unstable, which isSobol.jl's fault. I just filed a PR to fix this, after which everything, including the new parallelized code, should be statically inferrable.Closes #36