Opened 5 years ago

Closed 5 years ago

Update matplotlib to 2.1.0

Reported by: Owned by: dimpase major sage-8.2 packages: standard fbissey, strogdon, egourgoulhon, charpent, chapoton François Bissey, Steven Trogdon Eric Gourgoulhon, Dima Pasechnik None of the above - read trac for reasoning. ca3b242

GitHub link to the corresponding issue

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:

comment:2 Changed 5 years ago by fbissey

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

comment:3 Changed 5 years ago by dimpase

Dependencies: → #23689

Surely you meant #23689 (added to deps now).

comment:4 Changed 5 years ago by fbissey

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 jdemeyer

Dependencies: #23689 → #23689, #23711

I'm adding #23711 as dependency because it might be relevant to this ticket.

comment:7 Changed 5 years ago by jdemeyer

Dependencies: #23689, #23711

comment:8 Changed 5 years ago by jdemeyer

Summary: update matplotlib to 2.0.2 → Update matplotlib to 2.1.0

comment:9 Changed 5 years ago by jdemeyer

Description: modified (diff)

comment:10 Changed 5 years ago by jdemeyer

Description: modified (diff)

comment:11 follow-up:  12 Changed 5 years ago by fbissey

        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 in reply to:  11 Changed 5 years ago by strogdon

        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.

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.

Last edited 5 years ago by strogdon (previous) (diff)

comment:13 Changed 5 years ago by fbissey

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 strogdon

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 strogdon

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 strogdon

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])



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 fbissey

What does two_stroke_re.search(contents) says (similar to https://trac.sagemath.org/ticket/23696#comment:12)?

comment:18 in reply to:  17 Changed 5 years ago by strogdon

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 fbissey

Time to inspect the .eps file and figure out what we are looking for. But first, lunch :)

comment:20 Changed 5 years ago by strogdon

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 strogdon

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.

Last edited 5 years ago by strogdon (previous) (diff)

comment:22 Changed 5 years ago by fbissey

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 strogdon

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])



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 fbissey

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 fbissey

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 strogdon

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 strogdon

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])



and perhaps some comments. This also works with version 1.5.3

comment:28 Changed 5 years ago by fbissey

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 fbissey

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:30 Changed 5 years ago by fbissey

Yes, with a few more comments I think it can go that way.

comment:31 Changed 5 years ago by fbissey

