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

Add topological sort #2467

Merged
merged 12 commits into from
Apr 20, 2023
Merged

Add topological sort #2467

merged 12 commits into from
Apr 20, 2023

Conversation

AntoinePrv
Copy link
Member

@AntoinePrv AntoinePrv commented Apr 17, 2023

  • Fix is_reachable signature
  • Make depth_first_search a free function to mirror networkx, rename it dfs_
  • Add depth first search on all nodes
  • Add preorder and postorder dfs aliases
  • Improve tests
  • Add topological sort

@AntoinePrv
Copy link
Member Author

Looks like #2472 is going well, so making this as ready.

@AntoinePrv AntoinePrv marked this pull request as ready for review April 18, 2023 15:20
@AntoinePrv AntoinePrv self-assigned this Apr 18, 2023
libmamba/tests/src/util/test_graph.cpp Outdated Show resolved Hide resolved
libmamba/tests/src/util/test_graph.cpp Show resolved Hide resolved
libmamba/include/mamba/util/graph.hpp Show resolved Hide resolved
Copy link
Member

@Klaim Klaim left a comment

Choose a reason for hiding this comment

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

LGTM

const typename Graph::adjacency_list& adjacency
)
{
status[start] = Visited::ongoing;
Copy link
Member

Choose a reason for hiding this comment

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

Is it safe to do this instead of using .at here?

Copy link
Member

Choose a reason for hiding this comment

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

If it's required that start is in boundaries of status size, maybe just add an assertion?

Copy link
Member Author

Choose a reason for hiding this comment

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

@Klaim This is a private function, but it is true that the input is not checked from the caller (dfs_raw).
That being said, node_ids are passed from the user all over the class without being checked. Should we fix that, or is that a contract?

I'm adding a couple of assert where you specified.

Copy link
Member

Choose a reason for hiding this comment

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

I think that's a contract of the functions and the asserts are enough checks yeah.

@AntoinePrv AntoinePrv merged commit 686d058 into mamba-org:main Apr 20, 2023
20 checks passed
@AntoinePrv AntoinePrv deleted the topological-sort branch April 20, 2023 13:16
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.

None yet

4 participants