#23696 closed enhancement (fixed)
Update matplotlib to 2.1.0
| Reported by: | dimpase | Owned by: | |
|---|---|---|---|
| Priority: | major | Milestone: | sage-8.2 |
| Component: | packages: standard | Keywords: | |
| Cc: | fbissey, strogdon, egourgoulhon, charpent, chapoton | Merged in: | |
| Authors: | François Bissey, Steven Trogdon | Reviewers: | Eric Gourgoulhon, Dima Pasechnik |
| Report Upstream: | None of the above - read trac for reasoning. | Work issues: | |
| Branch: | ca3b242 (Commits, GitHub, GitLab) | Commit: | |
| Dependencies: | Stopgaps: |
GitHub link to the corresponding issue
Description (last modified by )
It's time we update; Debian already uses matplotlib 2.0.0.
For matplotlib 2.1.0 we need to update setuptools_scm and two new dependencies backports.functools_lru_cache and subprocess32.
Tarballs:
- http://sagetrac.lipn.univ-paris13.fr/sage/matplotlib-2.1.0.tar.bz2
- https://pypi.python.org/packages/4e/91/0e93d9455254b7b630fb3ebe30cc57cab518660c5fad6a08aac7908a4431/backports.functools_lru_cache-1.4.tar.gz
- https://pypi.python.org/packages/03/6d/aafdd01edd227ee879b691455bf19895091872af7e48192bea1758c82032/setuptools_scm-1.15.6.tar.gz
- http://ftp.swin.edu.au/gentoo/distfiles/subprocess32-3.2.7.tar.gz
Attachments (13)
Change History (298)
comment:1 Changed 5 years ago by
| Cc: | fbissey added |
|---|
comment:2 Changed 5 years ago by
comment:3 Changed 5 years ago by
| Dependencies: | → #23689 |
|---|
Surely you meant #23689 (added to deps now).
comment:4 Changed 5 years ago by
I was nicely self referential :) No patch for these in debian, they must live with it. I thought I had more stuff like the first failure, something silly like replacing [(0,0,0)] by [(0.0,0.0,0.0)] or some such fixes it if I remember correctly.
comment:5 Changed 5 years ago by
| Dependencies: | #23689 → #23689, #23711 |
|---|
I'm adding #23711 as dependency because it might be relevant to this ticket.
comment:6 Changed 5 years ago by
| Cc: | strogdon added |
|---|
comment:7 Changed 5 years ago by
| Dependencies: | #23689, #23711 |
|---|
comment:8 Changed 5 years ago by
| Summary: | update matplotlib to 2.0.2 → Update matplotlib to 2.1.0 |
|---|
comment:9 Changed 5 years ago by
| Description: | modified (diff) |
|---|
comment:10 Changed 5 years ago by
| Description: | modified (diff) |
|---|
comment:11 follow-up: 12 Changed 5 years ago by
Wondering about the test about arrow. From the file
Dashed arrows should have solid arrowheads,
:trac:`12852`. This test saves the plot of a dashed arrow to
an EPS file. Within the EPS file, ``stroke`` will be called
twice: once to draw the line, and again to draw the
arrowhead. We check that both calls do not occur while the
dashed line style is enabled::
sage: a = arrow((0,0), (1,1), linestyle='dashed')
sage: filename = tmp_filename(ext='.eps')
sage: a.save(filename=filename)
sage: with open(filename, 'r') as f:
....: contents = f.read().replace('\n', ' ')
sage: two_stroke_pattern = r'setdash.*stroke.*stroke.*setdash'
sage: import re
sage: two_stroke_re = re.compile(two_stroke_pattern)
sage: two_stroke_re.search(contents) is None
True
When I look at the result of the last one I get
sage: two_stroke_re.search(contents) <_sre.SRE_Match object at 0x7f0420011bf8>
Obviously not none but we don't know from that if what is being tested failed or not. For the other doctest, it is a case of input being integer when the interface request a "real" type. Replacing a (0,0,0) by (0.0,0.0,0.0) does indeed fix the test.
comment:12 Changed 5 years ago by
Replying to fbissey:
Wondering about the test about arrow. From the file
Dashed arrows should have solid arrowheads, :trac:`12852`. This test saves the plot of a dashed arrow to an EPS file. Within the EPS file, ``stroke`` will be called twice: once to draw the line, and again to draw the arrowhead. We check that both calls do not occur while the dashed line style is enabled:: sage: a = arrow((0,0), (1,1), linestyle='dashed') sage: filename = tmp_filename(ext='.eps') sage: a.save(filename=filename) sage: with open(filename, 'r') as f: ....: contents = f.read().replace('\n', ' ') sage: two_stroke_pattern = r'setdash.*stroke.*stroke.*setdash' sage: import re sage: two_stroke_re = re.compile(two_stroke_pattern) sage: two_stroke_re.search(contents) is None TrueWhen I look at the result of the last one I get
sage: two_stroke_re.search(contents) <_sre.SRE_Match object at 0x7f0420011bf8>Obviously not
nonebut we don't know from that if what is being tested failed or not.
Not sure about what two_stroke_re.search(contents) returns, but I think the doctest actually failed. If I replace two_stroke_re.search(contents) is None with two_stroke_re.search(contents) is not None I get True. If you look at the generated .eps file the arrow is drawn with dashed lines. So I think the Sage patch from ticket #12852 is not working. I don't have matplotlib-2.1.0, 2.0.2 was used.
comment:13 Changed 5 years ago by
Thanks Steve, I was working remotely so I didn't think of opening the generated .eps. I also use 2.0.2 right now but I am expecting 2.1.0 to be the same.
comment:14 Changed 5 years ago by
It seems that line 440 in src/sage/plot/arrow.py
pe2 = ConditionalStroke(CheckNthSubPath(p, 1), [pe.Stroke(linestyle="solid")])
is doing nothing. In particular pe.Stroke() is ignoring linestyle="solid". This is the piece that insures the head of the arrow is filled/solid. With matplotlib-1.5.3 if I replace linestyle="solid" with linestyle="junk" I get ValueError: Unrecognized linestyle: junk when saving the .eps
sage: a = arrow((0,0), (1,1), linestyle='dashed')
sage: filename = tmp_filename(ext='.eps')
sage: a.save(filename=filename)
I get no error when doing the same with matplotlib-2.0.2 and the head of the arrow is drawn with linestyle="dashed".
comment:15 Changed 5 years ago by
| Report Upstream: | N/A → Reported upstream. No feedback yet. |
|---|
The arrow issue has been reported upstream. See https://github.com/matplotlib/matplotlib/issues/9579
comment:16 Changed 5 years ago by
| Report Upstream: | Reported upstream. No feedback yet. → None of the above - read trac for reasoning. |
|---|
Apparently a not so obvious API change. The following should work for >= 1.5.3. The .eps looks (needs to be confirmed!) OK.
--- a/src/sage/plot/arrow.py
+++ b/src/sage/plot/arrow.py
@@ -437,7 +437,7 @@ class Arrow(GraphicPrimitive):
pe1.draw_path(renderer, gc, tpath, affine, rgbFace)
pe1 = ConditionalStroke(CheckNthSubPath(p, 0), [pe.Stroke()])
- pe2 = ConditionalStroke(CheckNthSubPath(p, 1), [pe.Stroke(linestyle="solid")])
+ pe2 = ConditionalStroke(CheckNthSubPath(p, 1), [pe.Stroke(dashes={'dash_offset': 0, 'dash_list': None})])
p.set_path_effects([pe1, pe2])
subplot.add_patch(p)
The doctest ./sage -t --long src/sage/plot/arrow.py does pass on vanilla Sage with the above changes with matplotlib-1.5.3. However on sage-on-gentoo with matplotlib-2.0.2 the generated .eps looks OK but the test still fails?
sage -t --long --warn-long 119.2 /usr/lib/python2.7/site-packages/sage/plot/arrow.py
**********************************************************************
File "/usr/lib/python2.7/site-packages/sage/plot/arrow.py", line 364, in sage.plot.arrow.Arrow._render_on_subplot
Failed example:
two_stroke_re.search(contents) is None
Expected:
True
Got:
False
comment:17 follow-up: 18 Changed 5 years ago by
What does two_stroke_re.search(contents) says (similar to https://trac.sagemath.org/ticket/23696#comment:12)?
comment:18 Changed 5 years ago by
Replying to fbissey:
What does
two_stroke_re.search(contents)says (similar to https://trac.sagemath.org/ticket/23696#comment:12)?
Basically the same
sage: two_stroke_re.search(contents) <_sre.SRE_Match object at 0x7fabe8b8e4a8>
comment:19 Changed 5 years ago by
Time to inspect the .eps file and figure out what we are looking for. But first, lunch :)
comment:20 Changed 5 years ago by
From what I have this setdash.*stroke.*stroke.*setdash is not matched in the .eps generated with version 1.5.3 but it is matched in the .eps generated with version 2.0.2. Nevertheless the .eps appears correct with version 2.0.2.
comment:21 Changed 5 years ago by
In the .eps it appears that setdash is used with multiple stroke calls to draw the axis tics. So I think this is what is being found when setdash.*stroke.*stroke.*setdash is matched using version 2.0.2. I don't think the given matching mechanism is implementation proof.
comment:22 Changed 5 years ago by
Yes, that's my conclusion too. So two choices:
1 Remove the test completely. That's rash, considering that enabled us to spot an interface change. 2 Work out something else to test. May not be possible.
comment:23 Changed 5 years ago by
Try the following:
--- a/src/sage/plot/arrow.py
+++ b/src/sage/plot/arrow.py
@@ -354,14 +354,15 @@ class Arrow(GraphicPrimitive):
dashed line style is enabled::
sage: a = arrow((0,0), (1,1), linestyle='dashed')
+ sage: a.axes(False)
sage: filename = tmp_filename(ext='.eps')
sage: a.save(filename=filename)
sage: with open(filename, 'r') as f:
....: contents = f.read().replace('\n', ' ')
- sage: two_stroke_pattern = r'setdash.*stroke.*stroke.*setdash'
+ sage: one_stroke_pattern = r'setdash.*stroke.*setdash.*stroke'
sage: import re
- sage: two_stroke_re = re.compile(two_stroke_pattern)
- sage: two_stroke_re.search(contents) is None
+ sage: one_stroke_re = re.compile(one_stroke_pattern)
+ sage: one_stroke_re.search(contents) is not None
True
"""
from sage.plot.misc import get_matplotlib_linestyle
@@ -437,7 +438,7 @@ class Arrow(GraphicPrimitive):
pe1.draw_path(renderer, gc, tpath, affine, rgbFace)
pe1 = ConditionalStroke(CheckNthSubPath(p, 0), [pe.Stroke()])
- pe2 = ConditionalStroke(CheckNthSubPath(p, 1), [pe.Stroke(linestyle="solid")])
+ pe2 = ConditionalStroke(CheckNthSubPath(p, 1), [pe.Stroke(dashes={'dash_offset': 0, 'dash_list': None})])
p.set_path_effects([pe1, pe2])
subplot.add_patch(p)
So I've
1) removed the axes
2) make sure we match
setdash.*stroke.*stroke.*setdash
We want to match the above since when successful the .eps looks something like
[7.4 3.2] 0 setdash 0.000 0.000 1.000 setrgbcolor gsave 434.2 319 10.7 10.7 clipbox 19.049231 16.833846 m 158.203077 119.064615 296.756245 220.85409 434.708734 322.20227 c stroke grestore [] 0 setdash gsave 434.2 319 10.7 10.7 clipbox 423.689492 320.311157 m 434.708734 322.20227 l 429.610079 312.252209 l 423.689492 320.311157 l cl gsave fill grestore stroke
The [] 0 setdash is the bit that draws the solid arrowhead. If that line is removed from the .eps file then the arrowhead is drawn with the dashed linetype. Of course, the description will have to be changed.
comment:24 Changed 5 years ago by
Turning things on their head a little bit. Can we look for 0 setdash instead? Basically we draw a line with a 0 dash. So without changing the original eps, looking for something like
sage: zero_dashing_pattern = r'setdash.*stroke.*stroke.*0 setdash' sage: zero_dashing_re = re.compile(zero_dashing_pattern) sage: zero_dashing_re.search(contents) is not None
again it uses is not None which is not great.
comment:25 Changed 5 years ago by
I have been trying a few things too. And I am getting frustrated because I fell like the doctest is now meaningless. It was introduced to check the behavior of matplotlib was conforming to some expectations, with an idea of before and after state. We have lost the "before" state which was in sage 5.10 (anyone wants to go back there?). In the meantime matplotlib behavior has changed and the output we are looking for doesn't make sense anymore.
I think we should just remove the doctest because we cannot make sense of what it should do anymore. It has become pointless and it is difficult to assert what we should test for now.
comment:26 Changed 5 years ago by
It seems to me that the intent is to always draw arrows with solid arrowheads (perhaps for cosmetic reasons). This can be abandoned, I guess. However, if it is retained I believe this setdash.*setdash.*stroke.*stroke.*setdash matches when the arrowhead is drawn incorrectly but does not match when it is solid. This keeps the original .eps file.
comment:27 Changed 5 years ago by
This would require that two lines be changed
--- a/src/sage/plot/arrow.py
+++ b/src/sage/plot/arrow.py
@@ -358,7 +358,7 @@ class Arrow(GraphicPrimitive):
sage: a.save(filename=filename)
sage: with open(filename, 'r') as f:
....: contents = f.read().replace('\n', ' ')
- sage: two_stroke_pattern = r'setdash.*stroke.*stroke.*setdash'
+ sage: two_stroke_pattern = r'setdash.*setdash.*stroke.*stroke.*setdash'
sage: import re
sage: two_stroke_re = re.compile(two_stroke_pattern)
sage: two_stroke_re.search(contents) is None
@@ -437,7 +437,7 @@ class Arrow(GraphicPrimitive):
pe1.draw_path(renderer, gc, tpath, affine, rgbFace)
pe1 = ConditionalStroke(CheckNthSubPath(p, 0), [pe.Stroke()])
- pe2 = ConditionalStroke(CheckNthSubPath(p, 1), [pe.Stroke(linestyle="solid")])
+ pe2 = ConditionalStroke(CheckNthSubPath(p, 1), [pe.Stroke(dashes={'dash_offset': 0, 'dash_list': None})])
p.set_path_effects([pe1, pe2])
subplot.add_patch(p)
and perhaps some comments. This also works with version 1.5.3
comment:28 Changed 5 years ago by
After a break I think you are right. One of the thing that bugs me is that the pattern searched for a bit too generic, it can catch a lot of things. The only thing you are sure of with the original pattern is that you look for at least two stroke (there could be even more) in between two setdash. I think we can try better by making sure we are in the right bounding box with
two_stroke_pattern = r'\[7\.4 3\.2\] 0 setdash.*stroke.*stroke.*setdash'
That ensure that we only look in that small box. And the result of the search should be negative. Of course I don't know if the values will stay the same across all systems or versions of matplotlib.
comment:29 Changed 5 years ago by
Cross-posting, now I see you try to have two setdash first because there is a total of 3 (for now) so that get rid of the uncertainty about the box.
comment:31 Changed 5 years ago by
| Authors: | → François Bissey, Steven Trogdon |
|---|---|
| Branch: | → u/fbissey/MPL-2.1.0 |
| Commit: | → 6a0510f35a19f907a719edb832c80dc8ecf6ffea |
| Description: | modified (diff) |
| Status: | new → needs_review |
All right, it is time to move ahead. Please review.
New commits:
| c35f93f | update matplotlib spkg. Use github in spkg-src instead of pypy (url more stable).
|
| d8630f8 | matplotlib doesn't like integer in some context, and after 2.0, doesn't seem to cast them.
|
| ecf0cb7 | fix arrow.py. 1) slight change of interface 2) in a doctest, improve the selection done by regular expression.
|
| 6a0510f | Remove warning suppression needed with MPL-1.5/numpy-1.13
|
comment:32 Changed 5 years ago by
| Status: | needs_review → needs_info |
|---|
it does seem to need libqhull, no?
[matplotlib-2.1.0] building 'matplotlib._qhull' extension [matplotlib-2.1.0] gcc -fno-strict-aliasing -g -O2 -DNDEBUG -g -fwrapv -O3 -Wall -Wno-unused -fPIC -DMPL_DEVNULL=/dev/null -DPY_ARRAY_UNIQUE_SYMBOL=MPL_matplotlib__qhull_ARRAY_API -DNPY_NO_DEPRECATED_API=NPY_1_7_API_VERSION -D__STDC_FORMAT_MACROS=1 -I/home/dima/Sage/sage-dev/local/lib/python2.7/site-packages/numpy/core/include -I/home/dima/Sage/sage-dev/local/lib/python2.7/site-packages/numpy/core/include -I/home/dima/Sage/sage-dev/local/include -I. -I/home/dima/Sage/sage-dev/local/include/python2.7 -c src/qhull_wrap.c -o build/temp.linux-x86_64-2.7/src/qhull_wrap.o [matplotlib-2.1.0] gcc -pthread -shared -L/home/dima/Sage/sage-dev/local/lib -Wl,-rpath,/home/dima/Sage/sage-dev/local/lib -L/home/dima/Sage/sage-dev/local/lib -Wl,-rpath,/home/dima/Sage/sage-dev/local/lib build/temp.linux-x86_64-2.7/src/qhull_wrap.o -L/home/dima/Sage/sage-dev/local/lib -L/home/dima/Sage/sage-dev/local/lib64 -L/home/dima/Sage/sage-dev/local/lib -lqhull -lpython2.7 -o build/lib.linux-x86_64-2.7/matplotlib/_qhull.so [matplotlib-2.1.0] /usr/lib/gcc/x86_64-pc-linux-gnu/6.3.0/../../../../x86_64-pc-linux-gnu/bin/ld: cannot find -lqhull [matplotlib-2.1.0] collect2: error: ld returned 1 exit status
comment:33 Changed 5 years ago by
Ok, it should build its own included copy. More details please! At the beginning of the log there should be a configuration summary before it starts building proper, can you post that?
comment:34 Changed 5 years ago by
That being said while it build successfully here I think some extra dependencies will need to be added.
comment:35 Changed 5 years ago by
That's what I have on in Sage:
$ ls -l local/lib/*qhull* -rw-r--r-- 1 dima dima 290024 Nov 7 23:15 local/lib/libqhullcpp.a lrwxrwxrwx 1 dima dima 19 Nov 7 23:15 local/lib/libqhull_r.so -> libqhull_r.so.7.2.0 -rwxr-xr-x 1 dima dima 418128 Nov 7 23:15 local/lib/libqhull_r.so.7.2.0 -rw-r--r-- 1 dima dima 627426 Nov 7 23:15 local/lib/libqhullstatic.a -rw-r--r-- 1 dima dima 618872 Nov 7 23:15 local/lib/libqhullstatic_r.a
Can one use that libqhull_r instead?
Note that qhull is an optional package. However, why does one need an included copy?
Is it a latest trend, include qhull everywhere (IIRC, scipy also builds its own copy...)? :-)
here is the config summary:
REQUIRED DEPENDENCIES AND EXTENSIONS
numpy: yes [version 1.13.3]
six: yes [using six version 1.11.0]
dateutil: yes [using dateutil version 2.5.3]
backports.functools_lru_cache: yes [backports.functools_lru_cache was not
found. It is required forPython versions prior to
3.2]
subprocess32: yes [subprocess32 was not found. It used for Python
versions prior to 3.2 to improves functionality on
Linux and OSX]
pytz: yes [using pytz version 2016.4]
cycler: yes [using cycler version 0.10.0]
tornado: yes [using tornado version 4.3]
pyparsing: yes [using pyparsing version 2.1.5]
libagg: yes [pkg-config information for 'libagg' could not
be found. Using local copy.]
freetype: yes [version 2.6.3]
png: yes [version 1.6.29]
qhull: yes [Using system Qhull (version unknown, no pkg-
config info)]
by the way, I just installed a systemwide qhull (on gentoo), and the build works --- although that's not what we want here I suppose...
comment:36 Changed 5 years ago by
That's not the latest trend. sage has not been in the habit of unbundling as much as possible unlike some distros. For scipy my experience in gentoo is worse. We tried unbundling some dependencies only to find they were not "pure", i.e. scipy had introduced stuff in the code to help hook it up.
In your case it looks like the sage optional qhull package may need updating or upgrading. I'll look into that latter. I suggest we make a separate ticket for it and depend on it here.
comment:37 Changed 5 years ago by
to complete building Sage I also needed to do
./sage --pip install backports.functools_lru_cache
comment:38 Changed 5 years ago by
Yes, as I was mentioning there will need dependencies added.
To go back to qhull, the fact that it works once you install it system wide probably point to a bug in matplotlib's build system. Definitely more stuff to look at.
comment:39 Changed 5 years ago by
Nope, I must be wrong on qhull. Why does the spkg doesn't install libqhull.so? Because we use a basic (patched) makefile instead of using cmake to configure, build and install.
Minimum fix producing libqhull.so or write a .pc file so libqhull_r.so is used. We could do a link or write a file from spkg-install. What is your preferred solution?
comment:40 Changed 5 years ago by
| Commit: | 6a0510f35a19f907a719edb832c80dc8ecf6ffea → 496ded5808d15c7d1830ca9662082d448a994215 |
|---|
Branch pushed to git repo; I updated commit sha1. New commits:
| 496ded5 | add backports_functools_lru_cache a new dependency of MPL. Fix MPL 2.1.0 dependencies.
|
comment:41 Changed 5 years ago by
| Description: | modified (diff) |
|---|
Apart from the issues of qhull this should be all for the dependencies.
comment:42 Changed 5 years ago by
Any obvious reason why this should be failing
Download error on https://pypi.python.org/simple/setuptools_scm/: [Errno 113] No route to host -- Some packages may not be found! Download error on https://pypi.python.org/simple/setuptools-scm/: [Errno 113] No route to host -- Some packages may not be found! Download error on https://pypi.python.org/simple/: [Errno 110] Connection timed out -- Some packages may not be found! No local packages or working download links found for setuptools_scm>=1.15.0
The route looks good to me.
comment:43 Changed 5 years ago by
To get this to work here I had to modify the existing setuptools_scm package, download setuptools_scm-1.15.6.tar.gz and install it manually.
comment:44 Changed 5 years ago by
I think sage runs pip in a way it cannot download stuff. That's probably an unhelpful message. How did I get the stuff to install properly when all these problems exist.
TODO: update setuptools_scm in this ticket.
comment:45 Changed 5 years ago by
| Status: | needs_info → needs_work |
|---|
Let's add subprocess32 I thought it was only optional from the message but it is probably better to have it.
comment:46 Changed 5 years ago by
| Commit: | 496ded5808d15c7d1830ca9662082d448a994215 → f6a9e539143095b1cd6a0e0ba762b850339d2746 |
|---|
Branch pushed to git repo; I updated commit sha1. New commits:
| f6a9e53 | update setuptools_scm for matplotlib-2.1.0
|
comment:47 Changed 5 years ago by
| Commit: | f6a9e539143095b1cd6a0e0ba762b850339d2746 → 717c3dad0ffd8aed76ba9dd7968c90c7a745252f |
|---|
Branch pushed to git repo; I updated commit sha1. New commits:
| 717c3da | Add subprocess32 as a dependency of MPL-2.1.0
|
comment:48 Changed 5 years ago by
| Description: | modified (diff) |
|---|---|
| Status: | needs_work → needs_review |
There! Apart from the qhull issue that we should tackle separately, this is ready I believe.
comment:49 Changed 5 years ago by
| Description: | modified (diff) |
|---|
comment:50 Changed 5 years ago by
In doctesting I see two of
UserWarning: Attempted to set non-positive ylimits for log-scale axis; invalid limits will be ignored.
in src/sage/plot/plot.py which I haven't investigated. There is also numerous
UserWarning: The following kwargs were not used by contour: 'label'
where it appears that extra arguments in Sage calling functions (src/sage/plot/graphics.py has such a function) are incompatible with allowed extra arguments is matplotlib functions. This incompatibility has probably been around for some time but is now exposed by the addition of
if kwargs:
s = ", ".join(map(repr, kwargs))
warnings.warn('The following kwargs were not used by contour: ' +
s)
in matplotlib/contour.py that prints the warning. If I remove the extra arguments for the Sage function calls, I don't get the warnings. This should all probably be revised/updated in some ticket.
comment:51 Changed 5 years ago by
Ok, all our previous debugging was done on MPL-2.0.2 so it is fair that new things show up in 2.1.0. I should have done a doctest run.
comment:52 Changed 5 years ago by
And there is ImportError: cannot import name delaunay in for example src/doc/en/thematic_tutorials/explicit_methods_in_number_theory/introduction.rst. Delaunay may be gone. I remember something about delaunay deprecation issues in the past.
comment:53 Changed 5 years ago by
Oh yes dear delaunay that I tried to replace only to find nothing else was like it. Was MPL-1.3 or 1.4? Well have to see if there is something it can be replaced with.
comment:54 follow-up: 60 Changed 5 years ago by
| Status: | needs_review → needs_work |
|---|
OK in sage/plot.py` we have instances of semilog (one) and log-log (one) graph where a log axis start at 0. This is in listplot function where, if not provided, the index starts at 0 - a problem on a log scale. I will re-work those examples to have indices starting from 1.
In sage/plot/graphics.py one warning
MatplotlibDeprecationWarning: The set_linestyle function was deprecated in version 2.1.
Will need to dig. Also in sage/graphs/generic_graph.py, sage/manifolds/differentiable/vectorfield.py, sage/manifolds/differentiable/tangent_vector.py, sage/combinat/posets/posets.py, sage/plot/arrow.py,
will be fun to fix
grep -rI set_linestyle src/sage/* src/sage/plot/ellipse.py: p.set_linestyle(get_matplotlib_linestyle(options['linestyle'],return_type='long')) src/sage/plot/arrow.py: p.set_linestyle(get_matplotlib_linestyle(options['linestyle'], return_type='long')) src/sage/plot/arrow.py: p.set_linestyle(get_matplotlib_linestyle(options['linestyle'], return_type='long')) src/sage/plot/circle.py: p.set_linestyle(get_matplotlib_linestyle(options['linestyle'],return_type='long')) src/sage/plot/arc.py: p.set_linestyle(get_matplotlib_linestyle(options['linestyle'], src/sage/plot/bezier_path.py: bpatch.set_linestyle(get_matplotlib_linestyle(options['linestyle'], return_type='long')) src/sage/plot/line.py: p.set_linestyle(get_matplotlib_linestyle(options['linestyle'],
In sage/plot/plot3d/list_plot3d.py we have delaunay failures (13)
from matplotlib import tri, delaunay
ImportError: cannot import name delaunay
also in sage/stats/distributions/discrete_gaussian_lattice.py, en/thematic_tutorials/_sources/explicit_methods_in_number_theory/introduction.rst
In sage/plot/arrow.py strange deprecation warning to investigate
MatplotlibDeprecationWarning: Adding an axes using the same arguments as a previous axes currently reuses the earlier instance. In a future version, a new instance will always be created and returned. Meanwhile, this warning can be suppressed, and the future behavior ensured, by passing a unique label to each axes instance.
In sage/plot/arc.py
sage -t --long --warn-long 89.4 /usr/lib64/python2.7/site-packages/sage/plot/arc.py
**********************************************************************
File "/usr/lib64/python2.7/site-packages/sage/plot/arc.py", line 299, in sage.plot.arc.Arc.bezier_path
Failed example:
b[0]
Expected:
Bezier path from (1.618..., 0.5877...) to (-0.5176..., 0.9659...)
Got:
Bezier path from (0.0, -1.0) to (0.0, -1.0)
**********************************************************************
In sage/probability/probability_distribution.pyx
File "/usr/lib64/python2.7/site-packages/sage/probability/probability_distribution.pyx", line 121, in sage.probability.probability_distribution.ProbabilityDistribution.generate_histogram_data
Failed example:
h
Expected:
[1.6299999999999999,
0.0,
0.0,
0.0,
0.0,
1.9049999999999985,
0.0,
0.0,
0.0,
1.4650000000000003]
Got:
[1.6300000000000003,
0.0,
0.0,
0.0,
0.0,
1.9049999999999976,
0.0,
0.0,
0.0,
1.4650000000000012]
**********************************************************************
The last two are curious. I am noting them but deem them suspicious until further notice.
comment:55 follow-up: 57 Changed 5 years ago by
This set_linestyle business is I believe what manifested itself with the arrow.py failure with version 2.0.2 and now throws a MatplotlibDeprecationWarning in other locations.
comment:56 Changed 5 years ago by
Fun bit. The stuff plot.py with log plots is self inflicted. The example do it on purpose but MPL didn't display a user warning before.
comment:57 Changed 5 years ago by
Replying to strogdon:
This
set_linestylebusiness is I believe what manifested itself with thearrow.pyfailure with version2.0.2and now throws aMatplotlibDeprecationWarningin other locations.
Any idea has to what to use instead of set_linestyle? Or just skip it?
comment:58 Changed 5 years ago by
| Commit: | 717c3dad0ffd8aed76ba9dd7968c90c7a745252f → 3508ea33211cc1eb1dff07f4fb4f2fb1fbe9c52a |
|---|
comment:59 Changed 5 years ago by
Looks like you have to use a linestyle option in the object definition rather than setting it with a method. It will take a bit of time to sort all of them but I think I have the idea.
New commits:
| 2f4fed4 | Merge branch 'develop' into matplotlib-2.1.0
|
| 3508ea3 | Add new user warnings in the expected output of a couple of tests
|
comment:60 Changed 5 years ago by
Replying to fbissey:
In
sage/plot/graphics.pyone warningMatplotlibDeprecationWarning: The set_linestyle function was deprecated in version 2.1.
I don't get this in graphics.py. The only warning here is
sage -t --long src/sage/plot/graphics.py
**********************************************************************
File "src/sage/plot/graphics.py", line 1713, in sage.plot.graphics.Graphics.show
Failed example:
p.show(gridlines=[[1,0],[-1,0,1]])
Expected nothing
Got:
doctest:warning
...
UserWarning: The following kwargs were not used by contour: 'label'
comment:62 Changed 5 years ago by
sage-on-gentoo with matplotlib-2.1.0 does give the MatplotlibDeprecationWarning but not the UserWarning. So there are some odd differences.
comment:64 Changed 5 years ago by
It looks like I may something to migrate delaunay to. It looks like the default triangulation is delaunay now.
comment:65 Changed 5 years ago by
| Commit: | 3508ea33211cc1eb1dff07f4fb4f2fb1fbe9c52a → e4d80390bfd6dc56e102623aa055bc64b8b1c983 |
|---|
comment:66 Changed 5 years ago by
We need a "specialist" that knows what bezier_path is supposed to do. The change in behavior in arc.py is huge. We have gone from bezier_path drawing arcs to drawing full ellipses.
comment:67 Changed 5 years ago by
| Commit: | e4d80390bfd6dc56e102623aa055bc64b8b1c983 → 7c028ad5ffaa40fa5333b40d43b0d3d0f547dd82 |
|---|
Branch pushed to git repo; I updated commit sha1. New commits:
| 7c028ad | easy fix
|
comment:68 Changed 5 years ago by
OK the UserWarning: The following kwargs were not used by contour: 'label' can be easily fixed in and of itself, the problem is that it is looks like the tip of the iceberg of some stuff that receives little testing. inside contour_plot.py.
Some stuff in there about labels is rotting.
New commits:
| 7c028ad | easy fix
|
comment:69 follow-up: 70 Changed 5 years ago by
| Commit: | 7c028ad5ffaa40fa5333b40d43b0d3d0f547dd82 → 5d27a242c7dfb8e8d7a9f6f030d07596e42ba5d4 |
|---|
Branch pushed to git repo; I updated commit sha1. New commits:
| 5d27a24 | contour and contourf from matplotlib do not have a "label" keyword argument. The whole handling of labelling and legends may need a spruce up.
|
comment:70 Changed 5 years ago by
Replying to git:
Branch pushed to git repo; I updated commit sha1. New commits:
5d27a24 contour and contourf from matplotlib do not have a "label" keyword argument. The whole handling of labelling and legends may need a spruce up.
Yes, I believe this is what I did to get rid of the the label warnings. And yes something is really wrong inside contour_plot.py. There is stuff there not consistent with even matplotlib-1.5.1, which someone needs to fix.
I'm curious, even though I think the set_linestyle needs to be addressed, I've never gotten a warning about it's use, that is with vanilla Sage.
comment:71 Changed 5 years ago by
There are certainly inconsistencies between vanilla and gentoo but I am not completely sure why yet.
I think I got the set_linestyle that triggered the warnings but there are other all over the place. And I am not sure if there is still a function for that particular object or if it isn't tested. I added a linestyle argument to particular test and it didn't result in a warning so some objects haven't deprecated it yet.
I am puzzled by the stuff about axes being re-used. Re-used from where? I guess even if it isn't re-used it is about it needing some label. I have a feeling that it is MPL's fault rather than sage's fault.
The next biggie is delaunay. I may well not have it under control until Monday (cross finger I can do something about it).
comment:72 Changed 5 years ago by
I have taken a look at delaunay. It's going to take some doing. In particular I can find no natural neighbor (nn) or spline options in the present triangulation. Or anything that would replicate what's presently done. But it may be hidden there somewhere.
comment:73 Changed 5 years ago by
For the record at this point in time with this branch
---------------------------------------------------------------------- sage -t --long --warn-long 73.7 src/sage/stats/distributions/discrete_gaussian_lattice.py # 1 doctest failed sage -t --long --warn-long 73.7 src/sage/plot/arrow.py # 1 doctest failed sage -t --long --warn-long 73.7 src/sage/plot/plot3d/list_plot3d.py # 13 doctests failed sage -t --long --warn-long 73.7 src/sage/plot/arc.py # 1 doctest failed sage -t --long --warn-long 73.7 src/doc/en/thematic_tutorials/explicit_methods_in_number_theory/introduction.rst # 1 doctest failed ----------------------------------------------------------------------
17 doctest failures. 15 are failure to import delaunay and the other two are unique.
comment:74 Changed 5 years ago by
This removes the MatplotlibDeprecationWarning in the src/sage/plot/arrow.py doctest
diff --git a/src/sage/plot/arrow.py b/src/sage/plot/arrow.py
index 2b293f17ca..fbce029306 100644
--- a/src/sage/plot/arrow.py
+++ b/src/sage/plot/arrow.py
@@ -336,7 +336,7 @@ class Arrow(GraphicPrimitive):
this into account. See :trac:`12836`::
sage: fig = Graphics().matplotlib()
- sage: sp = fig.add_subplot(1,1,1)
+ sage: sp = fig.add_subplot(1,1,1, label='axis1')
sage: a = arrow((0,0), (1,1))
sage: b = arrow((0,0), (1,1), width=20)
sage: p1 = a[0]._render_on_subplot(sp)
comment:75 follow-up: 78 Changed 5 years ago by
Humpf, use label here, label doesn't exist there, can't they make up their mind.
comment:76 Changed 5 years ago by
| Commit: | 5d27a242c7dfb8e8d7a9f6f030d07596e42ba5d4 → 87b33cf3f2cf959e161c857decc2be47ab824437 |
|---|
Branch pushed to git repo; I updated commit sha1. New commits:
| 87b33cf | supress last warning in arrow.py by adding a label in the right place
|
comment:77 Changed 5 years ago by
| Cc: | egourgoulhon added |
|---|
comment:78 Changed 5 years ago by
Replying to fbissey:
Humpf, use
labelhere,labeldoesn't exist there, can't they make up their mind.
This is all very contorted (my opinion). Let's hope that the fix doesn't just cover up the real issue. But from Sage
sage: fig = Graphics().matplotlib() sage: sp = fig.add_subplot(1,1,1, label='axis1') sage: sp <matplotlib.axes._subplots.AxesSubplot object at 0x7f1648115110>
So it would seem that add_subplot may call add_axes which then may require the label?
comment:79 Changed 5 years ago by
Sounds like it. From the message given by MPL, that's clearly what they want you to do.
comment:80 Changed 5 years ago by
To deal with delaunay we could always taint MPL-2.1.0 (or have an optional package) with the delaunay stuff from MPL-2.0.2. I have tried this and it works. But yet another item to maintain.
comment:81 Changed 5 years ago by
From the matplotlib commit
+Code Removal +```````````` + +matplotlib.delaunay +------------------- +Remove the delaunay triangulation code which is now handled by Qhull +via ``matplotlib.tri``
So I wonder if this is really correct?
comment:82 Changed 5 years ago by
That's why I said I may have a migration path. This was definitely not the case when they first deprecated delaunay and I fought to find a replacement. So I am hopeful, but I am not working on that until tomorrow.
comment:83 Changed 5 years ago by
OK now I remember the problem is not the triangulation, it is the "natural neighbor" interpolation going away. Solutions to that:
- get
delaunayback (-1) - get someone else's code (0)
- dropping "nearest neighbor" (0)
The last one wasn't very liked in #17618 2-3 years ago when upgrading to MPL-1.4.x. There is a couple of implementations floating around for natural neighbors. The original natgrid one that apparently has licensing issues, and I have seen at least another one with google but it may need a bit of work to "steal".
comment:84 follow-up: 85 Changed 5 years ago by
Perhaps scipy here if one can figure out how to use it.
comment:85 Changed 5 years ago by
comment:86 Changed 5 years ago by
Ha, forgotten the scipy stuff is "nearest neighbor", the stuff from delaunay is "natural neighbor" and they are definitely different things.
All that's available has already been tried during the upgrade to MPL 1.4. See https://git.sagemath.org/sage.git/commit/?id=b2c0045d3619b225008984e7631c3abcf4f382e8 for various way of coding things without delaunay. There is nothing available unless we adopt code from somewhere else or write our own :(
comment:87 follow-up: 88 Changed 5 years ago by
The documentation even confuses the terms nearest neighbor and natural neighbor.
comment:88 follow-ups: 89 92 Changed 5 years ago by
Replying to strogdon:
The documentation even confuses the terms
nearest neighborandnatural neighbor.
In scipy, matplotlib or sage? I thought I had done a good effort during the MPL 1.4.x upgrade in sage.
comment:89 Changed 5 years ago by
Replying to fbissey:
Replying to strogdon:
The documentation even confuses the terms
nearest neighborandnatural neighbor.In scipy, matplotlib or sage? I thought I had done a good effort during the MPL 1.4.x upgrade in sage.
Is it this one https://en.wikipedia.org/wiki/Natural_neighbor_interpolation ?
Surely Sage has more than one implementation of Voronoi/Delaunay stuff... Could you explain what exactly the issue is here?
comment:90 Changed 5 years ago by
If there is, I'd like to know. The only I know about is the one that was in matplotlib's delaunay. Now under the name voronoi we may find other stuff.
comment:92 Changed 5 years ago by
Replying to fbissey:
Replying to strogdon:
The documentation even confuses the terms
nearest neighborandnatural neighbor.In scipy, matplotlib or sage? I thought I had done a good effort during the MPL 1.4.x upgrade in sage.
Yes, not clear. I was referring to the Sage documentation. In list_plot3d.py there is
OPTIONAL KEYWORDS:
- ``interpolation_type`` - 'linear', 'nn' (natural neighbor), 'spline'
and
the triangulation. These points, the natural neighbors, are the ones
the interpolation point would connect to if inserted into the
triangulation.
and lastly
When v is a matrix the default is to use linear interpolation, when
v is a list of points the default is nearest neighbor.
comment:93 Changed 5 years ago by
@steve OK. I forgot one.
@dima That's probably a start but I doubt that's quite enough. We would need to build the interpolator based on it.
comment:94 Changed 5 years ago by
I am making experiments to time and see what various options look like. I used the same example than in #17618 but it looks slightly different even at the linear level. I don't know if it is the example or stuff in MPL 2.1.0 yet. I am doing a vanilla 8.1.rc0 build to be sure to have a good reference of before and after.
The spline looks bad - far worse than in #17618 and that's not based on MPL but on a scipy interpolator. The main problem is in the corners. There is spurious stuff trying to go to infinity in the corners which means the region that should be interesting in the middle of the graph is squashed. As always for this kind of things, it may be useful for some graphs.
I am also trialling https://docs.scipy.org/doc/scipy/reference/generated/scipy.interpolate.CloughTocher2DInterpolator.html#scipy.interpolate.CloughTocher2DInterpolator. It is fast (slightly faster than linear) and looks quite good.
I'll post again when I can see what it looks like with an identical build with MPL 1.5.3.
comment:95 Changed 5 years ago by
spline are looking equally bad before and after the upgrade of MPL (since it is scipy based no surprise). Interestingly #17618 doesn't show what the spline result looks like so it is probably not new.
The linear results are different between MPL 1.5.3 and 2.1.0. If anything 2.1.0 looks more refined.
Changed 5 years ago by
| Attachment: | MPL153-linear.png added |
|---|
Changed 5 years ago by
| Attachment: | MPL153-nn.png added |
|---|
Changed 5 years ago by
| Attachment: | MPL210-clough.png added |
|---|
Changed 5 years ago by
| Attachment: | MPL210-linear.png added |
|---|
comment:96 Changed 5 years ago by
So the code is basically
from sage.stats.distributions.discrete_gaussian_lattice import DiscreteGaussianDistributionLatticeSampler D = DiscreteGaussianDistributionLatticeSampler(identity_matrix(2), 3.0) S = [D() for _ in range(2^12)] l = [vector(v.list() + [S.count(v)]) for v in set(S)] %timeit P = list_plot3d(l, interpolation_type='linear', texture="automatic", point_list=True) list_plot3d(l, interpolation_type='spline', texture="automatic", point_list=True)
with various replacement for interpolation type.
Note the variation in z across all examples.
MPL-1.5.3, linear
The slowest run took 20.64 times longer than the fastest. This could mean that an intermediate result is being cached. 100 loops, best of 3: 4.91 ms per loop
MPL-1.5.3, natural neighbor (nn)
100 loops, best of 3: 5.8 ms per loop
MPL-2.1.0, linear
The slowest run took 22.77 times longer than the fastest. This could mean that an intermediate result is being cached. 100 loops, best of 3: 4.96 ms per loop
MPL-2.1.0, CloughTocher? (new)
100 loops, best of 3: 4.6 ms per loop
I'd say MPL-2.1.0 linear looks much better than 1.5.3 and could be the new default. It is definitely smoother. We could also replace "natural neighbor" with "CloughTocher?" in the selection. It is an interesting one. As the fastest in the lot and not depending on some caching (unlike linear) it is a viable default in my opinion.
Changed 5 years ago by
| Attachment: | MPL-2.1.0.png added |
|---|
Changed 5 years ago by
| Attachment: | MPL-1.5.3.png added |
|---|
comment:97 Changed 5 years ago by
With this somewhat naive change to list_plot3d.py added to this branch
diff --git a/src/sage/plot/plot3d/list_plot3d.py b/src/sage/plot/plot3d/list_plot3d.py
index c9f22dd464..dfc9aa8a51 100644
--- a/src/sage/plot/plot3d/list_plot3d.py
+++ b/src/sage/plot/plot3d/list_plot3d.py
@@ -373,7 +373,7 @@ def list_plot3d_tuples(v, interpolation_type, texture, **kwds):
sage: list_plot3d([(1, 2, 3), (0, 1, 3), (2, 1, 4), (1, 0, -2)], texture='yellow', num_points=50)
Graphics3d Object
"""
- from matplotlib import tri, delaunay
+ from matplotlib import tri
import numpy
import scipy
from random import random
@@ -441,16 +441,13 @@ def list_plot3d_tuples(v, interpolation_type, texture, **kwds):
if interpolation_type == 'nn' or interpolation_type =='default':
- T=delaunay.Triangulation(x,y)
- f=T.nn_interpolator(z)
+ T=tri.Triangulation(x,y)
+ f=tri.CubicTriInterpolator(T, z)
f.default_value=0.0
j=numpy.complex(0,1)
- vals=f[ymin:ymax:j*num_points,xmin:xmax:j*num_points]
from .parametric_surface import ParametricSurface
def g(x,y):
- i=round( (x-xmin)/(xmax-xmin)*(num_points-1) )
- j=round( (y-ymin)/(ymax-ymin)*(num_points-1) )
- z=vals[int(j),int(i)]
+ z = f(x, y)
return (x,y,z)
G = ParametricSurface(g, (list(numpy.r_[xmin:xmax:num_points*j]), list(numpy.r_[ymin:ymax:num_points*j])), texture=texture, **kwds)
G._set_extra_kwds(kwds)
I get for the failing doctest in src/doc/en/thematic_tutorials/explicit_methods_in_number_theory/introduction.rst
sage: v = [[len(factor(n*m)) for n in [1..15]] for m in [1..15]]
sage: list_plot3d(v, interpolation_type='nn')
the following
So with interpolation_type='nn' cubics are being used. This is to be compared with the same test but using MPL-1.5.3 (natural neighbor (nn) used)
However, this change exposes DeprecationWarnings.
comment:98 Changed 5 years ago by
For the record, this is what I used on top of this branch to get my graphs
-
src/sage/plot/plot3d/list_plot3d.py
diff --git a/src/sage/plot/plot3d/list_plot3d.py b/src/sage/plot/plot3d/list_plot3d.py index c9f22dd464..6fb03a468d 100644
a b def list_plot3d_tuples(v, interpolation_type, texture, **kwds): 373 373 sage: list_plot3d([(1, 2, 3), (0, 1, 3), (2, 1, 4), (1, 0, -2)], texture='yellow', num_points=50) 374 374 Graphics3d Object 375 375 """ 376 from matplotlib import tri , delaunay376 from matplotlib import tri 377 377 import numpy 378 378 import scipy 379 379 from random import random … … def list_plot3d_tuples(v, interpolation_type, texture, **kwds): 427 427 #arbitrary choice - assuming more or less a nxn grid of points 428 428 # x should have n^2 entries. We sample 4 times that many points. 429 429 430 if interpolation_type == 'linear': 430 points=[[x[i],y[i]] for i in range(len(x))] 431 j = numpy.complex(0, 1) 432 433 if interpolation_type == 'linear' or interpolation_type =='default': 431 434 T = tri.Triangulation(x, y) 432 435 f = tri.LinearTriInterpolator(T, z) 433 436 j = numpy.complex(0, 1) … … def list_plot3d_tuples(v, interpolation_type, texture, **kwds): 439 442 G._set_extra_kwds(kwds) 440 443 return G 441 444 442 if interpolation_type == 'nn' or interpolation_type =='default': 443 444 T=delaunay.Triangulation(x,y) 445 f=T.nn_interpolator(z) 446 f.default_value=0.0 447 j=numpy.complex(0,1) 448 vals=f[ymin:ymax:j*num_points,xmin:xmax:j*num_points] 445 if interpolation_type == 'clough': 446 f = interpolate.CloughTocher2DInterpolator(points,z) 449 447 from .parametric_surface import ParametricSurface 450 def g(x,y): 451 i=round( (x-xmin)/(xmax-xmin)*(num_points-1) ) 452 j=round( (y-ymin)/(ymax-ymin)*(num_points-1) ) 453 z=vals[int(j),int(i)] 454 return (x,y,z) 448 def g(x, y): 449 z = f([x, y]) 450 return (x, y, z) 455 451 G = ParametricSurface(g, (list(numpy.r_[xmin:xmax:num_points*j]), list(numpy.r_[ymin:ymax:num_points*j])), texture=texture, **kwds) 456 452 G._set_extra_kwds(kwds) 457 453 return G
comment:99 Changed 5 years ago by
I didn't select cubic this time around because it was 16 times slower than natural neighbor in #17618. Of course that could have changed in 2.1.0.
comment:100 Changed 5 years ago by
OK, I hadn't noticed the interpolate. business. Yet another twist.
comment:101 Changed 5 years ago by
I think the only difference is that interpolate.CloughTocher2DInterpolator is on a regular grid while tri.CubicTriInterpolator is a Clough-Tocher cubic interpolation on a triangular grid, but I could be wrong.
comment:102 Changed 5 years ago by
CloughTocher2DInterpolator is on unstructured data, so it can be any shape. Interestingly the input can be a delaunay triangulation, but it doesn't have to be on a grid. While it is a cubic interpolation, it is more sophisticated than tri.CubicTriInterpolator, in particular the scheme for evaluating the gradient (scipy) or derivatives (MPL) is vastly different.
comment:103 Changed 5 years ago by
@ steve I just tested your example with my branch. in that case your "nn" picture is quite close to the linear from MPL 2.1.0. And the cubic picture is very similar to what I get for "clough". But I imagine cubic is much slower. Here are my timings
sage: v = [[len(factor(n*m)) for n in [1..15]] for m in [1..15]] sage: %timeit NT=list_plot3d(v, interpolation_type='linear') The slowest run took 325.09 times longer than the fastest. This could mean that an intermediate result is being cached. 10000 loops, best of 3: 83.4 µs per loop sage: %timeit NT=list_plot3d(v, interpolation_type='clough') 100 loops, best of 3: 3.73 ms per loop
In this particular case linear is blazingly fast so may be cubic has a reasonable runtime.
comment:104 Changed 5 years ago by
@françois, back to this.
My tests here indicate that CloughTocher2DInterpolator with your changes is roughly 20 times faster that CubicTriInterpolator. There are some subtle differences in the pictures so they are not exactly the same. A disadvantage of using CubicTriInterpolator
is the DeprecationWarning mentioned above:
sage: v = [[len(factor(n*m)) for n in [1..15]] for m in [1..15]] sage: %timeit NT=list_plot3d(v, interpolation_type='nn') /64bitdev/storage/sage-git_develop/sage/local/lib/python2.7/site-packages/matplotlib/tri/triinterpolate.py:1074: DeprecationWarning: Both axis > a.ndim and axis < -a.ndim - 1 are deprecated and will raise an AxisError in the future. col0 = _prod_vectorized(J, np.expand_dims(tri_dz[:, 0, :], axis=3)) /64bitdev/storage/sage-git_develop/sage/local/lib/python2.7/site-packages/matplotlib/tri/triinterpolate.py:1075: DeprecationWarning: Both axis > a.ndim and axis < -a.ndim - 1 are deprecated and will raise an AxisError in the future. col1 = _prod_vectorized(J1, np.expand_dims(tri_dz[:, 1, :], axis=3)) /64bitdev/storage/sage-git_develop/sage/local/lib/python2.7/site-packages/matplotlib/tri/triinterpolate.py:1076: DeprecationWarning: Both axis > a.ndim and axis < -a.ndim - 1 are deprecated and will raise an AxisError in the future. col2 = _prod_vectorized(J2, np.expand_dims(tri_dz[:, 2, :], axis=3)) 1 loop, best of 3: 241 ms per loop
interpolation_type='nn' does use cubics.
If I change axis=3 to axis=2 in the 3 offending lines in matplotlib/tri/triinterpolate.py then the warnings disappear. I think there may be a mistake in the matplotlib code. There is this matplotlib pull that references this commit. But I'm not sure the changes were complete enough.
comment:105 Changed 5 years ago by
Exact match is not required. Subtle differences are OK, it is bound to happen considering the differences in methods. I definitely think CloughTocher2D is more sophisticated than plain cubic and cubic are usually way nicer than linear. The factor 20 in speed is quite nice.
I am not going to comment on the deprecation warning but if you think that warrant an issue in matplotlib you should go for it.
comment:106 Changed 5 years ago by
| Commit: | 87b33cf3f2cf959e161c857decc2be47ab824437 → d29091bf4a36027761bf08da8b62d06ca94a8aab |
|---|
Branch pushed to git repo; I updated commit sha1. New commits:
| d29091b | Merge branch 'develop' into matplotlib-2.1.0
|
comment:107 Changed 5 years ago by
| Commit: | d29091bf4a36027761bf08da8b62d06ca94a8aab → 594df5633ec94dde7a15cc3a866cba9cfedfde9e |
|---|
Branch pushed to git repo; I updated commit sha1. New commits:
| 594df56 | MPL's delaunay module has been removed so we switch to regular triangulation (which happen to Delaunay by default in MPL) and replace Delaunay's 'natural neighbors'
|
comment:108 Changed 5 years ago by
| Status: | needs_work → needs_review |
|---|
OK. Let's try to move this. I switched to the CloughTocher2D interpolator from scipy and hopefully updated all the documentation appropriately.
comment:109 Changed 5 years ago by
| Milestone: | sage-8.1 → sage-8.2 |
|---|
comment:110 follow-up: 114 Changed 5 years ago by
| Status: | needs_review → needs_work |
|---|
Thanks for the update!
However, with the new version atop Sage 8.2.beta0, I've noticed that the figures illustrating the "2D Plotting" section of the reference manual are of bad quality. For instance compare the attached plot-11.png (generated with MPL-2.1.0) with that at http://doc.sagemath.org/html/en/reference/plotting/sage/plot/plot.html
Changed 5 years ago by
| Attachment: | plot-11.png added |
|---|
Changed 5 years ago by
| Attachment: | latex_display_original.png added |
|---|
Changed 5 years ago by
| Attachment: | latex_display_MPL-2.1.0.png added |
|---|
comment:111 Changed 5 years ago by
Another issue is the display of LaTeX in axes labels or text graphic objects; consider for instance
sage: text(r"$\sin(x^2) + \int_0^\pi \sqrt{x}\mathrm{d} x$", (1,1), fontsize=20)
With MPL-2.1.0, the output is
which is ugly, compared to what we have in Sage 8.1:
Also the size of the picture is different.
comment:112 follow-up: 116 Changed 5 years ago by
It appears matplotlib is not finding the Computer Modern fonts. Here this seems to work
from matplotlib import rc
rc('text', usetex='True')
text(r"$\sin(x^2) + \displaystyle\int_0^\pi \sqrt{x}\mathrm{d} x$", (1,1), fontsize=20)
Here I need the \displaystyle to get the correct integral sign. And apparently matplotlib has to be told to use the TeX fonts.
@egourgoulhon does this work on your end?
comment:113 follow-up: 117 Changed 5 years ago by
Actually, I think this is the desired result
sage: import matplotlib
sage: matplotlib.rcParams['mathtext.fontset']='cm'
sage: text(r"$\sin(x^2) + \int_0^\pi \sqrt{x}\mathrm{d} x$", (1,1), fontsize=20)
I think the default mathtext font is dejavusans.
comment:114 follow-up: 118 Changed 5 years ago by
Replying to egourgoulhon:
Thanks for the update!
However, with the new version atop Sage 8.2.beta0, I've noticed that the figures illustrating the "2D Plotting" section of the reference manual are of bad quality. For instance compare the attached
plot-11.png(generated with MPL-2.1.0) with that at http://doc.sagemath.org/html/en/reference/plotting/sage/plot/plot.html
So when I do
sage: G = plot(cos(x), (x, -5, 5), thickness=5, color='green', title='A plot') sage: P = polygon([[1,2], [5,6], [5,0]], color='red') sage: Q = polygon([(-x,y) for x,y in P[0]], color='blue') sage: H = G + P + Q sage: H
I get a .png that has very good quality but the size of the file containing the .png is about 45% larger than plot-11.png. So there are differences somewhere? I wonder what is generating the .png file?
comment:115 Changed 5 years ago by
@fbissey
François, this might be a solution to the font issue (I don't want to mess up your branch):
-
build/pkgs/matplotlib/make-setup-config.py
diff --git a/build/pkgs/matplotlib/make-setup-config.py b/build/pkgs/matplotlib/make-setup-config.py index caca02b71c..ba35c001e6 100644
a b config = SafeConfigParser() 9 9 config.add_section('directories') 10 10 config.set('directories', 'basedirlist', os.environ['SAGE_LOCAL']) 11 11 12 config.add_section('rc_options') 13 config.set('rc_options', 'mathtext.fontsel', 'cm') 12 14 13 15 14 16 #####################################################################
comment:116 Changed 5 years ago by
Replying to strogdon:
It appears matplotlib is not finding the Computer Modern fonts. Here this seems to work
from matplotlib import rc rc('text', usetex='True') text(r"$\sin(x^2) + \displaystyle\int_0^\pi \sqrt{x}\mathrm{d} x$", (1,1), fontsize=20)Here I need the
\displaystyleto get the correct integral sign. And apparently matplotlib has to betoldto use theTeXfonts.@egourgoulhon does this work on your end?
No, I get the error message:
sage: from matplotlib import rc
sage: rc('text', usetex='True')
sage: text(r"$\sin(x^2) + \int_0^\pi \sqrt{x}\mathrm{d} x$", (1,1), fontsize=20)
/home/eric/sage/8.2.beta0/local/lib/python2.7/site-packages/sage/repl/rich_output/display_manager.py:590: RichReprWarning: Exception in _rich_repr_ while displaying object: [Errno 2] No such file or directory: 'dvipng'
RichReprWarning,
Graphics object consisting of 1 graphics primitive
I guess this requires the package dvipng to be installed on my system (it is not).
comment:117 Changed 5 years ago by
Replying to strogdon:
Actually, I think this is the desired result
sage: import matplotlib sage: matplotlib.rcParams['mathtext.fontset']='cm' sage: text(r"$\sin(x^2) + \int_0^\pi \sqrt{x}\mathrm{d} x$", (1,1), fontsize=20)
This one works for me (i.e. it does not require dvipng).
comment:118 Changed 5 years ago by
Replying to strogdon:
So when I do
sage: G = plot(cos(x), (x, -5, 5), thickness=5, color='green', title='A plot') sage: P = polygon([[1,2], [5,6], [5,0]], color='red') sage: Q = polygon([(-x,y) for x,y in P[0]], color='blue') sage: H = G + P + Q sage: HI get a
.pngthat has very good quality but the size of the file containing the.pngis about 45% larger thanplot-11.png.
Indeed, this generates a good quality figure, contrary to what happens for the same commands when building the doc with Sphinx.
Besides, the resolution of the png is 630x464, while for the same plot, it is 784x575 in Sage 8.1.
comment:119 Changed 5 years ago by
One more data point: after make doc-clean + make doc, the png of 3D plots in the section 3D Graphics / Plotting Functions of the reference manual are also of bad quality. Now, these figures are not produced by matplotlib but by JMol... Could this be due to one of the new packages installed as MPL-2.1.0 dependencies?
comment:120 Changed 5 years ago by
Sorry for the silence - I am sick. But now I am not lethargic anymore. I think changing things in make-setup-config.py is the way to go. It may be possible to change the size of the png file from there if it is desirable.
I am not sure what's happening with jmol the new packages are python only and jmol is java. What could be possible, but I don't know, is that jmol calls matplotlib to generate its png files. In that case changing matplotlib's default should have an impact.
comment:121 Changed 5 years ago by
| Commit: | 594df5633ec94dde7a15cc3a866cba9cfedfde9e → b81b49c8935bf50dd64267ce0931f45542a6ffa9 |
|---|
Branch pushed to git repo; I updated commit sha1. New commits:
| b81b49c | Add Steve's suggestion to set the default fonts in matplotlib
|
comment:123 follow-ups: 124 128 Changed 5 years ago by
Replying to fbissey:
ok can you check again now?
Thanks for the change, but it does not work: after rebuilding everything from scratch, the default font is still dejavusans.
By the way, I confirm that the low quality of all figures in the reference manual is due to this ticket and not to any other change introduced in Sage 8.2.beta0 (I mean, it is fine with 8.2.beta0 alone).
PS: I wish you a prompt recovery!
comment:124 follow-up: 125 Changed 5 years ago by
Replying to egourgoulhon:
By the way, I confirm that the low quality of all figures in the reference manual is due to this ticket and not to any other change introduced in Sage 8.2.beta0 (I mean, it is fine with 8.2.beta0 alone).
Maybe the code for sphinx_plot in src/doc/common/conf.py has to be updated. At present, it is
plot_pre_code = """
def sphinx_plot(plot):
import matplotlib.image as mpimg
from sage.misc.temporary_file import tmp_filename
import matplotlib.pyplot as plt
if os.environ.get('SAGE_SKIP_PLOT_DIRECTIVE', 'no') != 'yes':
fn = tmp_filename(ext=".png")
plot.plot().save(fn)
img = mpimg.imread(fn)
plt.imshow(img)
plt.margins(0)
plt.axis("off")
plt.tight_layout(pad=0)
from sage.all_cmdline import *
"""
comment:125 follow-up: 126 Changed 5 years ago by
Replying to egourgoulhon:
Replying to egourgoulhon:
By the way, I confirm that the low quality of all figures in the reference manual is due to this ticket and not to any other change introduced in Sage 8.2.beta0 (I mean, it is fine with 8.2.beta0 alone).
Maybe the code for
sphinx_plotinsrc/doc/common/conf.pyhas to be updated. At present, it isplot_pre_code = """ def sphinx_plot(plot): import matplotlib.image as mpimg from sage.misc.temporary_file import tmp_filename import matplotlib.pyplot as plt if os.environ.get('SAGE_SKIP_PLOT_DIRECTIVE', 'no') != 'yes': fn = tmp_filename(ext=".png") plot.plot().save(fn) img = mpimg.imread(fn) plt.imshow(img) plt.margins(0) plt.axis("off") plt.tight_layout(pad=0) from sage.all_cmdline import * """
All right, to what? This code is only when generating plots for the doc. I am currently working on sage exclusively remotely, so I have no idea what the documentation looks like at the end. That's why I really need input from people who can see the results.
comment:126 follow-up: 127 Changed 5 years ago by
Replying to fbissey:
All right, to what? This code is only when generating plots for the doc. I am currently working on sage exclusively remotely, so I have no idea what the documentation looks like at the end. That's why I really need input from people who can see the results.
The sphinx_plot code is clearly the culprit: if you run the following code in a Jupyter notebook (not in Sage console, because plt.show() does not work there):
import matplotlib.image as mpimg
import matplotlib.pyplot as plt
fn = tmp_filename(ext=".png")
g = plot(sin(x^2), (x, 0, 4))
g.plot().save(fn)
img = mpimg.imread(fn)
plt.imshow(img)
plt.margins(0)
plt.axis("off")
plt.tight_layout(pad=0)
plt.show()
you get a low quality figure, while if you do the same thing with Sage 8.1, you get a nice figure.
By the way, this explains why 3D plots are also affected, because plot.plot() in sphinx_plot code transforms them into a 2D figure, which is then processed by mpimg and plt.
comment:127 Changed 5 years ago by
Replying to egourgoulhon:
The
sphinx_plotcode is clearly the culprit
In sphinx_plot code, changing
plt.imshow(img)
to
plt.imshow(img, interpolation='spline16')
gives a much better result, still on small size as compare to the previous version though.
The doc from plt.imshow? says:
interpolation : string, optional, default: None Acceptable values are 'none', 'nearest', 'bilinear', 'bicubic', 'spline16', 'spline36', 'hanning', 'hamming', 'hermite', 'kaiser', 'quadric', 'catrom', 'gaussian', 'bessel', 'mitchell', 'sinc', 'lanczos' If interpolation is None, default to rc image.interpolation. See also the filternorm and filterrad parameters. If interpolation is 'none', then no interpolation is performed on the Agg, ps and pdf backends. Other backends will fall back to 'nearest'.
So maybe something around rc image.interpolation should be worked out.
comment:128 Changed 5 years ago by
Replying to egourgoulhon:
Replying to fbissey:
ok can you check again now?
Thanks for the change, but it does not work: after rebuilding everything from scratch, the default font is still dejavusans.
There is a typo in what I suggested. Not sure what I did, given the typo, to make the font selection work. The recent commit should change
config.set('rc_options', 'mathtext.fontsel', 'cm')
to
config.set('rc_options', 'mathtext.fontset', 'cm')
fontsel -> fontset
But even with the change, I'm unable to get matplotlib to recognize the font change. Something is wrong.
comment:129 Changed 5 years ago by
Perhaps the font can't be changed unless changed in matplotlibrc before the install? This doesn't seem right. However, If I insert mathtext.fontset : cm in local/lib/python2.7/site-packages/matplotlib/mpl-data/matplotlibrc things seem to work.
comment:130 Changed 5 years ago by
Thanks for all that info! I will dig into this slowly. I am still officially "unfit for work" (doctor's order) but a bit of activity at this stage should be alright.
comment:131 Changed 5 years ago by
All right. make-setup-config.py was a nice idea, but it doesn't have an impact on the final mpl-data/matplotlibrc. So if we want to play with that because we think the default sucks, we'll have to tweak it at some stage. I'd like to do it post-install but is it hard to do it right for all python. It is better to patch the default one.
So the branch will be reset one commit behind and then I'll add new stuff.
I am starting to compare mpl-data/matplotlibrc for interesting differences. For example image.interpolation was bilinear in 1.5.3 and is nearest in 2.1.0. Something for you to check Eric.
comment:132 follow-up: 133 Changed 5 years ago by
| Cc: | charpent added |
|---|
I'm pretty interested by this one. I'm trying to implement the "exclusions" never implemented in parametric_plot3d, and I might be reinventing the wheel...
comment:133 follow-up: 141 Changed 5 years ago by
Replying to charpent:
I'm pretty interested by this one. I'm trying to implement the "exclusions" never implemented in
parametric_plot3d, and I might be reinventing the wheel...
Feel free to dig around and interface anything you find. However I can hear Jeroen complaining that it should have been done in a separate ticket on top of that one. But I hope you find what you want in this release. It looks like 2.1.x may be the latest branch to support python 2.7 by the way (unless I misunderstood and 2.2.0 will be it).
comment:134 follow-up: 138 Changed 5 years ago by
My reading on the default matplotlibrc: transparency is off by default, not sure that's a big impact. Lines are thicker by default (by 0.5 points). Fonts are smaller by default (10 points instead of 12). Some more on fonts (- is 1.5.3, + is 2.1.0)
-#font.size : 12.0 -#font.serif : Bitstream Vera Serif, New Century Schoolbook, Century Schoolbook L, Utopia, ITC Bookman, Bookman, Nimbus Roman No9 L, Times New Roman, Times, Palatino, Charter, serif -#font.sans-serif : Bitstream Vera Sans, Lucida Grande, Verdana, Geneva, Lucid, Arial, Helvetica, Avant Garde, sans-serif +#font.size : 10.0 +#font.serif : DejaVu Serif, Bitstream Vera Serif, New Century Schoolbook, Century Schoolbook L, Utopia, ITC Bookman, Bookman, Nimbus Roman No9 L, Times New Roman, Times, Palatino, Charter, serif +#font.sans-serif : DejaVu Sans, Bitstream Vera Sans, Lucida Grande, Verdana, Geneva, Lucid, Arial, Helvetica, Avant Garde, sans-serif #font.cursive : Apple Chancery, Textile, Zapf Chancery, Sand, Script MT, Felipa, cursive -#font.fantasy : Comic Sans MS, Chicago, Charcoal, Impact, Western, Humor Sans, fantasy -#font.monospace : Bitstream Vera Sans Mono, Andale Mono, Nimbus Mono L, Courier New, Courier, Fixed, Terminal, monospace +#font.fantasy : Comic Sans MS, Chicago, Charcoal, Impact, Western, Humor Sans, xkcd, fantasy +#font.monospace : DejaVu Sans Mono, Bitstream Vera Sans Mono, Andale Mono, Nimbus Mono L, Courier New, Courier, Fixed, Terminal, monospace
Mainly DejaVu has displaced Bitstream Vera as the default. But more importantly in a later section
-#mathtext.fontset : cm # Should be 'cm' (Computer Modern), 'stix', - # 'stixsans' or 'custom' +#mathtext.fontset : dejavusans # Should be 'dejavusans' (default), + # 'dejavuserif', 'cm' (Computer Modern), 'stix', + # 'stixsans' or 'custom'
So upstream really thinks DejaVu is superior. We may be considered backward in our preferences soon. I am for bigger fonts myself as a short sighted person.
I suspect the following is causing the figures to be smaller - If someone wants to check
### FIGURE # See http://matplotlib.org/api/figure_api.html#matplotlib.figure.Figure -#figure.titlesize : medium # size of the figure title +#figure.titlesize : large # size of the figure title (Figure.suptitle()) #figure.titleweight : normal # weight of the figure title -#figure.figsize : 8, 6 # figure size in inches -#figure.dpi : 80 # figure dots per inch -#figure.facecolor : 0.75 # figure facecolor; 0.75 is scalar gray +#figure.figsize : 6.4, 4.8 # figure size in inches +#figure.dpi : 100 # figure dots per inch +#figure.facecolor : white # figure facecolor; 0.75 is scalar gray #figure.edgecolor : white # figure edgecolor #figure.autolayout : False # When True, automatically adjust subplot
notice that the figure is smaller (figsize) but the resolution is greater (dpi). And that's it. The rest is more fluffy compared to what we use normally.
comment:135 Changed 5 years ago by
We can bluntly override the default or create a sage style-sheet https://matplotlib.org/users/customizing.html and drop it under mpl-data/stylelib/ (where upstream is already storing them). The alternative is under MPLCONFIGDIR and it is not palatable because it is an individual user location. We could even submit our style for inclusion upstream.
comment:136 follow-up: 142 Changed 5 years ago by
There is a "classic" style, classic being MPL-1.5. I'll just push a little change and if I am not mistaken the doc will just look as before.
comment:137 Changed 5 years ago by
| Commit: | b81b49c8935bf50dd64267ce0931f45542a6ffa9 → 29a35c2fb62d0bc8ec98bb736b46d6905ecbff6e |
|---|
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
| 29a35c2 | use the classic MPL 1.5 style in sphinx_plot
|
comment:138 follow-up: 139 Changed 5 years ago by
Replying to fbissey:
So upstream really thinks
DejaVuis superior. We may be considered backward in our preferences soon.
Sage being mathematics software, we should set cm as the default font, whatever upstream thinks. Mathematical formulas with dejavusans look really ugly and cm has the big advantage of making labels/legends of figures for a scientific paper/textbook coincide with those of the main text.
I am for bigger fonts myself as a short sighted person.
+1; so for this one, we should keep MPL-2.1.0 defaults.
comment:139 follow-up: 140 Changed 5 years ago by
Replying to egourgoulhon:
Replying to fbissey:
So upstream really thinks
DejaVuis superior. We may be considered backward in our preferences soon.Sage being mathematics software, we should set
cmas the default font, whatever upstream thinks. Mathematical formulas withdejavusanslook really ugly andcmhas the big advantage of making labels/legends of figures for a scientific paper/textbook coincide with those of the main text.I am for bigger fonts myself as a short sighted person.
+1; so for this one, we should keep MPL-2.1.0 defaults.
Fonts are bigger in MPL 1.5.3! They are smaller in 2.1.0. But using "classic" should take care of that too.
comment:140 Changed 5 years ago by
Replying to fbissey:
Replying to egourgoulhon:
I am for bigger fonts myself as a short sighted person.
+1; so for this one, we should keep MPL-2.1.0 defaults.
Fonts are bigger in MPL 1.5.3! They are smaller in 2.1.0.
Sorry, I misunderstood. I agree with you: bigger fonts are better.
comment:141 Changed 5 years ago by
Replying to fbissey:
Replying to charpent:
I'm pretty interested by this one. I'm trying to implement the "exclusions" never implemented in
parametric_plot3d, and I might be reinventing the wheel...Feel free to dig around and interface anything you find. However I can hear Jeroen complaining that it should have been done in a separate ticket on top of that one. But I hope you find what you want in this release. It looks like 2.1.x may be the latest branch to support python 2.7 by the way (unless I misunderstood and 2.2.0 will be it).
Huh... Jeroen would be right ... if I intended to suggest anything exclusions-related to the present ticket, which is not the case. It is another problem, indeed...
My idea was as follows : I found that plot3d and friends leaved a "hole" in the plotted surface when a vertex of a rectangular facet had float("NaN") as z-coordinate. This can be pretty useful to represent exclusions (another way is to use implicit_plot3d, which is pretty costly in terms of computing time (proportional to resolution cubed, practically unusable....)). So I wanted to follow this to see if it entailed changes relevant to this.
Anyway, I now think that I'm running in a dead-end. My idea bumps on a pretty interesting list_plot3d snag... More on this in a future ticket (if I can exactly corner the problem...).
comment:142 follow-ups: 143 147 Changed 5 years ago by
Replying to fbissey:
There is a "classic" style, classic being MPL-1.5. I'll just push a little change and if I am not mistaken the doc will just look as before.
Yes I confirm it does look the same. However, this solution (i.e. setting plt.style.use('classic') in the code of sphinx_plot) may not be a good one, because the figures in the doc may differ from what the user get by typing the same commands, which is not desirable I guess. For instance, the doc figures are different from those produced in the console at the moment, because of the font issue.
comment:143 Changed 5 years ago by
Replying to egourgoulhon:
Replying to fbissey:
There is a "classic" style, classic being MPL-1.5. I'll just push a little change and if I am not mistaken the doc will just look as before.
Yes I confirm it does look the same. However, this solution (i.e. setting
plt.style.use('classic')in the code ofsphinx_plot) may not be a good one, because the figures in the doc may differ from what the user get by typing the same commands, which is not desirable I guess. For instance, the doc figures are different from those produced in the console at the moment, because of the font issue.
Ditto here. After a make doc-clean && make the figures in the docs (at least the ones I looked at) appear to be OK. But the documentation example to illustrate use of TeX fonts
sage: plot(exp(x), 0, 2, legend_label='$e^x$')
does not typeset the label with TeX fonts in the terminal. So the mathtext.fontset from build/pkgs/matplotlib/make-setup-config.py needs to be removed since it's not working there and the equivalent needs to be put somewhere. Where I don't know. And then let's hope that that doesn't mess with plt.style.use('classic').
comment:144 follow-up: 145 Changed 5 years ago by
I removed mathtext.fontset from make-setup-config.py in the last commit. I didn't think it was useful. So you could make the change to see if it has an impact for you. If it does, I can include it properly.
Now, Eric's comment about reproducibility is quite serious and probably a lot of work. The easiest solution is of course to just copy the classic style as matplotlibrc in the source and may be make a copy of the original one as a MPL-2.1.0 style. sage-on-distro people will rise to linch, verbally - we are quite civilized really [yes I am part of that mob].
So the alternative is documentation, documentation and more documentation :( It has to show up in examples that people would follow with an explanation why. There is currently 668 PLOT:: directives in sage's source code who's having fun yet?
comment:145 Changed 5 years ago by
Replying to fbissey:
I removed
mathtext.fontsetfrommake-setup-config.pyin the last commit. I didn't think it was useful. So you could make the change to see if it has an impact for you. If it does, I can include it properly.
I hadn't merged things properly and it was still there, but now it's gone.
Now, Eric's comment about reproducibility is quite serious and probably a lot of work. The easiest solution is of course to just copy the classic style as
matplotlibrcin the source and may be make a copy of the original one as a MPL-2.1.0 style. sage-on-distro people will rise to linch, verbally - we are quite civilized really [yes I am part of that mob]. So the alternative is documentation, documentation and more documentation :( It has to show up in examples that people would follow with an explanation why. There is currently 668PLOT::directives in sage's source code who's having fun yet?
I misunderstood. There are probably other items besides fonts that can show up if the classic style isn't available from the terminal. But here putting the mathtext.fontset declaration in matplotlibrc seems to resolve the font issue.
comment:146 Changed 5 years ago by
That's one of things a pre-made style help with. Unless you really, really think upstream screwed up with their choice of math fonts (I cannot really defend them myself).
comment:147 follow-up: 148 Changed 5 years ago by
Replying to egourgoulhon:
Replying to fbissey:
There is a "classic" style, classic being MPL-1.5. I'll just push a little change and if I am not mistaken the doc will just look as before.
Yes I confirm it does look the same. However, this solution (i.e. setting
plt.style.use('classic')in the code ofsphinx_plot) may not be a good one, because the figures in the doc may differ from what the user get by typing the same commands, which is not desirable I guess. For instance, the doc figures are different from those produced in the console at the moment, because of the font issue.
I think trying to make docs look exactly as online help is doomed to fail, in particular as online may mean
- terminal (with figures shown by a dedicated viewer),
- terminal - figures in the browser
- Jupyter
- sagenb
- ...
comment:148 Changed 5 years ago by
Replying to dimpase:
I think trying to make docs look exactly as online help is doomed to fail, in particular as online may mean
- terminal (with figures shown by a dedicated viewer),
- terminal - figures in the browser
- Jupyter
- sagenb
- ...
This may be true for 3D plots (which are not produced by matplotplib, by the way), but not for 2D plots generated by matplotlib: they all end up in a png file and all viewers will display this png figure in a unique way, whatever backend you are using (console, Jupyter, sagenb, CoCalc, SageMathCell, ...)
comment:149 Changed 5 years ago by
I now have the following failures:
sage -t --long src/sage/plot/arc.py sage -t --long src/sage/plot/plot3d/list_plot3d.py sage -t --long src/doc/en/thematic_tutorials/explicit_methods_in_number_theory/introduction.rst
sage -t --long src/sage/plot/arc.py
**********************************************************************
File "src/sage/plot/arc.py", line 299, in sage.plot.arc.Arc.bezier_path
Failed example:
b[0]
Expected:
Bezier path from (1.618..., 0.5877...) to (-0.5176..., 0.9659...)
Got:
Bezier path from (0.0, -1.0) to (0.0, -1.0)
**********************************************************************
1 item had failures:
1 of 7 in sage.plot.arc.Arc.bezier_path
[47 tests, 1 failure, 6.75 s]
----------------------------------------------------------------------
sage -t --long src/sage/plot/arc.py # 1 doctest failed
----------------------------------------------------------------------
sage -t --long src/sage/plot/plot3d/list_plot3d.py
**********************************************************************
File "src/sage/plot/plot3d/list_plot3d.py", line 95, in sage.plot.plot3d.list_plot3d.list_plot3d
Failed example:
list_plot3d(m, texture='yellow', interpolation_type='nn', frame_aspect_ratio=[1, 1, 1/3])
Expected:
Graphics3d Object
Got:
<BLANKLINE>
**********************************************************************
File "src/sage/plot/plot3d/list_plot3d.py", line 102, in sage.plot.plot3d.list_plot3d.list_plot3d
Failed example:
list_plot3d(m, texture='yellow', interpolation_type='nn', frame_aspect_ratio=[1, 1, 1/3], num_points=40)
Expected:
Graphics3d Object
Got:
<BLANKLINE>
**********************************************************************
File "src/sage/plot/plot3d/list_plot3d.py", line 143, in sage.plot.plot3d.list_plot3d.list_plot3d
Failed example:
list_plot3d(l, interpolation_type='nn', texture='yellow', num_points=100)
Expected:
Graphics3d Object
Got:
<BLANKLINE>
**********************************************************************
1 item had failures:
3 of 26 in sage.plot.plot3d.list_plot3d.list_plot3d
[39 tests, 3 failures, 18.32 s]
----------------------------------------------------------------------
sage -t --long src/sage/plot/plot3d/list_plot3d.py # 3 doctests failed
----------------------------------------------------------------------
sage -t --long src/doc/en/thematic_tutorials/explicit_methods_in_number_theory/introduction.rst
**********************************************************************
File "src/doc/en/thematic_tutorials/explicit_methods_in_number_theory/introduction.rst", line 77, in doc.en.thematic_tutorials.explicit_methods_in_number_theory.introduction
Failed example:
list_plot3d(v, interpolation_type='nn')
Exception raised:
Traceback (most recent call last):
File "/64bitdev/storage/sage-git_develop/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 517, in _run
self.compile_and_execute(example, compiler, test.globs)
File "/64bitdev/storage/sage-git_develop/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 920, in compile_and_execute
exec(compiled, globs)
File "<doctest doc.en.thematic_tutorials.explicit_methods_in_number_theory.introduction[3]>", line 1, in <module>
list_plot3d(v, interpolation_type='nn')
File "/64bitdev/storage/sage-git_develop/sage/local/lib/python2.7/site-packages/sage/plot/plot3d/list_plot3d.py", line 212, in list_plot3d
return list_plot3d_array_of_arrays(v, interpolation_type, texture, **kwds)
File "/64bitdev/storage/sage-git_develop/sage/local/lib/python2.7/site-packages/sage/plot/plot3d/list_plot3d.py", line 296, in list_plot3d_array_of_arrays
G._set_extra_kwds(kwds)
AttributeError: 'NoneType' object has no attribute '_set_extra_kwds'
**********************************************************************
1 item had failures:
1 of 9 in doc.en.thematic_tutorials.explicit_methods_in_number_theory.introduction
[6 tests, 1 failure, 2.22 s]
----------------------------------------------------------------------
sage -t --long src/doc/en/thematic_tutorials/explicit_methods_in_number_theory/introduction.rst # 1 doctest failed
----------------------------------------------------------------------
Most are related to additional interpolation_type='nn'. And then there is the Bezier business.
comment:150 Changed 5 years ago by
I had completely forgotten about the Bezier stuff in arc.py. I don't know what to do with that, the behavior is so completely different. The rest like like stuff I overlook.
comment:151 Changed 5 years ago by
| Commit: | 29a35c2fb62d0bc8ec98bb736b46d6905ecbff6e → 8f4ca6bfe57c467b5a13594f6699782068cf86c2 |
|---|
comment:152 Changed 5 years ago by
OK should only be the Bezier stuff in arc.py. If someone has a clue has to what needs to be done there. The issue is that previously just arcs would be plotted while now we get whole ellipses. And this is why the numerical value are so different - the values are for coordinates from the start to the end of the arc. As you can see with MPL-2.1.0 the beginning is equal to the end.
So there is probably an interface to tweak, so we get only arcs again.
comment:153 Changed 5 years ago by
This is the pull
request that was merged which breaks the arc.py doctest.
comment:154 Changed 5 years ago by
Lovely, if I read this correctly, they fixed a "wrong" behavior and sage may have been working around it - or possibly using it as a feature, that's not unprecedented - for some time. Also I notice 2.1.1 is released, may as well bump it.
comment:155 Changed 5 years ago by
I think the problem may well be with bezier_path. This generates an arc
sage: a = arc((0,0),2,1,0,(pi/5,pi/2+pi/12)) sage: a
However this generates an ellipse
sage: b=a[0].bezier_path() sage: b
and
sage: a[0] Arc with center (0.0,0.0) radii (2.0,1.0) angle 0.0 inside the sector (0.628318530718,1.83259571459)
comment:156 Changed 5 years ago by
Yup. I am still trying to work out how bezier_path is supposed to work. It is not exactly well explained, I had to track matplotlib classes that were not obvious. That's getting me back to high school and some stuff in first year. But why we do it, is a mystery so far. And there may be a matplotlib class that give you the same stuff, although there may be issue of it being a different class. Finally, I am unclear the stuff is ever used (even as it says it is needed for some other stuff).
comment:157 Changed 5 years ago by
| Cc: | chapoton added |
|---|
@ chapoton I am adding you to this ticket because you added bezier_path in plot/arc.py in #18939. Upgrading matplotlib to 2.1.0 makes the bezier_path whole ellipses rather that arcs. Steve Trogdon pointed out that a https://github.com/matplotlib/matplotlib/pull/8047 corrected the proportions in matplotlib and is the breaking commit. Could you advise on how to change your code?
Changed 5 years ago by
| Attachment: | bezier+arc.png added |
|---|
comment:158 Changed 5 years ago by
For discussion only. I'm sure all corner cases have not been addressed:
-
src/sage/plot/arc.py
diff --git a/src/sage/plot/arc.py b/src/sage/plot/arc.py index d18e7a7b90..87dc2a04ec 100644
a b class Arc(GraphicPrimitive): 301 301 """ 302 302 from sage.plot.bezier_path import BezierPath 303 303 from sage.plot.graphics import Graphics 304 from matplotlib.path import Path 305 import numpy as np 304 306 ma = self._matplotlib_arc() 305 transform = ma.get_transform().get_matrix() 307 def theta_stretch(theta, scale): 308 theta = np.deg2rad(theta) 309 x = np.cos(theta) 310 y = np.sin(theta) 311 return np.rad2deg(np.arctan2(scale * y, x)) 312 theta1 = theta_stretch(ma.theta1, ma.width / ma.height) 313 theta2 = theta_stretch(ma.theta2, ma.width / ma.height) 314 315 pa = ma 316 pa._path = Path.arc(theta1, theta2) 317 transform = pa.get_transform().get_matrix() 306 318 cA, cC, cE = transform[0] 307 319 cB, cD, cF = transform[1] 308 320 points = [] 309 for u in ma._path.vertices:321 for u in pa._path.vertices: 310 322 x, y = list(u) 311 323 points += [(cA * x + cC * y + cE, cB * x + cD * y + cF)] 312 324 cutlist = [points[0: 4]]
To test:
sage: a = arc((0,0),2,1,0,(pi/5,pi/2+pi/12), linestyle="--", color="red") sage: b = a[0].bezier_path() sage: c = arc((0,0),2,1,0,(pi/5,pi/2+pi/12)) sage: h = c + b sage: h
doctest:
sage -t --long src/sage/plot/arc.py
**********************************************************************
File "src/sage/plot/arc.py", line 299, in sage.plot.arc.Arc.bezier_path
Failed example:
b[0]
Expected:
Bezier path from (1.618..., 0.5877...) to (-0.5176..., 0.9659...)
Got:
Bezier path from (1.1338305413635981, 0.82377610785199551) to (-0.26557635219221642, 0.99114443967015675)
**********************************************************************
1 item had failures:
1 of 7 in sage.plot.arc.Arc.bezier_path
[47 tests, 1 failure, 6.77 s]
----------------------------------------------------------------------
sage -t --long src/sage/plot/arc.py # 1 doctest failed
----------------------------------------------------------------------
We do get an arc. The failure is perhaps expected since the original bezier graph was incorrect.
comment:159 follow-up: 160 Changed 5 years ago by
Could you show the original, so it is clear that the graph was incorrect?
comment:160 Changed 5 years ago by
Replying to fbissey:
Could you show the original, so it is clear that the graph was incorrect?
How to show this? The original will appear almost identical but the limits will be different. Let's try a calculation from Sage. With mpl-1.5.3 and the old code we get
sage: b[0] Bezier path from (1.6180339887498949, 0.58778525229247314) to (-0.51763809020504215, 0.9659258262890682)
With the revised code using mpl-2.1.0 we get
sage: b[0] Bezier path from (1.1338305413635981, 0.82377610785199551) to (-0.26557635219221642, 0.99114443967015675)
From the above example, the inputs, theta1 and theta2 are:
sage: theta1 = float(pi)/5; theta1 0.6283185307179586 sage: theta2 = float(pi)/2+float(pi)/12; theta2 1.832595714594046
Then using mpl-1.5.3 b[0] results, the angles are
sage: theta1 = arctan(0.58778525229247314/1.6180339887498949); theta1 0.3484485055191126 theta2 = arctan(0.9659258262890682/-0.51763809020504215) + float(pi); theta2 2.0627485381367054
which are incorrect.
Now using mpl-2.1.0 b[0] results, the angles are
sage: theta1 = arctan(0.82377610785199551/1.1338305413635981); theta1 0.6283185307179587 sage: theta2 = arctan(0.99114443967015675/-0.26557635219221642) + float(pi); theta2 1.8325957145940461
which are correct, to machine accuracy.
comment:161 Changed 5 years ago by
I should have been more careful. The graph isn't correct, is it? The endpoints are not correct. It's because of how things are displayed. Let me think how to correct this?
Changed 5 years ago by
Changed 5 years ago by
| Attachment: | bezier.png added |
|---|
comment:162 Changed 5 years ago by
The code
sage: a = arc((0,0),2,1,0,(pi/5,pi/2+pi/12), linestyle="--", color="red") sage: a
displays
While the code
sage: a = arc((0,0),2,1,0,(pi/5,pi/2+pi/12), linestyle="--", color="red") sage: b = a[0].bezier_path() sage: b
displays
Previously, I was trying to show that these two graphs are the same.
comment:163 follow-up: 164 Changed 5 years ago by
I guess it would help if they were on the same scale. The current display makes comparison difficult. I can believe they are the same but same scale would definitely look better.
comment:164 Changed 5 years ago by
Replying to fbissey:
I guess it would help if they were on the same scale. The current display makes comparison difficult. I can believe they are the same but same scale would definitely look better.
Yes, but that will require some investigating.
Changed 5 years ago by
| Attachment: | new_bezier+arc.png added |
|---|
comment:165 Changed 5 years ago by
sage: a = arc((0,0),2,1,0,(pi/5,pi/2+pi/12), linestyle="--", color="red")
sage: b = a[0].bezier_path()
sage: b.aspect_ratio()
'automatic'
sage: c = arc((0,0),2,1,0,(pi/5,pi/2+pi/12))
sage: c.aspect_ratio()
1.0
sage: c.set_aspect_ratio('automatic')
sage: h = c + b
sage: h
comment:166 Changed 5 years ago by
We'll have to start moving a bit faster. I will include stuff based on the last work by Steve. I will move all versions of sage in sage-on-gentoo to matplotlib 2.1.0 based on this ticket because 1.5.3 has been dropped.
comment:167 Changed 5 years ago by
| Commit: | 8f4ca6bfe57c467b5a13594f6699782068cf86c2 → 0db703433170a8a266151229194693242499da89 |
|---|
comment:168 Changed 5 years ago by
Perhaps ready for review? Or is how to deal with the classic style still up in the air?
comment:169 follow-up: 170 Changed 5 years ago by
| Status: | needs_work → needs_review |
|---|
There are still some questions I think, the classic style being one of them. But at this stage I think we should move forward and address new stuff in follow up tickets. There is bound to be some.
comment:170 Changed 5 years ago by
Replying to fbissey:
There are still some questions I think, the
classicstyle being one of them. But at this stage I think we should move forward and address new stuff in follow up tickets. There is bound to be some.
Thanks for the new commits. I agree that it becomes rather urgent to upgrade to Matplotlib 2.1.0. However,
somewhat independently of the classic style, there remains the computer modern font issue. As suggested by Steve in comment:129, it suffices to change one line (line 274) in the file
local/lib/python2.7/site-packages/matplotlib/mpl-data/matplotlibrc, namely change
mathtext.fontset : dejavusans # Should be 'dejavusans' (default),
to
mathtext.fontset : cm # Should be 'dejavusans' (default),
I guess this is feasible in the post-install phase, isn't it?
Without this, many users who are taking care of plots with nice labels, will be upset. Note that the change of the default to dejavusans generated some debate in the Matplotlib community, see e.g. https://github.com/matplotlib/matplotlib/issues/5860/ and https://husky.wordpress.com/2017/02/02/revert-matplotlib-2-0-mathtext-default-font-style-to-computer-modern/ .
comment:171 Changed 5 years ago by
Besides (but this may be delayed to a follow up ticket), I am not sure if we shall use the classic style while generating the doc. Actually, this yields many differences with what the user will get in a Sage session, as illustrated in this matplotlib page. I understand that using the classic style in sphinx_plot was a workaround to get png images generated with a decent quality (in this respect, compare the two images in the section Interpolation of the above page: it is hard to believe that the new default is a progess...). However to restore the png quality, it is not necessary to use the whole classic style, but simply to set the parameter image.interpolation to bilinear, i.e. we could replace the line
plt.style.use('classic')
in src/doc/common/conf.py by
import matplotlib as mpl mpl.rcParams['image.interpolation'] = 'bilinear' mpl.rcParams['image.resample'] = False
comment:172 follow-up: 173 Changed 5 years ago by
There is an option - which is not distro friendly - but could be applied post-install. Replacing the default matplotlibrc by the classic one. Possibly renaming the normal default one to something like default-2.1.0.
comment:173 Changed 5 years ago by
Replying to fbissey:
There is an option - which is not distro friendly - but could be applied post-install. Replacing the default matplotlibrc by the classic one. Possibly renaming the normal default one to something like default-2.1.0.
Note that what I proposed in comment:170 and comment:171 was something weaker: keep the 2.1.0 default style and simply restore the cm fonts for math display in plots.
However, what you propose here (i.e. using the classic style in all Sage) sounds reasonable to make a smooth transition for the users. This would also allow us to fix all issues with the new default style (in particular, the figure size issues) and use it after the transition period is over.
comment:174 Changed 5 years ago by
| Commit: | 0db703433170a8a266151229194693242499da89 → 41c21c6ac7a6fd7f2595129b33421ce5fb2c7521 |
|---|
comment:175 follow-up: 176 Changed 5 years ago by
I am not 100% happy about the work around but it will do for now. I apply it pre-install because at the end of the day it is far easier to deal with both python 2 and 3. But post-install would be best as the initial template is parsed and adjusted according to options. In fact it could be best to even bring the template back from 1.5.3 (with adjustments for new options and so on). Someone who is not working remotely should check the results.
comment:176 Changed 5 years ago by
Replying to fbissey:
Someone who is not working remotely should check the results.
OK, I'll check this.
comment:177 Changed 5 years ago by
| Reviewers: | → Eric Gourgoulhon, Dima Pasechnik |
|---|---|
| Status: | needs_review → positive_review |
I've checked on a fresh install. Everything is OK, both in the documentation and in Sage sessions (console + Jupyter).
Thank you very much for your work on this! IMHO, we can move on --> positive review.
comment:178 Changed 5 years ago by
| Status: | positive_review → needs_work |
|---|
sage -t --long --warn-long 78.8 src/sage/plot/arrow.py
**********************************************************************
File "src/sage/plot/arrow.py", line 364, in sage.plot.arrow.Arrow._render_on_subplot
Failed example:
two_stroke_re.search(contents) is None
Expected:
True
Got:
False
**********************************************************************
1 item had failures:
1 of 18 in sage.plot.arrow.Arrow._render_on_subplot
[69 tests, 1 failure, 6.16 s]
comment:179 Changed 5 years ago by
OK, that's weird, we specifically patch for this test. @Eric @Dima can you reproduce the error with the branch?
comment:180 Changed 5 years ago by
OK something changed in one of the latest revisions. The matching pattern doesn't work anymore.
comment:181 follow-up: 182 Changed 5 years ago by
It looks like there are some changes to arrow.py that didn't get committed, that may be the source of the trouble.
comment:182 Changed 5 years ago by
Replying to fbissey:
It looks like there are some changes to
arrow.pythat didn't get committed, that may be the source of the trouble.
No they are present, I was looking at a single commit rather than the branch.
What did it is the change to "classic style". This is annoying, the dashing stuff depends on the style. That makes it a fragile test in my book. I'd like to revisit it a little bit and document what's going on there a bit better.
comment:183 Changed 5 years ago by
OK, so I am now trying to insert the notion of matplotlib style sheet inside the plotting system and have it default to classic. I have redone the branch from scratch and I haven't corrected arrow.py for the dashing for now. I'd like to be sure I have everything under control.
I think the 2D plots are OK but I think I am missing a piece of info about 3D. If someone wants to confirm the stuff lives at u/fbissey/MPL-2.1.0b and I'll switch the ticket to it if other people think it is a good way to deal with our current issues.
comment:184 Changed 5 years ago by
For which 3D items should one look? Perhaps incorrectly, but I have applied the changes in u/fbisey/MPL-2.0b0 on top of this branch and docteting all under src/sage/plot/give no failures. Presumably this has the changes in arrow.py, at least they appear to be there.
comment:185 follow-up: 186 Changed 5 years ago by
With a pure branch MPL-2.1.0b (I am not sure how you applied that on top of MPL-2.1.0) I expect the arrow.py doctest to be broken (may be not - it depends on whether or not the previous changes were needed because of the new style sheet).
I am talking about the quality of the pictures. 2D pictures seems to be OK but I think 3D are a bit small. I suspect I need to apply a style sheet in a different place for 3D plots. I am looking for confirmation of this fact and whether people think it is a good approach.
comment:186 Changed 5 years ago by
Replying to fbissey:
I am looking for confirmation of this fact and whether people think it is a good approach.
I am looking at the new branch this morning. I'll be back in touch soon.
comment:187 Changed 5 years ago by
I've installed MPL-2.1.0 from scratch by pulling the new branch u/fbisey/MPL-2.0b0 (commit d109b7593) in a fresh Sage 8.2.beta2 (just built from a fresh git clone). I confirm that the 3D plots in the doc are of low quality, while the 2D ones are OK. I also confirm that the doctest failure pointed out by Volker in comment:178 is still there (not surprising since in comment:183, you say it is not corrected yet).
comment:188 follow-up: 189 Changed 5 years ago by
I think I understand now. 2D figures are directly produced by matplotlib but not 3D ones. For 3D an external program like jmol, tachyon or other is used. But for saving while doing the doc they are dumped from the external application to matplotlib. The only place for applying the style is sphinx_plot. Next we'll to work out what's happening with arrow.py.
comment:189 follow-up: 192 Changed 5 years ago by
Replying to fbissey:
I think I understand now. 2D figures are directly produced by matplotlib but not 3D ones. For 3D an external program like jmol, tachyon or other is used. But for saving while doing the doc they are dumped from the external application to matplotlib. The only place for applying the style is
sphinx_plot.
As I suggested in comment:171, maybe it is not necessary to change the style in sphinx_plot but
simply insert the lines
import matplotlib as mpl mpl.rcParams['image.interpolation'] = 'bilinear' mpl.rcParams['image.resample'] = False
just after
import matplotlib.pyplot as plt
in the code of sphinx_plot.
comment:190 follow-up: 191 Changed 5 years ago by
has anyone looked into using GR backend of Mathplotlib in Sage: https://gr-framework.org/tutorials/matplotlib.html ?
comment:191 follow-up: 193 Changed 5 years ago by
Replying to dimpase:
has anyone looked into using GR backend of Mathplotlib in Sage: https://gr-framework.org/tutorials/matplotlib.html ?
Not here at any rates. Feel free to submit a new optional/experimental package. That reminds me, I have to do something about the optional qhull spkg. Did you end up opening a ticket for it? We need to switch it from the custom basic makefile to cmake to fix your issues with it properly.
comment:192 Changed 5 years ago by
Replying to egourgoulhon:
Replying to fbissey:
I think I understand now. 2D figures are directly produced by matplotlib but not 3D ones. For 3D an external program like jmol, tachyon or other is used. But for saving while doing the doc they are dumped from the external application to matplotlib. The only place for applying the style is
sphinx_plot.As I suggested in comment:171, maybe it is not necessary to change the style in
sphinx_plotbut simply insert the linesimport matplotlib as mpl mpl.rcParams['image.interpolation'] = 'bilinear' mpl.rcParams['image.resample'] = Falsejust after
import matplotlib.pyplot as pltin the code of
sphinx_plot.
Yes, probably less ham-fisted. Although I'll check the interpolation to match the classic style.
comment:193 follow-up: 195 Changed 5 years ago by
Replying to fbissey:
That reminds me, I have to do something about the optional
qhullspkg. Did you end up opening a ticket for it? We need to switch it from the custom basic makefile to cmake to fix your issues with it properly.
I cannot recall the exact context of this. I do have trouble with scipy+qhull on FreeBSD, is it what you mean, or is it something else?
comment:194 Changed 5 years ago by
I finally got the new branch going. I always forget to make doc-clean && make to make sure the new matplotlib is used everywhere. The 3D are poorer quality and, if I've done things correctly, the generated .pngs are smaller - 512 x 384 compared to 640 x 480. I believe this was mentioned somewhere.
comment:195 Changed 5 years ago by
Replying to dimpase:
Replying to fbissey:
That reminds me, I have to do something about the optional
qhullspkg. Did you end up opening a ticket for it? We need to switch it from the custom basic makefile to cmake to fix your issues with it properly.I cannot recall the exact context of this. I do have trouble with scipy+qhull on FreeBSD, is it what you mean, or is it something else?
The problem is that matplotlib has its own copy of qhull but will happily use an already installed one. But the current spkg doesn't install library with names that matplotlib knows about. It detects qhull presence by headers and then fails to build because it cannot link. It is possible the issue with scipy is similar as scipy also ships its own qhull.
comment:196 Changed 5 years ago by
| Branch: | u/fbissey/MPL-2.1.0 → u/fbissey/MPL-2.1.0b |
|---|---|
| Commit: | 41c21c6ac7a6fd7f2595129b33421ce5fb2c7521 → 9b90efadd3a8aa3dfc2ecd77c3f8e538f20cf424 |
| Work issues: | → fix arrow.py and move qhull to cmake |
Switch the active branch on trac. 3D plots should look nice now. Still have to fix arrow.py again. Possibly do qhull as well.
New commits:
| 3c5d53c | Rebase MPL-2.1.0 upgrade branch
|
| d109b75 | WIP on inserting matplotlib style sheet as a plot option and defaulting it to classic
|
| 9b90efa | Set matplotlib options in sphinx_plot so that 3D plots look nice again.
|
comment:197 Changed 5 years ago by
I believe now when the arrowhead is drawn with linestyle='dashed' that the following will match
sage: two_stroke_pattern = r'setdash.*stroke.*stroke.*setdash.*setdash'
and it will not match when the arrowhead is draw solid. To draw the arrowhead with linestyle='dashed' I did
-
src/sage/plot/arrow.py
diff --git a/src/sage/plot/arrow.py b/src/sage/plot/arrow.py index f8d1e033f7..596a4ab3bf 100644
a b class Arrow(GraphicPrimitive): 437 437 pe1.draw_path(renderer, gc, tpath, affine, rgbFace) 438 438 439 439 pe1 = ConditionalStroke(CheckNthSubPath(p, 0), [pe.Stroke()]) 440 pe2 = ConditionalStroke(CheckNthSubPath(p, 1), [pe.Stroke( dashes={'dash_offset': 0, 'dash_list': None})])440 pe2 = ConditionalStroke(CheckNthSubPath(p, 1), [pe.Stroke()]) 441 441 p.set_path_effects([pe1, pe2]) 442 442 443 443 subplot.add_patch(p)
There may be a better way to do this.
comment:198 Changed 5 years ago by
| Commit: | 9b90efadd3a8aa3dfc2ecd77c3f8e538f20cf424 → 4005ec3dae5ab697d215d084d1e7bddea7ff110e |
|---|
Branch pushed to git repo; I updated commit sha1. New commits:
| 4005ec3 | Fix the arrow matching pattern
|
comment:199 Changed 5 years ago by
| Commit: | 4005ec3dae5ab697d215d084d1e7bddea7ff110e → 3ea7b1646ea138caf8e6b3651509c4711089cea0 |
|---|
Branch pushed to git repo; I updated commit sha1. New commits:
| 3ea7b16 | build qhull with cmake instead of custom script. This install can then be used by matplotlib.
|
comment:200 follow-ups: 201 206 Changed 5 years ago by
| Status: | needs_work → needs_review |
|---|---|
| Work issues: | fix arrow.py and move qhull to cmake |
Bonus, building with optional qhull spkg now works. So one more time in review!
comment:201 Changed 5 years ago by
Replying to fbissey:
Bonus, building with optional qhull spkg now works. So one more time in review!
Thank you Frédéric!
The 3D plots in the doc are now of good quality; however they are a little bit too small, which makes the axis labels difficult to read. To recover the same size as in Sage 8.1, it suffices to set some parameters in sphinx_plot. Following these suggestions, I've tried
-
src/doc/common/conf.py
diff --git a/src/doc/common/conf.py b/src/doc/common/conf.py index 5acdcda..3a6b650 100644
a b def sphinx_plot(plot): 26 26 from sage.misc.temporary_file import tmp_filename 27 27 import matplotlib.pyplot as plt 28 28 import matplotlib as mpl 29 mpl.rcParams['image.interpolation'] = 'bilinear'30 mpl.rcParams['image.resample'] = False31 29 if os.environ.get('SAGE_SKIP_PLOT_DIRECTIVE', 'no') != 'yes': 30 mpl.rcParams['image.interpolation'] = 'bilinear' 31 mpl.rcParams['image.resample'] = False 32 mpl.rcParams['figure.figsize'] = [8.0, 6.0] 33 mpl.rcParams['figure.dpi'] = 80 34 mpl.rcParams['savefig.dpi'] = 100 32 35 fn = tmp_filename(ext=".png") 33 36 plot.plot().save(fn) 34 37 img = mpimg.imread(fn)
and it works well: the original (i.e. Sage 8.1) size is restored (side note: all settings of mpl.rcParams are moved to the if block, for there is no need to run them if no plot is to be generated).
comment:202 follow-up: 203 Changed 5 years ago by
It's François actually, not Frederick. Anyway it starts to fell like setting the style would be shorter but I don't really mind that much at this stage. Incoming.
comment:203 Changed 5 years ago by
Replying to fbissey:
It's François actually, not Frederick.
My apologies about this! I know it is François, but I had just sent an e-mail to a real Frédéric, so I was still in some "Frédéric mode".
comment:204 Changed 5 years ago by
| Commit: | 3ea7b1646ea138caf8e6b3651509c4711089cea0 → 3a6792ea300002114fc701acfb14ea52f72fe040 |
|---|
Branch pushed to git repo; I updated commit sha1. New commits:
| 3a6792e | Setting more parameters in sphinx_plot for better 3D documentation
|
comment:205 follow-up: 207 Changed 5 years ago by
| Status: | needs_review → positive_review |
|---|
Thanks for the change in sphinx_plot. The 3D plots in the doc are OK now.
Moreover, make ptestlong reveals no error. Hence positive review. Dima, do you agree?
comment:206 follow-up: 208 Changed 5 years ago by
Replying to fbissey:
Bonus, building with optional qhull spkg now works. So one more time in review!
I think we need a follow-up ticket for the following:
It seems that we need sdh_cmake in addition to sdh_configure etc we already have in
src/bin/sage-dist-helpers. This would make spkg-install scripts where cmake is used more uniform and less verbose.
And spkg-install ought to use already available sdh_* things, too.
E.g. after cmake call one would only need something like
sdh_make sdh_make_install
comment:207 follow-up: 209 Changed 5 years ago by
Replying to egourgoulhon:
Thanks for the change in
sphinx_plot. The 3D plots in the doc are OK now. Moreover,make ptestlongreveals no error. Hence positive review. Dima, do you agree?
Do we want an OSX test?
comment:208 Changed 5 years ago by
Replying to dimpase:
Replying to fbissey:
Bonus, building with optional qhull spkg now works. So one more time in review!
I think we need a follow-up ticket for the following:
It seems that we need
sdh_cmakein addition tosdh_configureetc we already have insrc/bin/sage-dist-helpers. This would makespkg-installscripts wherecmakeis used more uniform and less verbose.
Sure, but I think we'll only get traction on that when cmake is standard. But I thought strongly about it too.
And
spkg-installought to use already availablesdh_*things, too. E.g. aftercmakecall one would only need something likesdh_make sdh_make_install
I should have thought of that. I'll just tweak it.
comment:209 Changed 5 years ago by
Replying to dimpase:
Replying to egourgoulhon:
Thanks for the change in
sphinx_plot. The 3D plots in the doc are OK now. Moreover,make ptestlongreveals no error. Hence positive review. Dima, do you agree?Do we want an OSX test?
Yes please. I am not expecting any bad things but I like some insurance.
comment:210 Changed 5 years ago by
| Commit: | 3a6792ea300002114fc701acfb14ea52f72fe040 → 929509edcfe5278337087f280222fc9b7bb10bbd |
|---|---|
| Status: | positive_review → needs_review |
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
| 929509e | use helper scripts in qhull
|
comment:211 Changed 5 years ago by
probably something for another follow-up ticket. Running headless on OSX 10.13, atop of the clang branch (from the epic clang ticket), I get
sage -t --long --warn-long 476.0 src/sage/plot/plot.py
**********************************************************************
File "src/sage/plot/plot.py", line 464, in sage.plot.plot
Failed example:
P = plt.plot(t, s, linewidth=1.0)
Expected nothing
Got:
_RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL.
which looks like a regression to me.
It can be reproduced at Sage prompt.
comment:212 follow-up: 214 Changed 5 years ago by
It feels like on OS X matplotlib may want to default the renderer to something OS X specific. I am assuming you have a batch of those, not just the one? This is similar to stuff solved by this patch https://anonscm.debian.org/cgit/debian-science/packages/sagemath.git/tree/debian/patches/u1-fix-dont-require-DISPLAY.patch in debian and I think there was ticket on trac about DISPLAY recently.
comment:213 Changed 5 years ago by
Is SAGE_MATPLOTLIB_GUI set to no when you build headless? If not you should.
comment:214 Changed 5 years ago by
Replying to fbissey:
It feels like on OS X matplotlib may want to default the renderer to something OS X specific. I am assuming you have a batch of those, not just the one?
no, it happens just once; while I test src/sage/plot I get just one error. At the Sage prompt I get
sage: import pylab as plt sage: t = plt.arange(0.0, 2.0, 0.01) sage: s = sin(2*pi*t) sage: P = plt.plot(t, s) _RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL.
It's some kind of initialisation error: if I re-run the last line then no 2nd error message is shown.
Note that it is indeed similar to this:
This is similar to stuff solved by this patch https://anonscm.debian.org/cgit/debian-science/packages/sagemath.git/tree/debian/patches/u1-fix-dont-require-DISPLAY.patch in debian and I think there was ticket on trac about
DISPLAYrecently.
If I run (in a fresh Sage session)
sage: import matplotlib; matplotlib.use('Agg')
sage: import pylab as plt
sage: t = plt.arange(0.0, 2.0, 0.01)
sage: s = sin(2*pi*t)
sage: P = plt.plot(t, s)
I see no error message.
This could be a clang-only issue, I don't know.
comment:215 Changed 5 years ago by
OK, surprising but then debian didn't patch every single call either. It would be nice to figure the trigger for a window.
comment:216 follow-up: 217 Changed 5 years ago by
It comes somewhere from the dark depths of matplotlib, this error message. Perhaps just add
import matplotlib; matplotlib.use('Agg')
right before this doctest and be done with?
comment:217 Changed 5 years ago by
Replying to dimpase:
It comes somewhere from the dark depths of matplotlib, this error message. Perhaps just add
import matplotlib; matplotlib.use('Agg')right before this doctest and be done with?
Sure, but in that case, I'll do the debian guys a favor and fix theirs too.
comment:218 Changed 5 years ago by
By the way, what we see here on OSX is pure matplotlib issue. In ./sage --ipython I see this error message just as well.
In [1]: import pylab as plt In [2]: t = plt.arange(0.0, 2.0, 0.01) In [3]: plt.plot(t,t) _RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL. Out[3]: [<matplotlib.lines.Line2D at 0x10d1446d0>] In [4]: plt.plot(t,t) Out[4]: [<matplotlib.lines.Line2D at 0x10d188a90>]
comment:219 follow-up: 220 Changed 5 years ago by
I never doubted it. I am also quite sure that if you build matplotlib with SAGE_MATPLOTLIB_GUI=no you'll be in the clear too. Because that's probably linked to the macosx backend like the debian guy issue wouldn't happen if they weren't building all the possible GUI backends.
comment:220 Changed 5 years ago by
Replying to fbissey:
I never doubted it. I am also quite sure that if you build matplotlib with
SAGE_MATPLOTLIB_GUI=noyou'll be in the clear too. Because that's probably linked to themacosxbackend like the debian guy issue wouldn't happen if they weren't building all the possible GUI backends.
With matplotlib-1.5.1 the default Sage backend was agg. Now with 2.1.0 the default is tkagg. So maybe there is something wrong with tkagg.
comment:221 Changed 5 years ago by
Really my last build shows that in matplotlibrc I have (without doing any special GUI settings)
#### CONFIGURATION BEGINS HERE # The default backend; one of GTK GTKAgg GTKCairo GTK3Agg GTK3Cairo # MacOSX Qt4Agg Qt5Agg TkAgg WX WXAgg Agg Cairo GDK PS PDF SVG # Template. # You can also deploy your own backend outside of matplotlib by # referring to the module name (which must be in the PYTHONPATH) as # 'module://my_backend'. # # If you omit this parameter, it will always default to "Agg", which is a # non-interactive backend. backend : agg
Now I bet Dima has macosx instead.
comment:222 Changed 5 years ago by
I didn't think I had done anything special either, but with this branch
sage: import matplotlib sage: matplotlib.get_backend() u'TkAgg'
comment:223 Changed 5 years ago by
OK, I have agg. And I don't have tcl/tk installed. I am wondering why I don't have some of the other backends for which you would think I have the dependencies.
comment:224 Changed 5 years ago by
I do have system tcl/tk installed. So maybe it is picking up on that. From the configure
OPTIONAL BACKEND EXTENSIONS
macosx: no [skipping due to configuration]
qt5agg: no [PySide2 not found; PyQt5 not found]
qt4agg: no [PySide not found; PyQt4 not found]
gtk3agg: no [Requires pygobject to be installed.]
gtk3cairo: no [Requires cairocffi or pycairo to be installed.]
gtkagg: no [skipping due to configuration]
tkagg: yes [installing; run-time loading from Python Tcl /
Tk]
wxagg: no [skipping due to configuration]
gtk: no [skipping due to configuration]
agg: yes [installing]
cairo: no [cairocffi or pycairo not found]
windowing: no [skipping due to configuration]
comment:225 Changed 5 years ago by
It probably does. tkagg is one of the backend set to auto in setup.cfg. It looks like gtk and gtkagg need pygtk. wxagg needs wxpython. Because all that stuff is actually some kind of python bindings it would have to be installed in sage in the first place. I guess some can be installed by calling sage's pip but OS specific and tkagg are the only ones you can really get out of the box.
comment:226 Changed 5 years ago by
So, should agg be the default and let the user change to whatever? I believe this would fix things on OSX.
comment:227 follow-up: 228 Changed 5 years ago by
I am not sure how things are ordered when several are available, I'll try to find out. However agg should always be the default in non-interactive mode. Dima and debian's problems may actually be subtle upstream bug of selection of the backend.
comment:228 Changed 5 years ago by
Replying to fbissey:
I am not sure how things are ordered when several are available, I'll try to find out. However
aggshould always be the default in non-interactive mode. Dima and debian's problems may actually be subtle upstream bug of selection of the backend.
I can replicate the debian problem on gentoo by simply unsetting the DISPLAY and doctesting src/sage/plot/plot.py and src/sage/probability/probability_distribution.pyx. I've seen this may times with sage-on-gentoo on Prefix where I forget to forward my DISPLAY when logging into the Prefix. I don't know about OSX, it looks like a different issue. It may be best to either configure Sage to have agg as the default matplotlib backend (which can be done) or to make sure that prior to using pylab or pyplot in a doctest the matplotlib backend is appropriately set.
comment:229 Changed 5 years ago by
another OSX failure of this type is here:
sage -t --long --warn-long 475.8 src/sage/probability/probability_distribution.pyx
**********************************************************************
File "src/sage/probability/probability_distribution.pyx", line 120, in sage.probability.probability_distribution.ProbabilityDistribut
ion.generate_histogram_data
Failed example:
h, b = X.generate_histogram_data(bins = 10)
Expected nothing
Got:
_RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL.
In case, on OSX the default backend is also TkAgg.
IMHO the right place to set it to Agg is in the testsuite settings.
Then no doctstring patches would be needed (there patches are not very useful as far
as documentation is concerned, as normally speaking one does not want Agg).
comment:230 follow-up: 231 Changed 5 years ago by
+1 to that. Do you have a clue where that should be set.
comment:231 Changed 5 years ago by
Replying to fbissey:
+1 to that. Do you have a clue where that should be set.
Somewhere in src/sage/doctest/control.py, but where exactly?
Jeroen, could you tell us? We'd like the doctests to run with import matplotlib; matplotlib.use('Agg') executed.
comment:232 Changed 5 years ago by
| Commit: | 929509edcfe5278337087f280222fc9b7bb10bbd → 115842500d43cc3866c27292cf5680a317b5da3f |
|---|
Branch pushed to git repo; I updated commit sha1. New commits:
| 1158425 | Make sure agg backend is set in matplotlib when doctests are run
|
comment:233 Changed 5 years ago by
After some thinking I came to the conclusion that it would probably be a burden to add the statement to every single doctests. Which is probably why the settings for the doctests are minimal. While I was looking for something else I noticed something neat about jmol and doctesting. I think it can be adapted here so it will only be effective on matplotlib plots.
Have a go with the new commit!
comment:234 Changed 5 years ago by
This seems to throw a bunch of warnings:
./sage -t --long src/sage/plot/plot.py
**********************************************************************
File "src/sage/plot/plot.py", line 92, in sage.plot.plot
Failed example:
plot(x^2, (x,0,5))
Expected:
Graphics object consisting of 1 graphics primitive
Got:
doctest:warning
...
UserWarning:
This call to matplotlib.use() has no effect because the backend has already
been chosen; matplotlib.use() must be called *before* pylab, matplotlib.pyplot,
or matplotlib.backends is imported for the first time.
...
Graphics object consisting of 1 graphics primitive
This was the only failure in plot.py.
comment:235 Changed 5 years ago by
The UserWarning was not complete. This is what was returned
UserWarning:
This call to matplotlib.use() has no effect because the backend has already
been chosen; matplotlib.use() must be called *before* pylab, matplotlib.pyplot,
or matplotlib.backends is imported for the first time.
<BLANKLINE>
The backend was *originally* set to u'TkAgg' by the following code:
comment:236 Changed 5 years ago by
Hum... Can you move the code in the commit up a few lines so it is above the other code I introduced to check stylesheet and try again? That block contains an import to matplotlib.pyplot which could be responsible for the message.
comment:237 Changed 5 years ago by
Yes, good spot, that gets rid of the warnings. Unfortunately I'm unable to test the OSX failures.
comment:238 Changed 5 years ago by
| Commit: | 115842500d43cc3866c27292cf5680a317b5da3f → 8dcadff0d0dcc86aff0111749f85082f888afc7a |
|---|
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
| 8dcadff | Make sure agg backend is set in matplotlib when doctests are run and put the call in the right spot
|
comment:239 Changed 5 years ago by
I have now redone the commit in the right spot. So Dima should be able to give it a go. Thanks for the testing (that I should have done).
comment:240 Changed 5 years ago by
Good, src/sage/plot/plot.py now passes the tests on OSX, but I still see
**********************************************************************
File "src/sage/probability/probability_distribution.pyx", line 120, in sage.probability.probability_distribution.ProbabilityDistribution.generate_histogram_data
Failed example:
h, b = X.generate_histogram_data(bins = 10)
Expected nothing
Got:
_RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL.
I suppose you need to put your agg setting even higher.
Otherwise this setting has be done per file, which is kind of silly, IMHO.
comment:241 Changed 5 years ago by
If I do
--- a/src/sage/doctest/control.py
+++ b/src/sage/doctest/control.py
@@ -42,6 +42,8 @@ optionaltag_regex = re.compile(r'^\w+$')
# Optional tags which are always automatically added
auto_optional_tags = set(['py2' if six.PY2 else 'py3'])
+import matplotlib
+matplotlib.use('agg')
class DocTestDefaults(SageObject):
"""
then all the tests work.
Needless to say, this does not affect normal running of Sage, as doctesting is not imported, unless you actually do doctesting.
comment:242 Changed 5 years ago by
Right, I had a look and several functions of probability_distribution.pyx starts with import pylab which would take effect before my own call. I guess there is nothing for it. Let's put it in control.py but I think there is an issue that should be addressed in matplotlib if we could figure a simple test case not involving sage to open an issue.
comment:243 follow-up: 244 Changed 5 years ago by
one way or another, "real" Sage library code should not be polluted by checks of whether is is being doctested.
comment:244 Changed 5 years ago by
Replying to dimpase:
one way or another, "real" Sage library code should not be polluted by checks of whether is is being doctested.
A laudable goal but not always reachable it seems https://github.com/sagemath/sage/blob/master/src/sage/structure/graphics_file.py#L175
I'll be getting code in the branch shortly (first day back to work).
comment:245 Changed 5 years ago by
| Commit: | 8dcadff0d0dcc86aff0111749f85082f888afc7a → e0534a8c68bc60cf2495acf32598096d64579184 |
|---|
comment:246 Changed 5 years ago by
the last commit leads to two test failures in src/sage/tests/startup.py - as the import statement in the last commit makes both
sage: 'numpy' in sys.modules
False
sage: 'pyparsing' in sys.modules
False
fail. I don't know the purpose of these examples - just showing the functionality? We can change numpy to, say, sympy, and pyparsing to something else, too...
comment:247 follow-up: 248 Changed 5 years ago by
You should read the whole section
Ensure that certain modules are not loaded on startup.
EXAMPLES::
sage: 'numpy' in sys.modules
False
sage: 'pyparsing' in sys.modules
False
sage: 'sage.libs.gap.libgap' in sys.modules
False
Check that IPython is not imported at startup (:trac:`18726`). It is
imported by the doctest framework, so the simple test like above would
not work. Instead, we test this by starting a new Python process::
sage: from sage.tests.cmdline import test_executable
sage: cmd = "from sage.all import *\nprint('IPython' in sys.modules)\n"
sage: print(test_executable(["sage", "--python"], cmd)[0]) # long time
False
basically we check that numpy and pyparsing are not imported at startup because it slows down the startup process considerably. Because of the manipulation of the last commit the tests would need to be made similar to the one for ipython.
I am feeling like I am playing a game of "whack a mole". I may abandon the most general way to just care for the particular as it is clearly less exhausting.
comment:248 follow-up: 249 Changed 5 years ago by
Replying to fbissey:
I am feeling like I am playing a game of "whack a mole". I may abandon the most general way to just care for the particular as it is clearly less exhausting.
I agree with this. There is clearly something awry with OSX that maybe should be on another ticket as long as the specific case here is addressed.
comment:249 Changed 5 years ago by
Replying to strogdon:
Replying to fbissey:
I am feeling like I am playing a game of "whack a mole". I may abandon the most general way to just care for the particular as it is clearly less exhausting.
I agree with this. There is clearly something awry with OSX that maybe should be on another ticket as long as the specific case here is addressed.
Well, the change I am proposing should actually as well take care of the problem that Debian people are patching, no? And this is actually a noninvasive way; otherwise Sage starts looking a bit like that famous VW dieselgate code (for running their diesel engines in test mode differently) should have looked...
comment:250 follow-up: 251 Changed 5 years ago by
Testing something different in test/startup.py? That misses the point the test was introduced for. https://github.com/sagemath/sage/commit/bff6915f037ef1d2acbda426f96cb6060509901c last file on that commit.
comment:251 Changed 5 years ago by
Replying to fbissey:
Testing something different in
test/startup.py? That misses the point the test was introduced for. https://github.com/sagemath/sage/commit/bff6915f037ef1d2acbda426f96cb6060509901c last file on that commit.
I am fine with doing tests for numpy and pyparsing the IPython way, as you proposed. One example how to do this in the normal setting (with libgap) looks sufficient.
comment:252 Changed 5 years ago by
| Commit: | e0534a8c68bc60cf2495acf32598096d64579184 → d5be931f48b77bb03d2d5750bea3805435e795fd |
|---|
Branch pushed to git repo; I updated commit sha1. New commits:
| d5be931 | Change the startup test for numpy and pyparsing now that they are imported in the doctesting framework via matplotlib.
|
comment:254 follow-up: 256 Changed 5 years ago by
Why not use a test like
sage: cmd = "print('numpy' in sys.modules)"
sage: print(test_executable(["sage", "-c", cmd])[0]) # long time
False
Doesn't that test this issue more directly? On my computer, this one might be a little faster, too.
comment:255 Changed 5 years ago by
(I think the current test works, so we can leave it as is, and consider changes on another ticket.)
comment:256 follow-up: 258 Changed 5 years ago by
Replying to jhpalmieri:
Why not use a test like
sage: cmd = "print('numpy' in sys.modules)" sage: print(test_executable(["sage", "-c", cmd])[0]) # long time FalseDoesn't that test this issue more directly? On my computer, this one might be a little faster, too.
Now you are making me think that the ipython test that I copied is actually wrong. sage --python should start a python shell not sage. Correct?
comment:257 Changed 5 years ago by
No, it is correct but yes something more direct looks possible. I should look at the ticket to see why this particular form has been chosen.
comment:258 Changed 5 years ago by
Replying to fbissey:
Replying to jhpalmieri:
Why not use a test like
sage: cmd = "print('numpy' in sys.modules)" sage: print(test_executable(["sage", "-c", cmd])[0]) # long time FalseDoesn't that test this issue more directly? On my computer, this one might be a little faster, too.
Now you are making me think that the ipython test that I copied is actually wrong.
sage --pythonshould start a python shell not sage. Correct?
This is correct, and in fact starting anything short of full Sage is Dieselgate approach to testing...
comment:259 Changed 5 years ago by
| Commit: | d5be931f48b77bb03d2d5750bea3805435e795fd → ba7f0ab8dbde03cdd935233fc4228451b2cb0c87 |
|---|
comment:260 follow-up: 261 Changed 5 years ago by
OK changed to sage -c by popular demand. Rebased on top of 8.2.beta4. Should we have a new positive review now!
comment:261 Changed 5 years ago by
Replying to fbissey:
OK changed to
sage -cby popular demand. Rebased on top of 8.2.beta4. Should we have a new positive review now!
LGTM (I've just checked it on Ubuntu, not on MacOS). Thanks again for all your work! (this is comment number 261, pfff!...).
comment:262 Changed 5 years ago by
| Status: | needs_review → positive_review |
|---|
OK, looks good on OSX too.
comment:263 Changed 5 years ago by
| Status: | positive_review → needs_work |
|---|
Sorry for removing the positive_review status, but I've just noticed the following doctest errors:
./sage -t --long --warn-long 46.0 src/sage/tests/startup.py
Running doctests with ID 2018-01-31-14-05-27-cae74607.
Git branch: MPL2.1.0
Using --optional=mpir,python2,sage
Doctesting 1 file.
sage -t --long --warn-long 46.0 src/sage/tests/startup.py
**********************************************************************
File "src/sage/tests/startup.py", line 25, in sage.tests.startup
Failed example:
print(test_executable(["sage", "-c"], cmd)[0]) # long time
Expected:
False
Got:
<BLANKLINE>
**********************************************************************
File "src/sage/tests/startup.py", line 28, in sage.tests.startup
Failed example:
print(test_executable(["sage", "-c"], cmd)[0]) # long time
Expected:
False
Got:
<BLANKLINE>
**********************************************************************
1 item had failures:
2 of 10 in sage.tests.startup
[9 tests, 2 failures, 4.22 s]
----------------------------------------------------------------------
sage -t --long --warn-long 46.0 src/sage/tests/startup.py # 2 doctests failed
----------------------------------------------------------------------
This is with Ubuntu 16.04.
comment:264 Changed 5 years ago by
In a Sage console, I indeed get a blank line for the same command:
sage: from sage.tests.cmdline import test_executable
sage: cmd = "print('numpy' in sys.modules)\n"
sage: print(test_executable(["sage", "-c"], cmd)[0])
sage: test_executable(["sage", "-c"], cmd)
('', '', 0)
while I do have:
sage: 'numpy' in sys.modules False
In the Linux terminal, ./sage -c "print('numpy' in sys.modules)" returns False.
comment:265 follow-up: 266 Changed 5 years ago by
Please try the test the way I suggested in comment:254 (["sage", "-c", cmd] all in one set of square brackets). Does that work?
comment:266 Changed 5 years ago by
Replying to jhpalmieri:
Please try the test the way I suggested in comment:254 (
["sage", "-c", cmd]all in one set of square brackets). Does that work?
Yes this one works:
sage: from sage.tests.cmdline import test_executable
sage: cmd = "print('numpy' in sys.modules)\n"
sage: print(test_executable(["sage", "-c", cmd])[0])
False
sage: test_executable(["sage", "-c", cmd])
('False\n', '', 0)
comment:268 Changed 5 years ago by
| Commit: | ba7f0ab8dbde03cdd935233fc4228451b2cb0c87 → ca3b242e6d7ca2ce22d8d23ef62d0cecd95b585f |
|---|
comment:270 Changed 5 years ago by
| Status: | needs_review → positive_review |
|---|
OK, I've run the tests on the newest branch now. Apologies for a mis-review the previous time - I only checked files that didn't work before...
comment:271 Changed 5 years ago by
It's also OK from my side: make ptestlong ==> "All tests passed!"
Many thanks!
comment:272 Changed 5 years ago by
| Branch: | u/fbissey/MPL-2.1.0b → ca3b242e6d7ca2ce22d8d23ef62d0cecd95b585f |
|---|---|
| Resolution: | → fixed |
| Status: | positive_review → closed |
comment:273 Changed 5 years ago by
| Commit: | ca3b242e6d7ca2ce22d8d23ef62d0cecd95b585f |
|---|
See #24670 for a follow-up.
comment:275 Changed 5 years ago by
Can somebody explain these changes?
@@ -141,7 +140,7 @@ def list_plot3d(v, interpolation_type='default', texture="automatic", point_list sage: for i in range(-5, 5): ....: for j in range(-5, 5): ....: l.append((normalvariate(0, 1), normalvariate(0, 1), normalvariate(0, 1))) - sage: list_plot3d(l, interpolation_type='nn', texture='yellow', num_points=100) + sage: list_plot3d(l, interpolation_type='clough', texture='yellow', num_points=100) Graphics3d Object TESTS:
I'm asking because of this:
sage -t --long --warn-long 10.0 src/sage/plot/plot3d/list_plot3d.py
**********************************************************************
File "src/sage/plot/plot3d/list_plot3d.py", line 143, in sage.plot.plot3d.list_plot3d.list_plot3d
Warning, slow doctest:
list_plot3d(l, interpolation_type='clough', texture='yellow', num_points=100)
Test ran for 926.47 s
**********************************************************************
926 seconds for a plot which is not even marked # long time?
comment:276 follow-up: 277 Changed 5 years ago by
Clearly an oversight on my part. interpolation_type='nn' is gone - and not coming back anytime soon. This was discussed at length in this ticket and I pinged sage-devel. Following deafening silence on the options available, I choose a new alternative that looked promising time wise on the example that I timed. As you just shown I haven't timed every single doctests otherwise I may have noticed that this one was putting the new default under stress.
That warrants a follow up ticket to deal with this.
comment:277 Changed 5 years ago by
Replying to fbissey:
Clearly an oversight on my part.
interpolation_type='nn'is gone - and not coming back anytime soon. This was discussed at length in this ticket and I pinged sage-devel. Following deafening silence on the options available, I choose a new alternative that looked promising time wise on the example that I timed. As you just shown I haven't timed every single doctests otherwise I may have noticed that this one was putting the new default under stress.
I'm not blaiming you... I didn't follow this ticket but given the number of comments, I guess it was not easy.
comment:278 Changed 5 years ago by
In 8.2.beta3, the same test took about 3 seconds on the same machine. So we have a regression by a factor 300.
comment:279 Changed 5 years ago by
OK, let's move on to do something about it. Is there any other plots that are taking an awful long time?
comment:283 Changed 5 years ago by
Replying to gh-dimpase:
By the way, matplotlib 2.2.0 has been released.
Is it python-3 only? I remember reading they were getting ready for that at the next "major" release.
I'll have to look at debian's patches sometimes tomorrow. I backed out of 2.0.2 in sage-on-gentoo a few weeks ago
sage -t --long /usr/lib64/python2.7/site-packages/sage/plot/plot.py ********************************************************************** File "/usr/lib64/python2.7/site-packages/sage/plot/plot.py", line 483, in sage.plot.plot Failed example: plt.savefig(os.path.join(SAGE_TMP, 'foo.png')) Exception raised: Traceback (most recent call last): File "/usr/lib64/python2.7/site-packages/sage/doctest/forker.py", line 518, in _run self.compile_and_execute(example, compiler, test.globs) File "/usr/lib64/python2.7/site-packages/sage/doctest/forker.py", line 888, in compile_and_execute exec(compiled, globs) File "<doctest sage.plot.plot[77]>", line 1, in <module> plt.savefig(os.path.join(SAGE_TMP, 'foo.png')) File "/usr/lib64/python2.7/site-packages/matplotlib/pyplot.py", line 697, in savefig res = fig.savefig(*args, **kwargs) File "/usr/lib64/python2.7/site-packages/matplotlib/figure.py", line 1573, in savefig self.canvas.print_figure(*args, **kwargs) File "/usr/lib64/python2.7/site-packages/matplotlib/backend_bases.py", line 2252, in print_figure **kwargs) File "/usr/lib64/python2.7/site-packages/matplotlib/backends/backend_agg.py", line 545, in print_png FigureCanvasAgg.draw(self) File "/usr/lib64/python2.7/site-packages/matplotlib/backends/backend_agg.py", line 464, in draw self.figure.draw(self.renderer) File "/usr/lib64/python2.7/site-packages/matplotlib/artist.py", line 63, in draw_wrapper draw(artist, renderer, *args, **kwargs) File "/usr/lib64/python2.7/site-packages/matplotlib/figure.py", line 1144, in draw renderer, self, dsu, self.suppressComposite) File "/usr/lib64/python2.7/site-packages/matplotlib/image.py", line 139, in _draw_list_compositing_images a.draw(renderer) File "/usr/lib64/python2.7/site-packages/matplotlib/artist.py", line 63, in draw_wrapper draw(artist, renderer, *args, **kwargs) File "/usr/lib64/python2.7/site-packages/matplotlib/axes/_base.py", line 2426, in draw mimage._draw_list_compositing_images(renderer, self, dsu) File "/usr/lib64/python2.7/site-packages/matplotlib/image.py", line 139, in _draw_list_compositing_images a.draw(renderer) File "/usr/lib64/python2.7/site-packages/matplotlib/artist.py", line 63, in draw_wrapper draw(artist, renderer, *args, **kwargs) File "/usr/lib64/python2.7/site-packages/matplotlib/image.py", line 543, in draw renderer, renderer.get_image_magnification()) File "/usr/lib64/python2.7/site-packages/matplotlib/image.py", line 770, in make_image unsampled=unsampled) File "/usr/lib64/python2.7/site-packages/matplotlib/image.py", line 426, in _make_image self.get_filternorm() or 0.0, self.get_filterrad() or 0.0) ValueError: 3-dimensional arrays must be of dtype unsigned byte, unsigned short, float32 or float64 **********************************************************************sage -t --long /usr/lib64/python2.7/site-packages/sage/plot/arrow.py ********************************************************************** File "/usr/lib64/python2.7/site-packages/sage/plot/arrow.py", line 364, in sage.plot.arrow.Arrow._render_on_subplot Failed example: two_stroke_re.search(contents) is None Expected: True Got: False **********************************************************************and
sage -t --long /usr/lib64/python2.7/site-packages/sage/probability/probability_distribution.pyx ********************************************************************** File "/usr/lib64/python2.7/site-packages/sage/probability/probability_distribution.pyx", line 120, in sage.probability.probability_distribution.ProbabilityDistribution.generate_histogram_data Failed example: h, b = X.generate_histogram_data(bins = 10) Expected nothing Got: doctest:warning File "/usr/lib/python-exec/python2.7/sage-runtests", line 98, in <module> err = DC.run() File "/usr/lib64/python2.7/site-packages/sage/doctest/control.py", line 1099, in run self.run_doctests() File "/usr/lib64/python2.7/site-packages/sage/doctest/control.py", line 824, in run_doctests self.dispatcher.dispatch() File "/usr/lib64/python2.7/site-packages/sage/doctest/forker.py", line 1777, in dispatch self.parallel_dispatch() File "/usr/lib64/python2.7/site-packages/sage/doctest/forker.py", line 1667, in parallel_dispatch w.start() # This might take some time File "/usr/lib64/python2.7/site-packages/sage/doctest/forker.py", line 1943, in start super(DocTestWorker, self).start() File "/usr/lib64/python2.7/multiprocessing/process.py", line 130, in start self._popen = Popen(self) File "/usr/lib64/python2.7/multiprocessing/forking.py", line 126, in __init__ code = process_obj._bootstrap() File "/usr/lib64/python2.7/multiprocessing/process.py", line 258, in _bootstrap self.run() File "/usr/lib64/python2.7/site-packages/sage/doctest/forker.py", line 1916, in run task(self.options, self.outtmpfile, msgpipe, self.result_queue) File "/usr/lib64/python2.7/site-packages/sage/doctest/forker.py", line 2218, in __call__ result = runner.run(test) File "/usr/lib64/python2.7/site-packages/sage/doctest/forker.py", line 663, in run return self._run(test, compileflags, out) File "/usr/lib64/python2.7/site-packages/sage/doctest/forker.py", line 518, in _run self.compile_and_execute(example, compiler, test.globs) File "/usr/lib64/python2.7/site-packages/sage/doctest/forker.py", line 888, in compile_and_execute exec(compiled, globs) File "<doctest sage.probability.probability_distribution.ProbabilityDistribution.generate_histogram_data[3]>", line 1, in <module> h, b = X.generate_histogram_data(bins = Integer(10)) File "/usr/lib64/python2.7/site-packages/matplotlib/pyplot.py", line 3075, in hist mplDeprecation) : MatplotlibDeprecationWarning: The 'hold' keyword argument is deprecated since 2.0. **********************************************************************which is solved by #23696.