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 DigraphCartesianProduct and DigraphDirectProduct methods #228

Merged
merged 12 commits into from
Oct 2, 2019

Conversation

reiniscirpons
Copy link
Collaborator

@reiniscirpons reiniscirpons commented Aug 16, 2019

This PR implements two types of digraph product as discussed in #226 for a collection of digraphs. DigraphCartesianProduct computes the Cartesian product of a collection of digraphs and DigraphDirectProduct computes the direct product of a collection of digraphs. It also includes two new attributes DigraphCartesianProductProjections and DigraphDirectProductProjections for storing projections onto the coordinates of vertices.

Since the vertex set of DigraphCartesianProduct(G, H) is the Cartesian product of the vertex sets of G and H, but the Digraphs package does not permit a vertex set to contain pairs of integers, only integers themselves, then we encode a pair (i, j) as i + n * (j-1) where n = DigraphNrVertices(G).

If D = DigraphCartesianProduct(gr_1, gr_2, ...) then DigraphCartesianProductProjections(D) returns a list of transformations, where the i-th transformation corresponds to a projection onto a copy of gr_i contained within D. The projections are useful as they allow for us to reconstruct (under some circumstances) the graphs used for creating the product, and also provide for a way of regaining the information of the coordinates of each vertex (which were lost when we decided to encode (i, j) as i + n * (j-1)).

To make sure my code works with mutable as well as immutable digraphs, I used DigraphJoin as a basis, changing only the moving parts responsible for creating the digraph and leaving the more technical bits untouched. It seems like this has worked, but I would appreciate if someone told me whether everything is sound with regards to mutability.

Not exactly sure about the time complexity. For two graphs I think it should be O(E_1 + E_2) for Cartesian product and O(E_1 * E_2) for Direct product, where E_1, E_2 is the number of edges of each digraph.

@wilfwilson
Copy link
Collaborator

Hi @reiniscirpons, I've been making some fairly major changes to the Digraphs package as part of introducing mutable digraphs and working towards Digraph v1.0. Unfortunately that means that I've created conflicts with this PR, so this will need to be rebased.

(However, I'm not finished making the big changes, so you should probably wait until next week before resolving the conflicts.)

@wilfwilson
Copy link
Collaborator

Should be safe to rebase and resolve conflicts now.

@james-d-mitchell james-d-mitchell added enhancement A label for PRs that provide enhancements. new-feature A label for new features. labels Aug 28, 2019
@james-d-mitchell james-d-mitchell added this to the 1.0.0 milestone Sep 19, 2019
Copy link
Collaborator

@wilfwilson wilfwilson left a comment

Choose a reason for hiding this comment

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

I've done a preliminary review - I've made some comments relating to the Cartesian product stuff, but probably the analogous comments apply to the direct product stuff, so if you could make changes in both places that would be great.

doc/attr.xml Show resolved Hide resolved
doc/attr.xml Outdated Show resolved Hide resolved
doc/attr.xml Outdated Show resolved Hide resolved
doc/attr.xml Outdated Show resolved Hide resolved
doc/oper.xml Outdated Show resolved Hide resolved
doc/oper.xml Outdated Show resolved Hide resolved
doc/oper.xml Outdated Show resolved Hide resolved
doc/attr.xml Outdated Show resolved Hide resolved
doc/attr.xml Outdated Show resolved Hide resolved
doc/attr.xml Outdated Show resolved Hide resolved
doc/attr.xml Outdated Show resolved Hide resolved
Copy link
Collaborator

@wilfwilson wilfwilson left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @reiniscirpons, I have a few more :)

doc/attr.xml Outdated Show resolved Hide resolved
doc/attr.xml Outdated Show resolved Hide resolved
doc/oper.xml Outdated Show resolved Hide resolved
doc/oper.xml Outdated Show resolved Hide resolved
doc/oper.xml Outdated Show resolved Hide resolved
doc/oper.xml Outdated Show resolved Hide resolved
doc/oper.xml Outdated Show resolved Hide resolved
Copy link
Collaborator

@wilfwilson wilfwilson left a comment

Choose a reason for hiding this comment

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

Good work @reiniscirpons, thank you!

@wilfwilson wilfwilson merged commit de1a008 into digraphs:master Oct 2, 2019
@wilfwilson
Copy link
Collaborator

Whoops, it seems that GitHub has some key commands that I accidentally pressed, which meant that I accidentally merged this before the tests had finished. Thankfully I'm sure they were going to pass anyway!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A label for PRs that provide enhancements. new-feature A label for new features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants