-
Notifications
You must be signed in to change notification settings - Fork 26
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
Separated the logic of branch and bound search #97
Separated the logic of branch and bound search #97
Conversation
Codecov Report
@@ Coverage Diff @@
## master #97 +/- ##
==========================================
- Coverage 59.58% 56.17% -3.41%
==========================================
Files 11 12 +1
Lines 532 623 +91
==========================================
+ Hits 317 350 +33
- Misses 215 273 +58
Continue to review full report at Codecov.
|
This looks great, thanks a lot! I am a bit worried about the performance implications of using a tree like that. Have you done any benchmarks? In general, I think we should try to leave as general as possible the data structures used, so you could specify that you wanted to store the nodes in a tree, or a vector as before, etc. How possible would that be? |
So I quickly added some benchmark using the Smiley and Chun examples introduced by @gwater and the Master:
This PR:
It looks like this PR implies about 5% to 10% penalty ( On the other hand I was expecting way more allocations with this PR. Good surprise I guess. As for allowing to store the data in arbitrary structure, while designin this PR I considerd that the ability to easily convert the result toward any data structure would be sufficient. It is also the simplest solution from the developper side. However if the current penalty we pay to keep maximal information about the run of the algorithm is too high (seems reasonnable to me, but I essentially use the package to solve rather simple equations so I may not be representative), it is possible to make it more generic (may become tricky thought). |
Wow, that's great! Let me look over this in detail then. |
Do you have a simple example to show how to use the new functionality / see the tree etc? I have been thinking that |
I updated the example in the By default, the intermediate results are not stored in the tree to spare memory. However, one of the example show how to do this by changing the Also the search object already store all final parameters (final in the sense that the contractor is stored, but not the input parameter such as the original function or if it uses automatic differentiation for example). |
Sorry, this now has conflicts :/ |
src/branch_and_bound.jl
Outdated
tuples `(node_id, lvl)` where `lvl` is the depth of the node in the tree. | ||
""" | ||
struct BBTree{DATA} | ||
nodes::Dict{Int, Union{BBNode, BBLeaf{DATA}}} |
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 there a way of avoiding this Union
?
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.
It is possible to have two dict: one for the branchings (BBNode
) and one for the leaves (BBLeaf{DATA}
).
I can attempt to rebase this if you would like. |
@dpsanders No need to, thanks. I'll do it soon. |
Allow to have type predictability for the full iteration cycle.
- Introduce default strategies - Rework interface to allow using them - Changes break tests for complex Krawczyk method which expectation has been lowered
ec42814
to
44f8a0c
Compare
Below is the benchmark of the current state of this PR against master. For now we see a slowdowns ranging from 5% to 20% in worst cases. I'll try to identify if there is a bottleneck and if I can do something to speed things up. The decrease in memory usage is a welcomed suprise. Benchmark Report for IntervalRootFindingJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
|
It seems like most of the overhead comes from the creation of the tree and the associated dictionnaries. However due to the profiler sometime crashing on Windows (JuliaLang/julia#30902) I have not yet been able to do extensive profiling. Also note that the build failures only concern Julia 0.7. |
I would say that we should just remove support for Julia 0.7 and merge this. |
Since the implementation is accepted, I can now do the tedious but necessary secondary work (add tests, check the examples are still valid and write some beautiful documentation). Hopefully I will be able to do it soon and then it will be ready for merging. |
@dpsanders This is now ready. |
Could you please update REQUIRE to put Julia 1.0 as the lower bound. |
@dpsanders Done. I've also remove 0.7 build from travis and appveyor. |
This looks really great, thanks! I think the best thing to do is merge once green and then iterate later if necessary. |
I think some of the printing etc. could probably be simplified using https://github.com/Keno/AbstractTrees.jl I'm not sure if there are any ready-made packages for trees that could simplify some of the tree logic itself. |
OK I'm going to merge for now. Thanks a lot! |
@dpsanders When I started working on this PR I checked for such packages. The closer which seems available are networks from packages such as I'll look into simplifying some part of this with |
Yes, true, |
I finally found the courage to update to 0.7/1.0 and to implement the idea proposed by @dpsanders in #72 to separate the logic of the search from the rest of the package. Therefore this PR superseed #72 (I do not update it though, as the scope is quite different).
Changes in this PR:
roots
function still return a simple list of intervals.rootsearch_iterator.jl
file, so the transition to the code in another package can be done easily.Root
object during the search rather than interval, which is forced by the genericity of the branch and bound algorithm (status of roots does not exactly match with the status of the elements in a search).