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

Revise Acyclic.AdjacencyMap #223

Merged
merged 7 commits into from
Jul 21, 2019
Merged

Revise Acyclic.AdjacencyMap #223

merged 7 commits into from
Jul 21, 2019

Conversation

snowleopard
Copy link
Owner

@snowleopard snowleopard commented Jul 20, 2019

This PR includes:

  • A lot of changes to comments, examples, implementation and tests.
  • Renaming disjointOverlay to union and disjointConnect to join, which are commonly used terms.
  • Adding some missing functions: isSubgraphOf, preSet, postSet.
  • Removal of PartialOrder: I'm still hesitating about providing an unsafe construction method.
  • Moving Acyclic.Ord to Acyclic.AdjacencyMap.Ord, and removing some functions from the API.

@adithyaov
Copy link
Contributor

A lot of changes to comments, examples, implementation and tests.

I apologize, I believe I should have done these properly. Please let me know during the PR review whether the format of the tests is proper, I'll change them accordingly.

@snowleopard
Copy link
Owner Author

I apologize, I believe I should have done these properly.

No need to apologise! It's much easier to do a revision than to start from scratch :)

Thanks again for your earlier work, and I hope you agree with my changes. If not, please let me know!

@snowleopard
Copy link
Owner Author

@adithyaov I think I'm finished with this revision.

I didn't implement the shrink idea here, but I think we should do it at some point. If shrink works well then we could probably drop Acyclic.AdjacencyMap.Ord to keep the API small.

@snowleopard
Copy link
Owner Author

@adithyaov I'd appreciate if you have a quick look at my changes. I'd like to merge as soon as possible to avoid blocking other PRs.

@adithyaov
Copy link
Contributor

@snowleopard I went over most of the files, it looks good to me. I did not cross-check the changes in the run time complexity yet. I'll go through this PR more thoroughly and get back to you by tomorrow.

@snowleopard
Copy link
Owner Author

@adithyaov Thank you! I'll wait with merging until tomorrow.

I did not cross-check the changes in the run time complexity yet.

I think I only fixed an error that I had in the documentation of induce where for some reason I didn't include the number of vertices n into the bound.

@adithyaov
Copy link
Contributor

@snowleopard I went through all the files. There seems to be a little inconsistency in the examples of Acyclic.AdjacencyMap.Ord.edge and the corresponding test cases. There seem to be a lot more test cases than examples.

Other than that everything seems to be fine.

@snowleopard
Copy link
Owner Author

@adithyaov Many thanks for having a look! I've fixed the inconsistency you found.

@snowleopard snowleopard merged commit d489e93 into master Jul 21, 2019
@snowleopard snowleopard deleted the revise-acyclic branch July 21, 2019 22:08
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.

2 participants