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 PancakeGraph #403

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

james-d-mitchell
Copy link
Member

No description provided.

@codecov

This comment has been minimized.

@olexandr-konovalov
Copy link
Contributor

@james-d-mitchell merge conflicts now should be resolved...

doc/examples.xml Show resolved Hide resolved
gap/grape.gi Outdated
InstallMethod(CayleyDigraphCons,
"for IsMutableDigraph, group, and list of elements",
[IsMutableDigraph, IsGroup, IsHomogeneousList],
function(filt, G, gens)
if not IsFinite(G) then
ErrorNoReturn("the 1st argument <G> must be a finite group,");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here the arguments have the wrong indices if the function is called directly. Of course, this function is undocumented, but this raises the question (which may have already been answered) of whether it should be obscured using DIGRAPHS_

Copy link
Collaborator

Choose a reason for hiding this comment

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

They also have the wrong indices if CayleyDigraph is called with the filter as first argument.

gap> digraph := CayleyDigraph(IsMutableDigraph, group);
<mutable digraph with 8 vertices, 24 edges>
gap> digraph := CayleyDigraph(IsMutableDigraph, FreeGroup(1));
Error, the 1st argument <G> must be a finite group,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Example of wrong indices

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.

Nice graph! I have a few comments/suggestions/complaints.

I would much prefer it if this PR consisted of two separate commits: one for the changes to CayleyDigraph, and one for the pancake stuff.

tst/standard/grape.tst Show resolved Hide resolved
doc/examples.xml Outdated Show resolved Hide resolved
doc/examples.xml Outdated Show resolved Hide resolved
gap/examples.gd Show resolved Hide resolved
gap/examples.gi Outdated Show resolved Hide resolved
tst/standard/examples.tst Show resolved Hide resolved
@marinaanagno
Copy link
Contributor

Just a reminder, there is a pull request on this branch about the burnt pancake graph (which has to do with signed permutations). I haven't finished documented it yet, but I guess I should do this and @james-d-mitchell should approve before merging this into master, if we still want burnt pancake as a family of graphs in digraphs :)

@james-d-mitchell james-d-mitchell force-pushed the pancake branch 2 times, most recently from 2666a8f to fdb8832 Compare May 26, 2021 10:18
@wilfwilson wilfwilson changed the title examples: add pancakes Add PancakeGraph May 26, 2021
@wilfwilson wilfwilson dismissed their stale review May 26, 2021 10:39

sorted

Copy link
Contributor

@LRacine LRacine left a comment

Choose a reason for hiding this comment

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

I really enjoyed looking through your code, it was clean and concise. Your documentation was also thorough and readable. There were just a couple of capitalisation errors and I was wondering where I could see the burnt pancake documentation.

doc/examples.xml Outdated
<Returns>A digraph.</Returns>
<Description>
If <A>n</A> is a positive integer, then this operation returns
the <E>Pancake graph</E> with <M>n!</M> vertices and <M>n!(n - 1)</M>
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking great! Pancake should be pancake though (inappropriate capitalisation).

doc/examples.xml Outdated
<Description>
If <A>n</A> is a positive integer, then this operation returns
the <E>Pancake graph</E> with <M>n!</M> vertices and <M>n!(n - 1)</M>
directed edges. The <M>n</M>th Pancake graph is the Cayley graph of the
Copy link
Contributor

Choose a reason for hiding this comment

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

Pancake -> pancake

Copy link
Contributor

Choose a reason for hiding this comment

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

fixed in burnt pancake PR :)


See <URL>https://en.wikipedia.org/wiki/Pancake_graph</URL> for further
details.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there documentation for the burnt pancake graph?

Copy link
Contributor

Choose a reason for hiding this comment

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

@LRacine I have created a pull request on James's branch and if you would like to have a look I'd love to hear your comments :)
james-d-mitchell#14

Copy link
Contributor

Choose a reason for hiding this comment

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

@marinaanagno Thank you, doing so now

@github-actions
Copy link

github-actions bot commented Mar 8, 2022

Stale pull request message

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

Successfully merging this pull request may close these issues.

6 participants