Authors: → François Bissey, Steven Trogdon → u/fbissey/MPL-2.1.0 → 6a0510f35a19f907a719edb832c80dc8ecf6ffea modified (diff) new → needs_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 dimpase

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 fbissey

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 fbissey

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 dimpase

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... Last edited 5 years ago by dimpase (previous) (diff) comment:36 Changed 5 years ago by fbissey 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 dimpase to complete building Sage I also needed to do ./sage --pip install backports.functools_lru_cache  comment:38 Changed 5 years ago by fbissey 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 fbissey 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 git 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 fbissey Description: modified (diff) Apart from the issues of qhull this should be all for the dependencies. comment:42 Changed 5 years ago by strogdon 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 strogdon 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 fbissey 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 fbissey 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 git 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 git 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 fbissey Description: modified (diff) 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 fbissey Description: modified (diff) comment:50 Changed 5 years ago by strogdon 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 fbissey 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 strogdon 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 fbissey 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 fbissey 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 strogdon 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 fbissey 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 in reply to: 55 Changed 5 years ago by fbissey Replying to strogdon: 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. Any idea has to what to use instead of set_linestyle? Or just skip it? comment:58 Changed 5 years ago by git Commit: 717c3dad0ffd8aed76ba9dd7968c90c7a745252f → 3508ea33211cc1eb1dff07f4fb4f2fb1fbe9c52a Branch pushed to git repo; I updated commit sha1. 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:59 Changed 5 years ago by fbissey 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 in reply to: 54 Changed 5 years ago by strogdon Replying to fbissey: In sage/plot/graphics.py one warning MatplotlibDeprecationWarning: 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:61 Changed 5 years ago by fbissey I need to do a new fresh build. comment:62 Changed 5 years ago by strogdon sage-on-gentoo with matplotlib-2.1.0 does give the MatplotlibDeprecationWarning but not the UserWarning. So there are some odd differences. comment:63 Changed 5 years ago by strogdon I see matplotlib is in CDEPEND, so maybe that's it. comment:64 Changed 5 years ago by fbissey 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 git Commit: 3508ea33211cc1eb1dff07f4fb4f2fb1fbe9c52a → e4d80390bfd6dc56e102623aa055bc64b8b1c983 Branch pushed to git repo; I updated commit sha1. New commits:  ​2902b62 WIP: replace deprecated set_linestyle ​e4d8039 better (correct) fix for userwarnings in plot.py comment:66 Changed 5 years ago by fbissey 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 git Commit: e4d80390bfd6dc56e102623aa055bc64b8b1c983 → 7c028ad5ffaa40fa5333b40d43b0d3d0f547dd82 Branch pushed to git repo; I updated commit sha1. New commits:  ​7c028ad easy fix comment:68 Changed 5 years ago by fbissey 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 git 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 in reply to: 69 Changed 5 years ago by strogdon 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 fbissey 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 strogdon 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 fbissey 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 strogdon 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 fbissey Humpf, use label here, label doesn't exist there, can't they make up their mind. comment:76 Changed 5 years ago by git 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 egourgoulhon Cc: egourgoulhon added comment:78 in reply to: 75 Changed 5 years ago by strogdon Replying to fbissey: Humpf, use label here, label doesn'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 fbissey 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 strogdon 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 strogdon 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 fbissey 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 fbissey OK now I remember the problem is not the triangulation, it is the "natural neighbor" interpolation going away. Solutions to that: • get delaunay back (-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 strogdon Perhaps scipy here if one can figure out how to use it. comment:85 in reply to: 84 Changed 5 years ago by fbissey Replying to strogdon: Perhaps scipy here if one can figure out how to use it. It wasn't good enough in #17618, however there is one new option that may improve things. I will have to dig the old code of #17618 to do some experiments. comment:86 Changed 5 years ago by fbissey 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 strogdon The documentation even confuses the terms nearest neighbor and natural neighbor. comment:88 in reply to: 87 ; follow-ups: 89 92 Changed 5 years ago by fbissey Replying to strogdon: The documentation even confuses the terms nearest neighbor and natural 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 in reply to: 88 Changed 5 years ago by dimpase Replying to fbissey: Replying to strogdon: The documentation even confuses the terms nearest neighbor and natural neighbor. In scipy, matplotlib or sage? I thought I had done a good effort during the MPL 1.4.x upgrade in sage. 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 fbissey 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:91 Changed 5 years ago by dimpase There is VoronoiDiagram in Sage, is it enough? comment:92 in reply to: 88 Changed 5 years ago by strogdon Replying to fbissey: Replying to strogdon: The documentation even confuses the terms nearest neighbor and natural 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 fbissey @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 fbissey 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. Last edited 5 years ago by fbissey (previous) (diff) comment:95 Changed 5 years ago by fbissey 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 fbissey Attachment: MPL153-linear.png​ added Changed 5 years ago by fbissey Attachment: MPL153-nn.png​ added Changed 5 years ago by fbissey Attachment: MPL210-clough.png​ added Changed 5 years ago by fbissey Attachment: MPL210-linear.png​ added comment:96 Changed 5 years ago by fbissey 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 strogdon Attachment: MPL-2.1.0.png​ added Changed 5 years ago by strogdon Attachment: MPL-1.5.3.png​ added comment:97 Changed 5 years ago by strogdon 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 fbissey 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 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 def list_plot3d_tuples(v, interpolation_type, texture, **kwds): #arbitrary choice - assuming more or less a nxn grid of points # x should have n^2 entries. We sample 4 times that many points. if interpolation_type == 'linear': points=[[x[i],y[i]] for i in range(len(x))] j = numpy.complex(0, 1) if interpolation_type == 'linear' or interpolation_type =='default': T = tri.Triangulation(x, y) f = tri.LinearTriInterpolator(T, z) j = numpy.complex(0, 1) def list_plot3d_tuples(v, interpolation_type, texture, **kwds): G._set_extra_kwds(kwds) return G if interpolation_type == 'nn' or interpolation_type =='default': T=delaunay.Triangulation(x,y) f=T.nn_interpolator(z) f.default_value=0.0 j=numpy.complex(0,1) vals=f[ymin:ymax:j*num_points,xmin:xmax:j*num_points] if interpolation_type == 'clough': f = interpolate.CloughTocher2DInterpolator(points,z) 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)] return (x,y,z) def g(x, y): 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) return G comment:99 Changed 5 years ago by fbissey 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 strogdon OK, I hadn't noticed the interpolate. business. Yet another twist. comment:101 Changed 5 years ago by strogdon 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 fbissey 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 fbissey @ 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 strogdon @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 fbissey 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 git 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 git 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 fbissey 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 fbissey Milestone: sage-8.1 → sage-8.2 comment:110 follow-up: 114 Changed 5 years ago by egourgoulhon 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 egourgoulhon Attachment: plot-11.png​ added Changed 5 years ago by egourgoulhon Attachment: latex_display_original.png​ added Changed 5 years ago by egourgoulhon Attachment: latex_display_MPL-2.1.0.png​ added comment:111 Changed 5 years ago by egourgoulhon 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. Last edited 5 years ago by egourgoulhon (previous) (diff) comment:112 follow-up: 116 Changed 5 years ago by 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 \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 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)  I think the default mathtext font is dejavusans. comment:114 in reply to: 110 ; follow-up: 118 Changed 5 years ago by strogdon 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 strogdon @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 config = SafeConfigParser() config.add_section('directories') config.set('directories', 'basedirlist', os.environ['SAGE_LOCAL']) config.add_section('rc_options') config.set('rc_options', 'mathtext.fontsel', 'cm') ##################################################################### comment:116 in reply to: 112 Changed 5 years ago by egourgoulhon 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 \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? 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 in reply to: 113 Changed 5 years ago by egourgoulhon 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 in reply to: 114 Changed 5 years ago by egourgoulhon 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: 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. 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 egourgoulhon 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 fbissey 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 git 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:122 follow-up: 123 Changed 5 years ago by fbissey ok can you check again now? comment:123 in reply to: 122 ; follow-ups: 124 128 Changed 5 years ago by 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. 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 in reply to: 123 ; follow-up: 125 Changed 5 years ago by 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_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 in reply to: 124 ; follow-up: 126 Changed 5 years ago by fbissey 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_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 * """  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 in reply to: 125 ; follow-up: 127 Changed 5 years ago by egourgoulhon 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. Last edited 5 years ago by egourgoulhon (previous) (diff) comment:127 in reply to: 126 Changed 5 years ago by egourgoulhon Replying to egourgoulhon: The sphinx_plot code 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 in reply to: 123 Changed 5 years ago by strogdon 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 strogdon 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 fbissey 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 fbissey 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 charpent 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 in reply to: 132 ; follow-up: 141 Changed 5 years ago by 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). comment:134 follow-up: 138 Changed 5 years ago by fbissey 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 fbissey 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 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. comment:137 Changed 5 years ago by git 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 in reply to: 134 ; follow-up: 139 Changed 5 years ago by egourgoulhon Replying to fbissey: So upstream really thinks DejaVu is 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 in reply to: 138 ; follow-up: 140 Changed 5 years ago by fbissey Replying to egourgoulhon: Replying to fbissey: So upstream really thinks DejaVu is 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. 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 in reply to: 139 Changed 5 years ago by egourgoulhon 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 in reply to: 133 Changed 5 years ago by charpent 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 in reply to: 136 ; follow-ups: 143 147 Changed 5 years ago by 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 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 in reply to: 142 Changed 5 years ago by strogdon 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 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. 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 fbissey 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 in reply to: 144 Changed 5 years ago by strogdon Replying to fbissey: 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. 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 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? 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 fbissey 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 in reply to: 142 ; follow-up: 148 Changed 5 years ago by dimpase 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 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. 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 in reply to: 147 Changed 5 years ago by egourgoulhon 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 strogdon 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 fbissey 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 git Commit: 29a35c2fb62d0bc8ec98bb736b46d6905ecbff6e → 8f4ca6bfe57c467b5a13594f6699782068cf86c2 Branch pushed to git repo; I updated commit sha1. New commits:  ​91749f5 Merge branch 'develop' into matplotlib-2.1.0 ​8f4ca6b convert all instances of interpolation='nn' to 'clough' comment:152 Changed 5 years ago by fbissey 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 strogdon This is the pull request that was merged which breaks the arc.py doctest. comment:154 Changed 5 years ago by fbissey 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 strogdon 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 fbissey 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 fbissey 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 strogdon Attachment: bezier+arc.png​ added comment:158 Changed 5 years ago by strogdon 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 class Arc(GraphicPrimitive): """ from sage.plot.bezier_path import BezierPath from sage.plot.graphics import Graphics from matplotlib.path import Path import numpy as np ma = self._matplotlib_arc() transform = ma.get_transform().get_matrix() def theta_stretch(theta, scale): theta = np.deg2rad(theta) x = np.cos(theta) y = np.sin(theta) return np.rad2deg(np.arctan2(scale * y, x)) theta1 = theta_stretch(ma.theta1, ma.width / ma.height) theta2 = theta_stretch(ma.theta2, ma.width / ma.height) pa = ma pa._path = Path.arc(theta1, theta2) transform = pa.get_transform().get_matrix() cA, cC, cE = transform[0] cB, cD, cF = transform[1] points = [] for u in ma._path.vertices: for u in pa._path.vertices: x, y = list(u) points += [(cA * x + cC * y + cE, cB * x + cD * y + cF)] 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 fbissey Could you show the original, so it is clear that the graph was incorrect? comment:160 in reply to: 159 Changed 5 years ago by strogdon 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 strogdon 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 strogdon Attachment: arc.png​ added Changed 5 years ago by strogdon Attachment: bezier.png​ added comment:162 Changed 5 years ago by strogdon 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 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. comment:164 in reply to: 163 Changed 5 years ago by strogdon 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 strogdon Attachment: new_bezier+arc.png​ added comment:165 Changed 5 years ago by strogdon 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 fbissey 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 git Commit: 8f4ca6bfe57c467b5a13594f6699782068cf86c2 → 0db703433170a8a266151229194693242499da89 Branch pushed to git repo; I updated commit sha1. New commits:  ​07b732c Merge branch 'develop' into matplotlib-2.1.0 ​0db7034 Add Steve's corrections to arc.py comment:168 Changed 5 years ago by strogdon 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 fbissey 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 in reply to: 169 Changed 5 years ago by egourgoulhon Replying to fbissey: 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. 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 egourgoulhon 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 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. comment:173 in reply to: 172 Changed 5 years ago by egourgoulhon 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 git Commit: 0db703433170a8a266151229194693242499da89 → 41c21c6ac7a6fd7f2595129b33421ce5fb2c7521 Branch pushed to git repo; I updated commit sha1. New commits:  ​ca9f8e2 make the classic style (from 1.5.3) the default style for now. ​41c21c6 Add a comment on enforcing the classics style in sphinx_plot for sage-on-distro comment:175 follow-up: 176 Changed 5 years ago by fbissey 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 in reply to: 175 Changed 5 years ago by egourgoulhon 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 egourgoulhon Reviewers: → Eric Gourgoulhon, Dima Pasechnik 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 vbraun 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 fbissey 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 fbissey 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 fbissey 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 in reply to: 181 Changed 5 years ago by fbissey Replying to fbissey: It looks like there are some changes to arrow.py that 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 fbissey 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 strogdon 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 fbissey 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 in reply to: 185 Changed 5 years ago by egourgoulhon 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 egourgoulhon 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 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. Next we'll to work out what's happening with arrow.py. comment:189 in reply to: 188 ; follow-up: 192 Changed 5 years ago by 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_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 dimpase has anyone looked into using GR backend of Mathplotlib in Sage: https://gr-framework.org/tutorials/matplotlib.html ? comment:191 in reply to: 190 ; follow-up: 193 Changed 5 years ago by fbissey 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 in reply to: 189 Changed 5 years ago by fbissey 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_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. Yes, probably less ham-fisted. Although I'll check the interpolation to match the classic style. comment:193 in reply to: 191 ; follow-up: 195 Changed 5 years ago by dimpase Replying to fbissey: 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. 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 strogdon 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 in reply to: 193 Changed 5 years ago by fbissey Replying to dimpase: Replying to fbissey: 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. 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 fbissey Branch: u/fbissey/MPL-2.1.0 → u/fbissey/MPL-2.1.0b 41c21c6ac7a6fd7f2595129b33421ce5fb2c7521 → 9b90efadd3a8aa3dfc2ecd77c3f8e538f20cf424 → 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 strogdon 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 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(dashes={'dash_offset': 0, 'dash_list': None})]) pe2 = ConditionalStroke(CheckNthSubPath(p, 1), [pe.Stroke()]) p.set_path_effects([pe1, pe2]) subplot.add_patch(p) There may be a better way to do this. comment:198 Changed 5 years ago by git 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 git 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 fbissey Status: needs_work → needs_review fix arrow.py and move qhull to cmake Bonus, building with optional qhull spkg now works. So one more time in review! comment:201 in reply to: 200 Changed 5 years ago by egourgoulhon 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 def sphinx_plot(plot): from sage.misc.temporary_file import tmp_filename import matplotlib.pyplot as plt import matplotlib as mpl mpl.rcParams['image.interpolation'] = 'bilinear' mpl.rcParams['image.resample'] = False if os.environ.get('SAGE_SKIP_PLOT_DIRECTIVE', 'no') != 'yes': mpl.rcParams['image.interpolation'] = 'bilinear' mpl.rcParams['image.resample'] = False mpl.rcParams['figure.figsize'] = [8.0, 6.0] mpl.rcParams['figure.dpi'] = 80 mpl.rcParams['savefig.dpi'] = 100 fn = tmp_filename(ext=".png") plot.plot().save(fn) 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). Last edited 5 years ago by egourgoulhon (previous) (diff) comment:202 follow-up: 203 Changed 5 years ago by fbissey 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 in reply to: 202 Changed 5 years ago by egourgoulhon 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". Last edited 5 years ago by egourgoulhon (previous) (diff) comment:204 Changed 5 years ago by git 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 egourgoulhon 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 in reply to: 200 ; follow-up: 208 Changed 5 years ago by 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_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 in reply to: 205 ; follow-up: 209 Changed 5 years ago by dimpase Replying to egourgoulhon: 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? Do we want an OSX test? comment:208 in reply to: 206 Changed 5 years ago by fbissey 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_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. Sure, but I think we'll only get traction on that when cmake is standard. But I thought strongly about it too. 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  I should have thought of that. I'll just tweak it. comment:209 in reply to: 207 Changed 5 years ago by fbissey Replying to dimpase: Replying to egourgoulhon: 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? 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 git Commit: 3a6792ea300002114fc701acfb14ea52f72fe040 → 929509edcfe5278337087f280222fc9b7bb10bbd 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 dimpase 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. Last edited 5 years ago by dimpase (previous) (diff) comment:212 follow-up: 214 Changed 5 years ago by 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? 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 fbissey Is SAGE_MATPLOTLIB_GUI set to no when you build headless? If not you should. comment:214 in reply to: 212 Changed 5 years ago by dimpase 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 DISPLAY recently. 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. Last edited 5 years ago by dimpase (previous) (diff) comment:215 Changed 5 years ago by fbissey 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 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? comment:217 in reply to: 216 Changed 5 years ago by fbissey 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 dimpase 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 fbissey 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 in reply to: 219 Changed 5 years ago by strogdon Replying to fbissey: 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. 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 fbissey 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 strogdon 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 fbissey 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 strogdon 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 fbissey 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. Last edited 5 years ago by fbissey (previous) (diff) comment:226 Changed 5 years ago by strogdon 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 fbissey 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 in reply to: 227 Changed 5 years ago by strogdon Replying to fbissey: 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. 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 dimpase 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 fbissey +1 to that. Do you have a clue where that should be set. comment:231 in reply to: 230 Changed 5 years ago by dimpase 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 git 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 fbissey 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 strogdon 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 strogdon 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 fbissey 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 strogdon 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 git 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 fbissey 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 dimpase 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 dimpase 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.

