Skip to content
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

fix mincut() #325

Merged
merged 6 commits into from
Jan 25, 2024
Merged

fix mincut() #325

merged 6 commits into from
Jan 25, 2024

Conversation

axsk
Copy link
Contributor

@axsk axsk commented Dec 27, 2023

To my understanding adj_cost stores the cut weight between the last two vertices, s and t.
If that is lower then the current best cut it becomes the new best.

I do not really understand the cutweight computation inside the mincut phase. (It reminds me more of a flow computation) but also didn't think hard enough about the new intermediate pruning.

Anyways, with this change I get the correct cut for the AoC graph (c.f. #324).
I did not test this any further, so please someone knowledgable look into it before merge

@axsk
Copy link
Contributor Author

axsk commented Dec 27, 2023

I removed the weird computation of cutweight alltogether and set it to the weight of the last cut (what was adj_weight) before.
To my understanding this is what plain Stoer-Wagner does.
I now believe the trick to remove stronger connected vertices early still works with this "simple" cutweight computation.

Copy link

codecov bot commented Dec 27, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (da6f801) 97.26% compared to head (dca0cc8) 97.28%.
Report is 2 commits behind head on master.

Files Patch % Lines
src/traversals/maxadjvisit.jl 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #325      +/-   ##
==========================================
+ Coverage   97.26%   97.28%   +0.02%     
==========================================
  Files         115      115              
  Lines        6795     6789       -6     
==========================================
- Hits         6609     6605       -4     
+ Misses        186      184       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@axsk axsk changed the title RFC: fix mincut() fix mincut() Jan 2, 2024
@gdalle
Copy link
Member

gdalle commented Jan 3, 2024

Thanks for this contribution! I will need a bit of time to review it but I fully intend to

@gdalle
Copy link
Member

gdalle commented Jan 3, 2024

Feel free to ping me if I take too long

@gdalle gdalle added the bug Something isn't working label Jan 3, 2024
@axsk
Copy link
Contributor Author

axsk commented Jan 4, 2024

Sure let me know if you have any questions.

@gdalle gdalle mentioned this pull request Jan 15, 2024
@etiennedeg
Copy link
Member

Oh well, you are right, the computation of cutweight inside the cut of the phase is unneeded.
What it does is to compute the running cut of the phase (which upper bound the mincut).
It was part of the previous incorrect implementation when the cutweght was compared to the mincut at each step of each phase.
I kept it in a first time because used in combination with my heuristic, it may save times, but according to by benchmarks, it was not worth it, so I removed it, but then, the whole cutweight calculation is unneeded.

Thus I think we can continue with this PR, I will close mine.

To my understanding adj_cost stores the cut weight between the last two vertices, s and t.
If that is lower then the current best cut it becomes the new best.

This is only true at the last step of the phase. during the phase, it is equal to $w(A_v, v)$ from wikipedia, and it gives a lower bound on the $uv$-mincut, where $u$ is the last vertex added before $v$ (which is the justification of the heuristic)

@etiennedeg
Copy link
Member

If nobody disagree, I will merge, there is nothing controversial in these changes

@etiennedeg etiennedeg merged commit a1ea44e into JuliaGraphs:master Jan 25, 2024
8 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants