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:  sage8.4 
Component:  graphics  Keywords:  sagedays@icerm 
Cc:  jipilab, tscrim, bsalisbury1, ghelizabethjd  Merged in:  
Authors:  Reviewers:  
Report Upstream:  N/A  Work issues:  
Branch:  u/kdilks/3d_cayley_graphs (Commits, GitHub, GitLab)  Commit:  e1ee29f14b901570c057096d8ca7b023093abdf1 
Dependencies:  Stopgaps: 
Description
On the website reflections.swarthmore.edu, we have implemented a model builder for Cayley graphs of lowrank 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
comment:2 Changed 4 years ago by
 Cc jipilab added
comment:3 Changed 4 years ago by
 Branch set to u/kjcollins/3d_cayley_graphs
comment:4 Changed 4 years ago by
 Commit set to 80faffa9b4ea115f02aafbc5c6eafdfa76144d65
 Milestone changed from sage8.1 to sage8.3
 Status changed from new to needs_info
New commits:
80faffa  uploading package to ticket

comment:5 Changed 4 years ago by
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
Hi,
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:
EXAMPLES: 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 https://doc.sagemath.org/html/en/developer/#writingcodeforsage . Further, the first line of test above is not needed.
 The code should also try to follow pep8 conventions: https://www.python.org/dev/peps/pep0008/
 The examples given in the documentation should work outofthebox, that is, running
sage t sage/src/sage/plot/plot3d/reflection_group3d.py
should not return failures (currently has 120, for example, you should not forget to import the relevant function likeReflectionGroup
in the preamble of the file.)
 The name of the class is perhaps not so wellchosen, perhaps
cayley_graph3d
? Which leads to a potential followup question: could this work eventually for general groups?
 General question: do you really need
gap3
? You can requiregap3
, 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
 Status changed from needs_info to needs_work
comment:8 Changed 4 years ago by
 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
 Cc bsalisbury1 added
 Keywords sagedays@icerm added
 Milestone changed from sage8.3 to sage8.4
comment:10 Changed 3 years ago by
 Branch changed from u/kjcollins/3d_cayley_graphs to u/kdilks/3d_cayley_graphs
comment:11 Changed 3 years ago by
 Cc ghelizabethjd added
 Commit changed from 80faffa9b4ea115f02aafbc5c6eafdfa76144d65 to c62a7925a58ce0aed29eb40fec9d2ab7d8bce9df
comment:12 Changed 3 years ago by
The problem is that having
warnings.simplefilter("always")
which is meant to bring up warnings for 3 specific doctests is causing a normal doctest to throw hundreds of warnings. Removing that import just causes those 3 specific doctests to return a blank line instead of a warning, so that 3 doctests will just be modified/removed.
comment:13 Changed 3 years ago by
 Commit changed from c62a7925a58ce0aed29eb40fec9d2ab7d8bce9df to 798dd74e518a692c4ac0967adec128f0693592d2
Branch pushed to git repo; I updated commit sha1. New commits:
798dd74  Pushing changes from my computer since it's not working in the cloud

comment:14 Changed 3 years ago by
Hi,
without the gap3
optional package, I still get 3 doctest failures:
File "reflection_group3d.py", line 63, in sage.plot.plot3d.reflection_group3d Failed example: g_plot = cayley_graph_3d(A1A1, point=(21,11,31)) Expected nothing Got: doctest:warning File "reflection_group3d.py", line 212, in sage.plot.plot3d.reflection_group3d.cayley_graph_3d._real_dimension Failed example: A = cayley_graph_3d(W) File "reflection_group3d.py", line 213, in sage.plot.plot3d.reflection_group3d.cayley_graph_3d._real_dimension Failed example: A.real_dimension 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:
https://doc.sagemath.org/html/en/developer/coding_basics.html#documentationstrings
comment:15 Changed 3 years ago by
 Commit changed from 798dd74e518a692c4ac0967adec128f0693592d2 to e1ee29f14b901570c057096d8ca7b023093abdf1
Branch pushed to git repo; I updated commit sha1. New commits:
19dbf74  Merge branch 'u/kdilks/3d_cayley_graphs' of git://trac.sagemath.org/sage into 24280cayley3d

e1ee29f  All 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.

Package code currently at this link: https://github.com/kjcollins/3d_cayley_graphs. Model class is in cayley_model.sage.
We would like to include this as an addition to sage/plot/plot3d, if it's appropriate.