Last edited 5 years ago by dimpase (previous) (diff)

comment:242 Changed 5 years ago by fbissey

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 dimpase

one way or another, "real" Sage library code should not be polluted by checks of whether is is being doctested.

comment:244 in reply to:  243 Changed 5 years ago by fbissey

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 git

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

 ​d7db6e1 Merge branch 'develop' into MPL-2.1.0b ​e0534a8 Make all doctests set agg as the backend for matplotlib.

comment:246 Changed 5 years ago by dimpase

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 fbissey

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 in reply to:  247 ; follow-up:  249 Changed 5 years ago by strogdon

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 in reply to:  248 Changed 5 years ago by dimpase

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

Last edited 5 years ago by dimpase (previous) (diff)

comment:250 follow-up:  251 Changed 5 years ago by 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.

comment:251 in reply to:  250 Changed 5 years ago by dimpase

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 git

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:253 Changed 5 years ago by fbissey

And I hope it is the last of it.

comment:254 follow-up:  256 Changed 5 years ago by jhpalmieri

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 jhpalmieri

(I think the current test works, so we can leave it as is, and consider changes on another ticket.)

comment:256 in reply to:  254 ; follow-up:  258 Changed 5 years ago by fbissey

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.

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 fbissey

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 in reply to:  256 Changed 5 years ago by dimpase

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.

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?

