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 tarjan algorithm #409

Merged
merged 1 commit into from
Apr 2, 2024
Merged

Conversation

edogawashinichi
Copy link
Contributor

All 300 unit tests passed.

To explain the correctness of the modified version:

For any node, discoveryTime[node] >= lowestDisc[node].
PROOF:
because lowestDisc is initialized with discoveryTime,
and may be updated by smaller values.

If a node finishes its dfs_helper, its lowestDisc is the true value,
otherwise its lowestDisc may be greater than the true value.

Therefore the first time to update lowestDisc[curNode] in file Tarjan_impl.hpp shall be unchanged.
PROOF:
because in the process of the node's dfs_helper,
lowestDisc[node] may be updated smaller by lowestDisc of any of its neighbor node,
after the neighbor node finishes its dfs_helper.

If we are in curNode's dfs_helper, and neighborNode we are traversing is already discovered, then
neighborNode doesn't finish its dfs_helper when the graph is undirected,
and hence lowestDisc[neighborNode] may be greater.

PROOF by contradiction:
suppose to the contrary that neighborNode finishes its dfs_helper.
Noting that neighborNode and curNode are adjacent in the undirected graph,
and that discoveryTime[neighborNode] < discoveryTime[curNode],
we can conclude that curNode must also finish its dfs_helper,
violating that we are in curNode's dfs_helper.

For the second time to update lowestDisc[curNode] in file Tarjan_impl.hpp,
replacing lowestDisc[neighborNode] with discoveryTime[neighborNode] is correct
when the graph is undirected.

PROOF:
we can verify each case.
case TARJAN_FIND_BRIDGE.
####if (discoveryTime[curNode] < lowestDisc[neighborNode]) then curNode-neighborNode is a bridge.
####if (discoveryTime[curNode] < lowestDisc[neighborNode]) is false for all curNode's neighbors, then
########curNode-neighborNode is not a bridge.
case TARJAN_FIND_CUTV.
####if (curNode not root node and discoveryTime[curNode] <= lowestDisc[neighborNode]), then
########curNode is a cut vertex. (This case needs discreetly consider the traversal order of nodes.)
####otherwise of course holds true.
case TARJAN_FIND_VBCC.
####similar to case TARJAN_FIND_CUTV.
case TARJAN_FIND_SCC or TARJAN_FIND_EBCC.
####similar to case TARJAN_FIND_CUTV.

The above pipeline only demonstrates the correctness of any TASK for UNDIRECTED graph.
For other TASKs with DIRECTED graph and neighborNode inStack,
it needs to be considered carefully,
and perhaps a better choice is to add more unit tests.

@edogawashinichi
Copy link
Contributor Author

edogawashinichi commented Apr 1, 2024

The following is some references.

  • TARJAN_FIND_VBCC

https://www.geeksforgeeks.org/biconnected-components/
uses low[u] = min(low[u], disc[v]),
for the second time to update lowestDisc[curNode].

https://www.hackerearth.com/practice/algorithms/graphs/biconnected-components/tutorial/
uses an equivalent version
else if parent[vertex] != i AND disc[i] < low[vertex] low[vertex] = disc[i].

image

https://www.cs.cmu.edu/~15451-f23/lectures/lecture11-dfs-bicon.pdf
is the same solution.

  • TARJAN_FIND_BRIDGE

https://cp-algorithms.com/graph/bridge-searching.html
uses if (visited[to]) { low[v] = min(low[v], tin[to]); }

  • TARJAN_FIND_CUTV

https://cp-algorithms.com/graph/cutpoints.html
uses if (visited[to]) { low[v] = min(low[v], tin[to]); }

  • TARJAN_FIND_SCC

https://en.wikipedia.org/wiki/Tarjan%27s_strongly_connected_components_algorithm
uses v.lowlink := min(v.lowlink, w.index)

  • TARJAN_FIND_EBCC

@ZigRazor ZigRazor self-requested a review April 2, 2024 06:51
@ZigRazor ZigRazor linked an issue Apr 2, 2024 that may be closed by this pull request
Copy link

codecov bot commented Apr 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.88%. Comparing base (1b31e63) to head (3983db4).
Report is 16 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #409      +/-   ##
==========================================
+ Coverage   97.58%   97.88%   +0.29%     
==========================================
  Files          87       87              
  Lines        9492    10061     +569     
  Branches        0      660     +660     
==========================================
+ Hits         9263     9848     +585     
+ Misses        229      213      -16     
Flag Coverage Δ
unittests 97.88% <100.00%> (+0.29%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@ZigRazor
Copy link
Owner

ZigRazor commented Apr 2, 2024

Well Done!

@ZigRazor ZigRazor merged commit d2de092 into ZigRazor:master Apr 2, 2024
27 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TarjanTest.test_4 failed
2 participants