Open 7ee7619c-ba8c-4a11-88d1-b1bbf7e7cb4b opened 6 years ago
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.
Branch: u/kjcollins/3d_cayley_graphs
Commit: 80faffa
New commits:
80faffa | uploading package to ticket |
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
Hi,
I had a look at the code, here are already some things to have a look at:
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/#writing-code-for-sage . 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/pep-0008/
The examples given in the documentation should work out-of-the-box, 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 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
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)?
Changed keywords from none to sagedays@icerm
Changed branch from u/kjcollins/3d_cayley_graphs to u/kdilks/3d_cayley_graphs
The problem is that having
warnings.simplefilter("always")
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.
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 |
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#documentation-strings
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. |
On the website reflections.swarthmore.edu, 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.
CC: @jplab @tscrim @bsalisbury1 @elizabethjd
Component: graphics
Keywords: sagedays@icerm
Branch/Commit: u/kdilks/3d_cayley_graphs @
e1ee29f
Issue created by migration from https://trac.sagemath.org/ticket/24280