This is correct, and in fact starting anything short of full Sage is Dieselgate approach to testing...

comment:259 Changed 5 years ago by git

Commit: d5be931f48b77bb03d2d5750bea3805435e795fd → ba7f0ab8dbde03cdd935233fc4228451b2cb0c87

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

 ​a5edbfe Change the startup test for numpy and pyparsing now that they are imported in the doctesting framework via matplotlib. ​ba7f0ab Merge branch 'develop' into MPL-2.1.0b

comment:260 follow-up:  261 Changed 5 years ago by fbissey

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 in reply to:  260 Changed 5 years ago by egourgoulhon

OK changed to sage -c by 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 dimpase

Status: needs_review → positive_review

OK, looks good on OSX too.

comment:263 Changed 5 years ago by egourgoulhon

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>
**********************************************************************
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 egourgoulhon

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.

Last edited 5 years ago by egourgoulhon (previous) (diff)

comment:265 follow-up:  266 Changed 5 years ago by 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?

Last edited 5 years ago by jhpalmieri (previous) (diff)

comment:266 in reply to:  265 Changed 5 years ago by egourgoulhon

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:267 Changed 5 years ago by fbissey

Sorry I didn't pay enough attention. I'll redo this ASAP.

comment:268 Changed 5 years ago by git

Commit: ba7f0ab8dbde03cdd935233fc4228451b2cb0c87 → ca3b242e6d7ca2ce22d8d23ef62d0cecd95b585f

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

 ​f65ac29 Merge with develop ​ca3b242 correct test in startup.py

