Refactor and extend root systems plots

[nthiery]

The attached patch refactors in depth the root sytem plotting facilities of Sage. In particular it contains all the features provided in MuPAD-Combinat and many more.

Some of those features came up during discussions with Reda Chaibi, Arthur Lubovski and Christopher Hanusa. Thanks to them!

See: ​http://wiki.sagemath.org/combinat/CoolPictures?action=AttachFile&do=get&target=root-system-plots.pdf for a (slightly outdated) version of the enclosed tutorial with all the pictures shown.

[nborie]

I find this ticket is completely integrated inside #4326

I remember that i did this job. So feel free to merge/destroy/erase this ticket. I don't know the current way to dealt with such ticket...

[nthiery]

Actually, there is still room for improvement to the current plots. So let's keep this open. I'll just update the description.

[nborie]

Hello,

Here is a patch which realize the porting of what was done in MuPAD... I tried to do my best but this patch really need a strong English language review. For anybody interssinting in reviewing, commenting, or anything... here are some pointers :

MuPAD example : ​http://wstein.org/home/wstein/www/home/nthiery/2008-03-01-RootSystemPlots.html

Sage-support discussion : ​https://groups.google.com/forum/?hl=fr&fromgroups=#!topic/sage-support/fRTEE_IECzU

[ncohen]

Hellooooooooooo !!

Nicolas M. Thiéry : that's the ticket that should have been set to needs_review ages ago and never was. It would be nice to have this into Sage, if only to be able to print beautifu Sage-combinat posters with fancy pictures inside.

Nathann

[nthiery]

I'll upload a (quite heavily) refactored patch shortly.

[nthiery]

So, the test pass, but the startup module complains about the new module that is imported on startup. I'll try with a lazy import tomorrow. I leave it as needs review, as the review of all the rest can continue!

[nthiery]

Replying to nthiery:

So, the test pass, but the startup module complains about the new module that is imported on startup. I'll try with a lazy import tomorrow. I leave it as needs review, as the review of all the rest can continue!

Hmm, I just had a look, but am not sure how to handle this. Lazy importing works just fine for the code, but then:

    sage: sage.combinat.root_system.plot?

fails, whereas I think we want to support this natural way to access this tutorial.

The seemingly natural solution would be to lazy import the full module. However:

sage: sage.misc.lazy_import.lazy_import('sage.combinat.root_system', 'plot')
sage: type(plot)
sage.misc.lazy_import.LazyImport
sage: plot
/opt/sage/sage : ligne 135 :  6989 Erreur de segmentation  (core dumped) "$SAGE_ROOT/spkg/bin/sage" "$@"
Process SAGE exited abnormally with code 139

I'll post on Sage-devel on this topic. In the mean time the rest of the review can continue!

If we can't get a good solution shortly, I am at this point in favor of sticking to a non lazy import in order to allow for accessing the tutorial as above, even if the price is to import (yet another) new module in Sage.

Cheers,

Nicolas

[nthiery]

The updated patch implements wireframe drawing for 3D alcoves, and specifying a color as None to disable certain pieces. It also fixes a couple typos here and there in the doc.

[tscrim]

Hey Nicolas²,

Does this conflict with #2023, and if so, which patch do you want to have the dependency?

Thanks,
Travis

[nthiery]

It could possibly conflict. Both patches should get soon into Sage, and I already had a good look at #2023. So once we have had that little discussion with Dan, we should just get it in. And then #4327 which is already based on it anyway.

[nthiery]

Folded in Travis' reviewers patch and reuploaded.

[tscrim]

Looks good. Thank you.

[nthiery]

Uploaded a new version (double checked by Travis) fixing one of the long doctest failure on 5.10. It was caused by the new matplotlib that emits a warning for arrows of length 0:

sage: arrow([1,1],[1,1])
/home/nthiery/sage-5.10.beta1/local/lib/python2.7/site-packages/matplotlib/patches.py:3039: RuntimeWarning: invalid value encountered in double_scalars
  ddx = pad_projected * dx / cp_distance
/home/nthiery/sage-5.10.beta1/local/lib/python2.7/site-packages/matplotlib/patches.py:3040: RuntimeWarning: invalid value encountered in double_scalars
  ddy = pad_projected * dy / cp_distance
