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

Remove components sort, Kosaraju should be O(n) #1330

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

chubakueno
Copy link

sort(component.begin(), component.end()) is linearithmic, which breaks the expected linear complexity of Kosaraju. Line is also superfluous and unnecesary.

sort(component.begin(), component.end()) is linearithmic, which breaks the expected linear complexity of Kosaraju. Line is also superfluous and unnecesary.
@mhayter
Copy link
Contributor

mhayter commented Sep 3, 2024

I'm shocked that someone rewrote this and made the code worse... It was correct before it was updated 2 months ago. You're right that there's no need to sort.

@el-sambal
Copy link
Contributor

el-sambal commented Sep 3, 2024

Sorry, I didn't think about that. The line should indeed be removed, but this sentence should then also be modified (e.g. by removing the word 'sorted'):

When building the adjacency list of the condensation graph, we select the root of each component as the first vertex in its sorted list of vertices (this is an arbitrary choice).

Thanks for catching this mistake!

Modify description to match the removal of sort() in algorithm.
@chubakueno
Copy link
Author

@el-sambal suggestion taken, updated the description!

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.

3 participants