Open
Conversation
dfb2612 to
1f46844
Compare
3110b0c to
5fae321
Compare
james-d-mitchell
requested changes
Mar 31, 2022
Member
james-d-mitchell
left a comment
There was a problem hiding this comment.
@finnbuck looks great! Lots of small things to fix, but generally this is good! Some additional things:
- please also add some tests when the arguments are mutable digraphs, I don't think this'll work quite like you expect (see comments in the PR)
- Add a version that takes 4 digraphs,
D1,D2, andSand a transformationmap1(embeddingSinD1), then computesmap2(an embedding ofSinD2) and callsDigraphAmalgam(D1, D2, S, map1, map2); - Add a version that takes 3 digraphs:
D1,D2andSand computesmap1andmap2(embeddings intoD1andD2resp), and callsDigraphAmalgam(D1, D2, S, map1, map2); - Add a version that takes 2 digraphs
D1andD2, and callsx := MaximumCommonSubdigraph(D1, D2);and then returnsDigraphAmalgam(D1, D2, x[1], x[2], x[3]);
Contributor
Author
|
@james-d-mitchell thanks for the review, I'll get to work on that! |
a604293 to
49e5ef5
Compare
f10e594 to
2528c7b
Compare
3f5ce71 to
5e70d76
Compare
74b3afd to
0551db9
Compare
Collaborator
|
I've squashed and rebased this on the latest |
24760a9 to
f5e6563
Compare
f5e6563 to
155fd3e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Based on Issue #428. This pull request adds two functions: AmalgamDigraphs and AmalgamDigraphsIsomorphic.
Description:
The AmalgamDigraphs function takes as input two digraphs and two lists of vertices corresponding to two identical subdigraphs of the two input digraphs. It returns a new digraph which consists of the two input digraphs joined together by their common subdigraph in such a way that the edge connectivity between the vertices in the new digraph matches the edge connectivity of the two input digraphs.
AmalgamDigraphsIsomorphic functions similarly, but the two lists of vertices given as inputs need only describe subdigraphs that are isomorphic to one another rather than have them be precisely equal. AmalgamDigraphsIsomorphic then rearranges the second list of subdigraph vertices so that the induced subdigraphs become equal and then calls AmalgamDigraphs.
Both functions return a tuple of size two, with the first element being the output digraph and the second being a record which maps each vertex number in the second input digraph to its new vertex number in the output digraph. The mapping of the vertices of the first input digraph can be seen as the identity mapping.