#4327 closed enhancement (fixed)
Refactor and extend root systems plots
Reported by: | nthiery | Owned by: | mhansen |
---|---|---|---|
Priority: | minor | Milestone: | sage-5.10 |
Component: | combinatorics | Keywords: | |
Cc: | sage-combinat, alubovsky | Merged in: | sage-5.10.beta4 |
Authors: | Nicolas Borie, Nicolas M. Thiéry | Reviewers: | Nicolas M. Thiéry, Nicolas Borie, Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #14175, #14176, #14177, #2023 | Stopgaps: |
Description (last modified by )
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.
Attachments (1)
Change History (33)
comment:1 Changed 8 years ago by
- Cc sage-combinat added
comment:2 Changed 8 years ago by
comment:3 Changed 8 years ago by
- Description modified (diff)
- Summary changed from Root systems plots to Improve root systems plots
Actually, there is still room for improvement to the current plots. So let's keep this open. I'll just update the description.
comment:4 Changed 5 years ago by
- Report Upstream set to N/A
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
comment:5 Changed 5 years ago by
- Status changed from new to needs_review
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
comment:6 Changed 4 years ago by
- Dependencies set to #14175, #14176, #14177
- Reviewers set to Nicolas M. Thiéry, Nicolas Borie
- Status changed from needs_review to needs_work
I'll upload a (quite heavily) refactored patch shortly.
comment:7 Changed 4 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
- Summary changed from Improve root systems plots to Refactor and extend root systems plots
comment:8 follow-up: ↓ 9 Changed 4 years ago by
- Work issues set to lazy import sage.combinat.root_system.plot
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!
comment:9 in reply to: ↑ 8 Changed 4 years ago by
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
comment:10 Changed 4 years ago by
- Cc alubovsky added
comment:11 Changed 4 years ago by
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.
comment:12 Changed 4 years ago by
Hey Nicolas^{2},
Does this conflict with #2023, and if so, which patch do you want to have the dependency?
Thanks,
Travis
comment:13 Changed 4 years ago by
comment:14 Changed 4 years ago by
- Dependencies changed from #14175, #14176, #14177 to #14175, #14176, #14177, #2023
comment:15 Changed 4 years ago by
- Reviewers changed from Nicolas M. Thiéry, Nicolas Borie to Nicolas M. Thiéry, Nicolas Borie, Travis Scrimshaw
- Work issues lazy import sage.combinat.root_system.plot deleted
comment:16 Changed 4 years ago by
Folded in Travis' reviewers patch and reuploaded.
comment:17 Changed 4 years ago by
- Description modified (diff)
comment:18 Changed 4 years ago by
- Status changed from needs_review to positive_review
Looks good. Thank you.
comment:19 Changed 4 years ago by
- Status changed from positive_review to needs_work
- Work issues set to minor long test failure
comment:20 Changed 4 years ago by
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
comment:21 Changed 4 years ago by
- Status changed from needs_work to needs_review
- Work issues minor long test failure deleted
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
comment:22 Changed 4 years ago by
- Status changed from needs_review to positive_review
Looks good to me. Thanks Nicolas.
comment:23 follow-up: ↓ 24 Changed 4 years ago by
- Status changed from positive_review to needs_work
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.
Changed 4 years ago by
comment:24 in reply to: ↑ 23 ; follow-up: ↓ 26 Changed 4 years ago by
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:
comment:25 Changed 4 years ago by
- Status changed from needs_work to positive_review
comment:26 in reply to: ↑ 24 ; follow-up: ↓ 27 Changed 4 years ago by
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.
comment:27 in reply to: ↑ 26 Changed 4 years ago by
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.
comment:28 Changed 4 years ago by
- Status changed from positive_review to needs_work
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)] **********************************************************************
comment:29 Changed 4 years ago by
- Status changed from needs_work to positive_review
Never mind, last doctest failure is because of #13735.
comment:30 follow-up: ↓ 31 Changed 4 years ago by
- Merged in set to sage-5.10.beta4
- Resolution set to fixed
- Status changed from positive_review to closed
comment:31 in reply to: ↑ 30 Changed 4 years ago by
Yippee!
Thanks Nicolas, Travis, and everyone who contributed to get this done!
comment:32 Changed 4 years ago by
The plot_expose()
function should be a method of plot objects. Its definitely useful outside of root lattices. See #14640
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...