/home/nthiery/sage-5.10.beta1/local/lib/python2.7/site-packages/matplotlib/patches.py:3043: RuntimeWarning: invalid value encountered in double_scalars
  dx = dx / cp_distance * head_dist
/home/nthiery/sage-5.10.beta1/local/lib/python2.7/site-packages/matplotlib/patches.py:3044: RuntimeWarning: invalid value encountered in double_scalars
  dy = dy / cp_distance * head_dist

The other failure is due to some doctests being ignored. See:

​https://groups.google.com/forum/?fromgroups=#!topic/sage-devel/4m1ydGdiGf8

[nthiery]

Hi Travis,

The updated patch fixes the doc so that later on doctests won't be ignored, and update the number of ignored doctests in the mean time.

All long tests passed.

Here is the metadiff:

diff --git a/trac_4327-root_system_plot_refactor-nt.patch b/trac_4327-root_system_plot_refactor-nt.patch
--- a/trac_4327-root_system_plot_refactor-nt.patch
+++ b/trac_4327-root_system_plot_refactor-nt.patch
@@ -1,5 +1,5 @@
 # HG changeset patch
-# Parent aa718acb2dbac8faab463c994aa7f4052a546363
+# Parent 89d0e0f941ae79ca2d8dd83d3ac6d20a4b82382a
 #4327: Refactor and extend root systems plots
 
 diff --git a/doc/en/reference/combinat/root_systems.rst b/doc/en/reference/combinat/root_systems.rst
@@ -2569,7 +2569,7 @@ diff --git a/sage/combinat/root_system/t
  import ambient_space
  
  class AmbientSpace(ambient_space.AmbientSpace):
