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

Replace recursive topological sort with iterative algorithm #25

Merged
merged 1 commit into from
Oct 10, 2018
Merged

Replace recursive topological sort with iterative algorithm #25

merged 1 commit into from
Oct 10, 2018

Conversation

phyrwork
Copy link
Contributor

No description provided.

Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@phyrwork Thank you for filing this PR, I agree that this change makes perfect sense!

Can you add some test cases to verify this works as expected?

* the topologic sorting may be non-unique depending on your edges. this
* algorithm tries to keep the order of vertices as added to the graph in
* this case.
* the topological sorting may be non-unique depending on your edges
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this removed intentionally? If so, should this be considered a BC break? Can you add some test cases for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it should be considered BC - I'm not sure the topsort should not have made any potential promises about the ordering in the first place.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure the topsort should not have made any potential promises about the ordering in the first place.

I agree that it should not have made this promise in the first place.

That being said, I suppose this is something some projects may currently rely on it. As such, I'm marking this as a BC break just in case and will merge this as-is.

Again, thank you! 👍

@clue clue added this to the v0.9.0 milestone Oct 10, 2018
@clue clue merged commit 9a0f93b into graphp:master Oct 10, 2018
@clue clue modified the milestones: v0.9.0, v0.8.1 Feb 17, 2020
@clue
Copy link
Member

clue commented Feb 17, 2020

Just for my information, why do you reverse the vertices? Is that because you want to loosely keep the order at which they were added to the graph?

@Gillu13 Yes, the original concept aimed to keep the original order. This is not currently documented, so I do not consider this to be a BC break and have just changed the milestone accordingly 👍

@clue clue removed the BC break label Feb 17, 2020
@clue clue modified the milestones: v0.8.1, v0.8.2 Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants