Opened 4 years ago

Last modified 3 years ago

#24280 needs_work enhancement

experimental package for the 3D printing of Cayley graphs.

Reported by: kjcollins Owned by:
Priority: major Milestone: sage-8.4
Component: graphics Keywords: sagedays@icerm
Cc: jipilab, tscrim, bsalisbury1, gh-elizabethjd Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: u/kdilks/3d_cayley_graphs (Commits, GitHub, GitLab) Commit: e1ee29f14b901570c057096d8ca7b023093abdf1
Dependencies: Stopgaps:

Status badges


On the website, we have implemented a model builder for Cayley graphs of low-rank reflection groups in a series of SageCells?. We would like to use this code to create an experimental package.

Change History (15)

comment:1 Changed 4 years ago by kjcollins

Package code currently at this link: Model class is in cayley_model.sage.

We would like to include this as an addition to sage/plot/plot3d, if it's appropriate.

comment:2 Changed 4 years ago by jipilab

  • Cc jipilab added

comment:3 Changed 4 years ago by kjcollins

  • Branch set to u/kjcollins/3d_cayley_graphs

comment:4 Changed 4 years ago by kjcollins

  • Commit set to 80faffa9b4ea115f02aafbc5c6eafdfa76144d65
  • Milestone changed from sage-8.1 to sage-8.3
  • Status changed from new to needs_info

New commits:

80faffauploading package to ticket

comment:5 Changed 4 years ago by jipilab

In order for the bot to test your ticket, it is required to have the field "Authors" filled with the full names of the authors

comment:6 Changed 4 years ago by jipilab


I had a look at the code, here are already some things to have a look at:

  • The module should follow the general coding convention of sage, for example the indentation:

Basic plot of a reflection group::     <---- double colon here.
             <------ line break here
    sage: ReflectionGroup(['A',3])                             # optional - gap3
    Irreducible real reflection group of rank 3 and type A3
    sage: w = ReflectionGroup(['A',3])                         # optional - gap3
    sage: ReflectionGroup3d(w)
    Rigid graphical representation of Irreducible real reflection group of rank 3 and type A3
    sage: g = ReflectionGroup3d(w)
    sage: g.plot3d()
    Graphics3d Object

for more details, see here . Further, the first line of test above is not needed.

  • The examples given in the documentation should work out-of-the-box, that is, running sage -t sage/src/sage/plot/plot3d/ should not return failures (currently has 120, for example, you should not forget to import the relevant function like ReflectionGroup in the preamble of the file.)
  • The name of the class is perhaps not so well-chosen, perhaps cayley_graph3d? Which leads to a potential follow-up question: could this work eventually for general groups?
  • General question: do you really need gap3? You can require gap3, but then should be nice to the user and say it at in the docstring of the module.
  • Sage does not usually privilege the usage of user warnings. It should be explained in the docstring of the function what is done, with an example.
  • About the Jmol bug, could you give a minimal working instance of the bug?

That's what I can see for now. If you could work on the above list, then I can continue to give feedback once new versions come along!

Best, JP

comment:7 Changed 4 years ago by jipilab

  • Status changed from needs_info to needs_work

comment:8 Changed 4 years ago by tscrim

  • Cc tscrim added
  • Component changed from packages: experimental to graphics

Is there something specific about ReflectionGroup that you need to use? Why not WeylGroup or CoxeterGroup (when the Cartan/Coxeter? type is a finite type)?

comment:9 Changed 4 years ago by bsalisbury1

  • Cc bsalisbury1 added
  • Keywords sagedays@icerm added
  • Milestone changed from sage-8.3 to sage-8.4

comment:10 Changed 3 years ago by kdilks

  • Branch changed from u/kjcollins/3d_cayley_graphs to u/kdilks/3d_cayley_graphs

comment:11 Changed 3 years ago by kdilks

  • Cc gh-elizabethjd added
  • Commit changed from 80faffa9b4ea115f02aafbc5c6eafdfa76144d65 to c62a7925a58ce0aed29eb40fec9d2ab7d8bce9df

New commits:

b4b9a58Merge branch 'u/kjcollins/3d_cayley_graphs' of git:// into 24280CayGraph
c62a792Addressing reviewer changes, one doctest is spitting out magic doctest errors before giving right result.

comment:12 Changed 3 years ago by kdilks

The problem is that having


which is meant to bring up warnings for 3 specific doc-tests is causing a normal doc-test to throw hundreds of warnings. Removing that import just causes those 3 specific doc-tests to return a blank line instead of a warning, so that 3 doc-tests will just be modified/removed.

comment:13 Changed 3 years ago by git

  • Commit changed from c62a7925a58ce0aed29eb40fec9d2ab7d8bce9df to 798dd74e518a692c4ac0967adec128f0693592d2

Branch pushed to git repo; I updated commit sha1. New commits:

798dd74Pushing changes from my computer since it's not working in the cloud

comment:14 Changed 3 years ago by jipilab


without the gap3 optional package, I still get 3 doctest failures:

File "", line 63, in sage.plot.plot3d.reflection_group3d
Failed example:
    g_plot = cayley_graph_3d(A1A1, point=(21,11,31))
Expected nothing

File "", line 212, in sage.plot.plot3d.reflection_group3d.cayley_graph_3d._real_dimension
Failed example:
    A = cayley_graph_3d(W)

File "", line 213, in sage.plot.plot3d.reflection_group3d.cayley_graph_3d._real_dimension
Failed example:
Exception raised:

The last two are caused by a missing flag # optional - gap3.

Question: are those warnings _really_ important? This is not typical to have warnings issued to the user like this in Sage.

Instead, perhaps writing a warning block inside of the class to tell the user what the hidden behavior is might be a good idea, as suggested by the Sage Developer's manual:

comment:15 Changed 3 years ago by git

  • Commit changed from 798dd74e518a692c4ac0967adec128f0693592d2 to e1ee29f14b901570c057096d8ca7b023093abdf1

Branch pushed to git repo; I updated commit sha1. New commits:

19dbf74Merge branch 'u/kdilks/3d_cayley_graphs' of git:// into 24280cayley3d
e1ee29fAll tests are now passing on my cocalc version of sage devel, both with and without the optional gap3. The user warnings have been removed and a warning block about requiring gap3 has been added.
Note: See TracTickets for help on using tickets.