-@@ -135,13 +136,38 @@ class AmbientSpace(ambient_space.Ambient
+@@ -135,13 +136,36 @@ class AmbientSpace(ambient_space.Ambient
          given, returns (k, ... ,k), the k-th power of the
          determinant.
  
@@ -2582,14 +2582,8 @@ diff --git a/sage/combinat/root_system/t
          """
          return self.sum(self.monomial(j)*k for j in range(self.n))
  
-+    """
-+    Use barycentric projection by default
-+
-+    .. SEEALSO::
-+
-+        - :meth:`sage.combinat.root_system.root_lattice_realizations.RootLatticeRealizations._plot_projection`
-+
-+    EXAMPLES::
++    __doc__ += """
++    By default, this ambient space uses the barycentric projection for plotting::
 +
 +        sage: L = RootSystem(["A",2]).ambient_space()
 +        sage: e = L.basis()
@@ -2604,6 +2598,10 @@ diff --git a/sage/combinat/root_system/t
 +        (2, 2, 3, 0)
 +        sage: L._plot_projection(l)
 +        (0, -1121/1189, 7/3)
++
++    .. SEEALSO::
++
++        - :meth:`sage.combinat.root_system.root_lattice_realizations.RootLatticeRealizations.ParentMethods._plot_projection`
 +    """
 +    _plot_projection = RootLatticeRealizations.ParentMethods.__dict__['_plot_projection_barycentric']
  
@@ -2621,18 +2619,12 @@ diff --git a/sage/combinat/root_system/t
  class AmbientSpace(ambient_space.AmbientSpace):
      """
      EXAMPLES::
-@@ -82,6 +82,32 @@ class AmbientSpace(ambient_space.Ambient
+@@ -82,6 +82,30 @@ class AmbientSpace(ambient_space.Ambient
          return Family({ 1: self([1,0,-1]),
                          2: self([2,-1,-1])})
  
-+    """
-+    Use barycentric projection by default
-+
-+    .. SEEALSO::
-+
-+        - :meth:`sage.combinat.root_system.root_lattice_realizations.RootLatticeRealizations._plot_projection`
-+
-+    EXAMPLES::
++    __doc__ += """
++    By default, this ambient space uses the barycentric projection for plotting::
 +
 +        sage: L = RootSystem(["G",2]).ambient_space()
 +        sage: e = L.basis()
@@ -2647,6 +2639,10 @@ diff --git a/sage/combinat/root_system/t
 +        (2, 2, 3, 0)
 +        sage: L._plot_projection(l)
 +        (0, -1121/1189, 7/3)
++
++    .. SEEALSO::
++
++        - :meth:`sage.combinat.root_system.root_lattice_realizations.RootLatticeRealizations.ParentMethods._plot_projection`
 +    """
 +    _plot_projection = RootLatticeRealizations.ParentMethods.__dict__['_plot_projection_barycentric']
 +
@@ -3011,3 +3007,15 @@ diff --git a/sage/combinat/root_system/w
 -            G.axes(False)
 -            return G
 -
+diff --git a/sage/doctest/sources.py b/sage/doctest/sources.py
+--- a/sage/doctest/sources.py
++++ b/sage/doctest/sources.py
+@@ -675,6 +675,8 @@ class FileDocTestSource(DocTestSource):
+             There are 18 tests in sage/combinat/partition.py that are not being run
+             There are 12 tests in sage/combinat/tableau.py that are not being run
+             There are 15 tests in sage/combinat/root_system/cartan_type.py that are not being run
++            There are 8 tests in sage/combinat/root_system/type_A.py that are not being run
++            There are 8 tests in sage/combinat/root_system/type_G.py that are not being run
+             There are 3 unexpected tests being run in sage/doctest/parsing.py
+             There are 1 unexpected tests being run in sage/doctest/reporting.py
+             There are 9 tests in sage/graphs/graph_plot.py that are not being run

[tscrim]

Looks good to me. Thanks Nicolas.

[jdemeyer]

The PDF documentation doesn't build:

! Package inputenc Error: Keyboard character used is undefined
(inputenc)                in inputencoding `utf8x'.

See the inputenc package documentation for explanation.
Type  H <return>  for immediate help.
 ...

l.96802 ...}1\PYGZcb{}\$' at the point (0.0,1.0)]}

?  [54]
! Emergency stop.
 ...

l.96802 ...}1\PYGZcb{}\$' at the point (0.0,1.0)]}

!  ==> Fatal error occurred, no output PDF file produced!
Transcript written on combinat.log.

[nthiery]

Replying to jdemeyer:

The PDF documentation doesn't build:

! Package inputenc Error: Keyboard character used is undefined
(inputenc)                in inputencoding `utf8x'.
...

Ah shoot, sorry about that. The docstrings for this method was missing its starting 'r', and which caused a \a in it to be misinterpreted as a non UTF-8 character.

The newly updated patch fixes this. While I was at it, it fixes half a dozen other missing 'r' in other methods introduced by this patch.

Since the change is trivial, I am allowing myself to put it back to positive review.

For the record, here is the diff between the two patches:

diff --git a/trac_4327-root_system_plot_refactor-nt.patch b/trac_4327-root_system_plot_refactor-nt.patch
--- a/trac_4327-root_system_plot_refactor-nt.patch
+++ b/trac_4327-root_system_plot_refactor-nt.patch
@@ -1,5 +1,5 @@
 # HG changeset patch
-# Parent 89d0e0f941ae79ca2d8dd83d3ac6d20a4b82382a
+# Parent 75e26170e32bcbb5f78e1784a1df985ecb71e1db
 #4327: Refactor and extend root systems plots
 
 diff --git a/doc/en/reference/combinat/root_systems.rst b/doc/en/reference/combinat/root_systems.rst
@@ -631,7 +631,7 @@ new file mode 100644
 +lazy_import("sage.combinat.root_system.root_lattice_realizations", "RootLatticeRealizations")
 +
 +class PlotOptions:
-+    """
++    r"""
 +    A class for plotting options for root lattice realizations.
 +
 +    .. SEEALSO::
@@ -740,7 +740,7 @@ new file mode 100644
 +
 +    @cached_method
 +    def in_bounding_box(self, x):
-+        """
++        r"""
 +        Return whether ``x`` is in the bounding box.
 +
 +        INPUT:
@@ -763,7 +763,7 @@ new file mode 100644
 +        return self.bounding_box.contains(self.projection(x))
 +
 +    def text(self, label, position):
-+        """
++        r"""
 +        Return text widget with label ``label`` at position ``position``
 +
 +        INPUT:
@@ -851,7 +851,7 @@ new file mode 100644
 +                return self._color("other")
 +
 +    def projection(self, v):
-+        """
++        r"""
 +        Return the projection of ``v``.
 +
 +        INPUT:
@@ -880,7 +880,7 @@ new file mode 100644
 +        return v
 +
 +    def intersection_at_level_1(self, x):
-+        """
++        r"""
 +        Return ``x`` scaled at the appropriate level, if level is set;
 +        otherwise return ``x``.
 +
@@ -912,7 +912,7 @@ new file mode 100644
 +            return x
 +
 +    def empty(self, *args):
-+        """
++        r"""
 +        Return an empty plot.
 +
 +        EXAMPLES::
@@ -1255,7 +1255,7 @@ new file mode 100644
 +
 +@cached_function
 +def barycentric_projection_matrix(n, angle=0):
-+    """
++    r"""
 +    Returns a family of `n+1` vectors evenly spaced in a real vector space of dimension `n`
 +
 +    Those vectors are of norm `1`, the scalar product between any two
@@ -2339,7 +2339,7 @@ diff --git a/sage/combinat/root_system/r
 +
 +
 +        def plot_bounding_box(self, **options):
-+            """
++            r"""
 +            Plot the bounding box.
 +
 +            INPUT:

[jdemeyer]

Replying to nthiery:

Since the change is trivial, I am allowing myself to put it back to positive review.

Assuming that you checked that the PDF documentation does build, that's okay.

[nthiery]

Replying to jdemeyer:

Assuming that you checked that the PDF documentation does build, that's okay.

Yup, I did. Well at least for reference/combinat.

[jdemeyer]

sage -t devel/sage/sage/combinat/root_system/root_lattice_realizations.py
**********************************************************************
File "devel/sage/sage/combinat/root_system/root_lattice_realizations.py", line 1840, in sage.combinat.root_system.root_lattice_realizations.RootLatticeRealizations.Pare
ntMethods.plot_roots
Failed example:
    list(RootSystem(["A",2]).root_lattice().plot_roots("all"))
Expected:
    [Arrow from (0.0,0.0) to (1.0,0.0),
     Text '$\alpha_{1}$' at the point (1.05,0.0),
     Arrow from (0.0,0.0) to (0.0,1.0),
     Text '$\alpha_{2}$' at the point (0.0,1.05),
     Arrow from (0.0,0.0) to (1.0,1.0),
     Text '$\alpha_{1} + \alpha_{2}$' at the point (1.05,1.05),
     Arrow from (0.0,0.0) to (-1.0,0.0),
     Text '$\left(-1\right)\alpha_{1}$' at the point (-1.05,0.0),
     Arrow from (0.0,0.0) to (0.0,-1.0),
     Text '$\left(-1\right)\alpha_{2}$' at the point (0.0,-1.05),
     Arrow from (0.0,0.0) to (-1.0,-1.0),
     Text '$\left(-1\right)\alpha_{1} + \left(-1\right)\alpha_{2}$' at the point (-1.05,-1.05)]
Got:
    [Arrow from (0.0,0.0) to (1.0,0.0), Text '$\alpha_{1}$' at the point (1.05,0.0), Arrow from (0.0,0.0) to (0.0,1.0), Text '$\alpha_{2}$' at the point (0.0,1.05), Arr
ow from (0.0,0.0) to (1.0,1.0), Text '$\alpha_{1} + \alpha_{2}$' at the point (1.05,1.05), Arrow from (0.0,0.0) to (-1.0,0.0), Text '$-\alpha_{1}$' at the point (-1.05,
0.0), Arrow from (0.0,0.0) to (0.0,-1.0), Text '$-\alpha_{2}$' at the point (0.0,-1.05), Arrow from (0.0,0.0) to (-1.0,-1.0), Text '$-\alpha_{1} - \alpha_{2}$' at the p
oint (-1.05,-1.05)]
**********************************************************************

[jdemeyer]

Never mind, last doctest failure is because of #13735.

[nthiery]

Yippee!

Thanks Nicolas, Travis, and everyone who contributed to get this done!

[vbraun]

The plot_expose() function should be a method of plot objects. Its definitely useful outside of root lattices. See #14640