Add BishopsGraph, RooksGraph, and QueensGraph

[finnbuck]

This pull request adds constructor functions for the Bishop's, Rook's and Queen's graphs.

[tomcontileslie]

Very nice pull request with some really useful examples. There is very little redundant work thanks to clever use of other line piece graphs, along with graph products, to give the more complicated graphs. Some thought is necessary to decide what's best semantics-wise for the bishop's graph, given that in its current form, output is very convenient but doesn't quite line up with the quoted definition from Wolfram MathWorld. Whether you decide to change the output format, or instead just change the definition you give in the documentation to make it consistent with the implementation, this will be a great feature.

[wilfwilson]

I started to make many small documentation-related changes, but then I realised that it would be quicker and easier for me to just make the changes and push then to your branch. So I'm going to do just that.

[wilfwilson]

Thanks for adding these. I haven't looked at the methods closely, but there is one problem that I found (commented on below).

I also think it would be nice (although I don't insist on it) if the vertices were labelled with the co-ordinates of their square. Ideally "A1" etc - but we can't do that since there are only 26 letters but we want to make arbitrarily large graphs 🙂 If it would be easy to add, then, could you please add vertex labels like we have for KingsGraph:

gap> DigraphVertexLabels(KingsGraph(4, 7));
[ [ 1, 1 ], [ 2, 1 ], [ 3, 1 ], [ 4, 1 ], [ 1, 2 ], [ 2, 2 ], [ 3, 2 ],
  [ 4, 2 ], [ 1, 3 ], [ 2, 3 ], [ 3, 3 ], [ 4, 3 ], [ 1, 4 ], [ 2, 4 ],
  [ 3, 4 ], [ 4, 4 ], [ 1, 5 ], [ 2, 5 ], [ 3, 5 ], [ 4, 5 ], [ 1, 6 ],
  [ 2, 6 ], [ 3, 6 ], [ 4, 6 ], [ 1, 7 ], [ 2, 7 ], [ 3, 7 ], [ 4, 7 ] ]

so that the vertex with label [1,1] would correspond to the square A1, and [2,4] to B4, etc.

Changes addressed, I think.

[finnbuck]

Hi, thanks for reviewing this pull request @wilfwilson! I just wanted to quickly respond to this comment you sent a few days ago:

I also think it would be nice (although I don't insist on it) if the vertices were labelled with the co-ordinates of their square. Ideally "A1" etc - but we can't do that since there are only 26 letters but we want to make arbitrarily large graphs. If it would be easy to add, then, could you please add vertex labels like we have for KingsGraph:

gap> DigraphVertexLabels(KingsGraph(4, 7));
[ [ 1, 1 ], [ 2, 1 ], [ 3, 1 ], [ 4, 1 ], [ 1, 2 ], [ 2, 2 ], [ 3, 2 ],
  [ 4, 2 ], [ 1, 3 ], [ 2, 3 ], [ 3, 3 ], [ 4, 3 ], [ 1, 4 ], [ 2, 4 ],
  [ 3, 4 ], [ 4, 4 ], [ 1, 5 ], [ 2, 5 ], [ 3, 5 ], [ 4, 5 ], [ 1, 6 ],
  [ 2, 6 ], [ 3, 6 ], [ 4, 6 ], [ 1, 7 ], [ 2, 7 ], [ 3, 7 ], [ 4, 7 ] ]

so that the vertex with label [1,1] would correspond to the square A1, and [2,4] to B4, etc.

Basically, I was a bit surprised at first that the king's graph had vertex labels, since I do not remember programming these into that function and I could not find any later edits that added them!

After looking at it for a little bit I think the vertex labeling is coming from the use of the Cartesian product function, since the queen's and rook's graphs also have it while the bishop's graph does not (the bishop's graph function is also the only one that does not rely on the Cartesian product of two graphs).

So I've decided for consistency's sake I will try and add the vertex labeling to the bishop's graph as well (and probably also to the knight's graph in a separate pull request).

[wilfwilson]

Thanks for the reply @finnbuck, I'm sorry I broke your tests but I think I've just fixed them now (I hope). The 'problem' was that I switched the meaning of the arguments m and n for the Rooks graph, but not for the Bishops graph, and so the Queens graph (which is the combination of the two) ended up being confused. I've put RooksGraph back to how it was.

The reason I made the change is that I wanted the m x n Rooks graph (whose vertices are already labelled, as you say) to have the vertex with label [m, n]. i.e I wanted the 3x2 graph to have a vertex labelled [3,2] (corresponding to square C2 on a chessboard). I felt that this was more natural. But currently the 3x2 graph has the square labelled [2,3] instead of one labelled [3,2].

The reason that KingsGraph has labelled vertices is because it uses TriangularGridGraph, and that produces a digraph with the vertices already labelled appropriately. So it wasn't something that you had to add explicitly.

[wilfwilson]

So please feel free to give it a go, labelling the vertices of the digraphs. I'm happy to sort it out if it proves too tricky.

If we're going to do it, I really think it should be done consistently with the existing named pieces, so that the m x n piece-graph has its labels in the range from [1,1] to [m,n], and everything in between, like:

gap> DigraphVertexLabels(KingsGraph(2, 3));
[ [ 1, 1 ], [ 2, 1 ], [ 1, 2 ], [ 2, 2 ], [ 1, 3 ], [ 2, 3 ] ]

(in particular note it has the label [2,3] but not [3,2]).

Once the vertex labels are done I'll be happy for this to be merged! Thanks again.

[finnbuck]

Apologies for the wait! I think I've just about figured out how to do the vertex labels by now and will hopefully be pushing those changes later this evening...

[wilfwilson]

I'm taking a look at this again, thanks @finnbuck

[wilfwilson]

Thanks for your patience @finnbuck.

I've updated the pull request so that all five of the of the chess piece graph functions are 'consistent', in the way that they use labels that they use for vertices.

i.e. vertex 1 is always [1, 1], vertex 2 is [2, 1], vertex 3 is [3, 1], ..., vertex m is [m, 1], vertex m+1 is [1, 2], and so on.

This way, the QueensGraph(m, n) contains KingsGraph(m, n), RooksGraph(m, n), and BishopsGraph(m, n) as subdigraphs, and the vertex labels are the same in all cases.

I also reworked the documentation a bit.

Let's see if the tests pass!

[finnbuck]

Thank you @wilfwilson! Looks like all the checks have indeed passed. I've had a read through the changes and it all looks quite sensible to me. I'll try to keep in mind some of these tricks to make my code more readable and maintainable in the future!