Revise Acyclic.AdjacencyMap

snowleopard / alga

Algebraic graphs

MIT License

717 stars 68 forks source link

Revise Acyclic.AdjacencyMap #223

Closed snowleopard closed 5 years ago

snowleopard commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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