Fix Tarjan implementation #34
Conversation
| // tarjan implements Tarjan's strongly connected component finding | ||
| // algorithm. The implementation is from the pseudocode at | ||
| // | ||
| // http://en.wikipedia.org/wiki/Tarjan%27s_strongly_connected_components_algorithm |
There was a problem hiding this comment.
It would be good to add a date accessed as part of the reference here. Wikipedia changes.
Edit: or even a link to a specific revision, like http://en.wikipedia.org/w/index.php?title=Tarjan%27s_strongly_connected_components_algorithm&oldid=647184742
|
Ugh. The current WP pseudocode doesn't even look correct. The commit comment makes the assumption that it's expensive to check if a node is on the stack and introduces the isRemoved flag. But without initialization, it's unclear that !isRemoved is a valid way to check if a node is on the stack. Instead of isRemoved, I would add a an "onStack" flag. Update: Hmm. It does seem to work. I'll stick to my claim that it isn't clear that it should work. The comment says "Successor w is in stack S" and yet the flag doesn't correspond to that condition for all nodes. Maybe it does for the node being tested at that point in the algorithm, but gosh, that's obscure. |
kortschak
force-pushed
the
tarjan
branch
2 times, most recently
from
af012fa
to
e5c6b0e
Compare
|
I have made that change. PTAL Would you edit the WP article, or would you like me to add a comment that it is with changes? I'll update the comment when I know which. |
kortschak
force-pushed
the
tarjan
branch
3 times, most recently
from
597da1a
to
3451bee
Compare
|
ping |
|
Sorry I was distracted with other things for a bit. LGTM. For the comment, I would reference the previous version at WP with "http://en.wikipedia.org/w/index.php?title=Tarjan%27s_strongly_connected_components_algorithm&oldid=642744644." Pseudocode in that version simply reads I had a WP account once. You're right I should try to recover it and suggest changes there. |
This was raised in code review for gonum/graph#34. The code was changed, but the comment was left. Now the comment points to the correct WP revision.
The previous code was giving incorrect results for a 2SAT solver I have built on it (cannot release). This fixes that.
The previous code was giving incorrect results for a 2SAT solver I have
built on it (cannot release). This fixes that.
Tests to come.