comment:269 Changed 5 years ago by fbissey

Status: needs_work → needs_review

OK, it is corrected.

comment:270 Changed 5 years ago by dimpase

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 egourgoulhon

It's also OK from my side: make ptestlong ==> "All tests passed!"

Many thanks!

comment:272 Changed 5 years ago by vbraun

Branch: u/fbissey/MPL-2.1.0b → ca3b242e6d7ca2ce22d8d23ef62d0cecd95b585f → fixed positive_review → closed

comment:273 Changed 5 years ago by jdemeyer

Commit: ca3b242e6d7ca2ce22d8d23ef62d0cecd95b585f

See #24670 for a follow-up.

comment:274 Changed 5 years ago by jdemeyer

Serious breakage at #24671.

comment:275 Changed 5 years ago by jdemeyer

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:


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

That warrants a follow up ticket to deal with this.

comment:277 in reply to:  276 Changed 5 years ago by jdemeyer

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 jdemeyer

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 fbissey

OK, let's move on to do something about it. Is there any other plots that are taking an awful long time?

comment:280 Changed 5 years ago by jdemeyer

Follow-up at #24862.

comment:281 Changed 5 years ago by jdemeyer

Welcome to the past: we just got hit by #13135 again.

comment:282 follow-up:  283 Changed 5 years ago by gh-dimpase

By the way, matplotlib 2.2.0 has been released.

comment:283 in reply to:  282 Changed 5 years ago by fbissey

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.

comment:284 Changed 5 years ago by dimpase

I guess major should be vers. 3

comment:285 Changed 5 years ago by mmezzarobba

Probably caused by commit d109b7593ef` from this ticket: #25165.

Note: See TracTickets for help on using tickets.