-
Notifications
You must be signed in to change notification settings - Fork 13
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
Algorithms for Network Interdiction (flow) #6
Conversation
|
||
include("matching/matching.jl") | ||
include("spectral/spectral.jl") | ||
include("datasets/Datasets.jl") | ||
include("community/detection.jl") | ||
include("interdiction/Interdiction.jl") |
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.
filenames need to be lowercase (yes, we need to change Datasets 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.
OK. I was not sure since I was starting a new submodule.
Do you want me to changeDatasets.jl too? It is not a hard task!
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 changed Datasets already :)
54cd368
to
5d71e5d
Compare
Current coverage is 100% (diff: 100%)@@ master #6 diff @@
===================================
Files 4 12 +8
Lines 78 167 +89
Methods 0 0
Messages 0 0
Branches 0 0
===================================
+ Hits 78 167 +89
Misses 0 0
Partials 0 0
|
Correcting coverage sorry >< |
Update PR: changes required and bug in BMILP Update PR: coverage 100%
5d71e5d
to
903179b
Compare
wow, this is huge, great work! Don't worry about the docs, we should use Documeter.jl for this package in the near future |
Thank you @CarloLucibello. As I said in #5, I still have several algorithms to implement. And I have work for 2-4 more articles of improvement to implement here in LightGraphsExtras, so you will have a lot of PRs in the coming academic year :) |
@CarloLucibello can you do a technical review? I'll merge as soon as you say it's OK. |
this is really too big for me to review accurately in reasonable time. At a first glance I couldn't detect anything odd, and it seems the guy knows what he is doing :) Given these premises, and unless @jpfairbanks finds the time to give this a look, for me it's OK |
Just to give an insight on the accuracy of the algorithms. The one called Multilink-Attack was adapted once in C++ and twice in Julia (in NetworkFlows.jl, that I will not maintain since it is now fully integrated in JuliaGraphs with this PR). The second one, the BMILP is new work that will be submitted soon and is coherent with the Multilink-Attack (MLA) algorithm. Basically, we can check: MLA Lower Bound <= BMILP lower bound <= BMILP upper bound <= MLA upper bound. That is one of the test in the test file actually. Sorry for the long post. Will update the indentation thing in the PR in a minute. |
I don't want to wait to merge this. Let's go ahead. This is great work, @Azzaare - thanks. |
I updated the indentation, but it can be merge in my next update on the interdiction module anyway. Thank you @sbromberger |
Here is the PR discussed in #5. Coverage should be 100%, however some cases are covered by dummy functions. Should be updated during September.
I updated the doc, though it seems to be different from
LightGraphs.jl
(no build.jl script), so please let me know if I have to do something more about it.