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

Update Data.Graph documentation #923

Merged
merged 3 commits into from
Feb 1, 2023
Merged

Conversation

meooow25
Copy link
Contributor

@meooow25 meooow25 commented Jan 31, 2023

  • Add time complexities for Data.Graph functions.
  • Avoid the Forest synonym as it can be confusing. We stopped using it in Data.Tree in Stop using the Forest type synonym #665.
  • Document expected input graph properties for topSort, reverseTopSort and bcc.

For topSort and reverseTopSort the graph is expected to be acyclic.
For bcc the graph is expected to be undirected.
@meooow25
Copy link
Contributor Author

One more for the release 😄

@@ -383,8 +383,8 @@ graphFromEdges' x = (a,b) where
(a,b,_) = graphFromEdges x
{-# INLINABLE graphFromEdges' #-}

-- | Build a graph from a list of nodes uniquely identified by keys,
-- with a list of keys of nodes this node should have edges to.
-- | \(O((V+E) \log V)\). Build a graph from a list of nodes uniquely identified
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it $O(V + E \log V)$?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The sort step is $O(V \log V)$, then the graph takes $O(V + E \log V)$ to build, so the total seems right to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're absolutely right. I got my edges and vertices crossed for a moment.

--
-- Note: A topological sort exists only when there are no cycles in the graph.
-- If the graph has cycles, the output of this function will not be a
-- topological sort. In such a case consider using 'scc'.
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to mention that reverseTopSort is (currently?) faster.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because of the reverse? That seems avoidable. We build the list from the tree so we should also be able to build the reverse of the list directly. I can make a PR later.

@treeowl
Copy link
Contributor

treeowl commented Feb 1, 2023

I'm going to merge this for the release, but I think my comments should be considered for the next one.

@treeowl treeowl merged commit c8e6e8b into haskell:master Feb 1, 2023
@meooow25
Copy link
Contributor Author

meooow25 commented Feb 1, 2023

Thanks for the fast merge!

@meooow25
Copy link
Contributor Author

meooow25 commented Feb 4, 2023

I think we can close #157 and #648 from this PR, @treeowl?

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.

None yet

2 participants