Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#23696 closed enhancement (fixed)

Update matplotlib to 2.1.0

Reported by: dimpase Owned by:
Priority: major Milestone: sage-8.2
Component: packages: standard Keywords:
Cc: fbissey, strogdon, egourgoulhon, charpent, chapoton Merged in:
Authors: François Bissey, Steven Trogdon Reviewers: Eric Gourgoulhon, Dima Pasechnik
Report Upstream: None of the above - read trac for reasoning. Work issues:
Branch: ca3b242 (Commits) Commit:
Dependencies: Stopgaps:

Attachments (13)

MPL153-linear.png (102.6 KB) - added by fbissey 3 years ago.
MPL153-nn.png (113.2 KB) - added by fbissey 3 years ago.
MPL210-clough.png (108.2 KB) - added by fbissey 3 years ago.
MPL210-linear.png (103.8 KB) - added by fbissey 3 years ago.
MPL-2.1.0.png (90.6 KB) - added by strogdon 3 years ago.
MPL-1.5.3.png (81.9 KB) - added by strogdon 3 years ago.
plot-11.png (13.7 KB) - added by egourgoulhon 3 years ago.
latex_display_original.png (10.7 KB) - added by egourgoulhon 3 years ago.
latex_display_MPL-2.1.0.png (9.6 KB) - added by egourgoulhon 3 years ago.
bezier+arc.png (12.3 KB) - added by strogdon 3 years ago.
arc.png (10.8 KB) - added by strogdon 3 years ago.
bezier.png (14.7 KB) - added by strogdon 3 years ago.
new_bezier+arc.png (15.6 KB) - added by strogdon 3 years ago.

Download all attachments as: .zip

Change History (298)

comment:1 Changed 3 years ago by fbissey

  • Cc fbissey added

comment:2 Changed 3 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
        task(self.options, self.outtmpfile, msgpipe, self.result_queue)
      File "/usr/lib64/python2.7/site-packages/sage/doctest/forker.py", line 2218, in __call__
        result = runner.run(test)
      File "/usr/lib64/python2.7/site-packages/sage/doctest/forker.py", line 663, in run
        return self._run(test, compileflags, out)
      File "/usr/lib64/python2.7/site-packages/sage/doctest/forker.py", line 518, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/usr/lib64/python2.7/site-packages/sage/doctest/forker.py", line 888, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.probability.probability_distribution.ProbabilityDistribution.generate_histogram_data[3]>", line 1, in <module>
        h, b = X.generate_histogram_data(bins = Integer(10))
      File "/usr/lib64/python2.7/site-packages/matplotlib/pyplot.py", line 3075, in hist
        mplDeprecation)
    :
    MatplotlibDeprecationWarning: The 'hold' keyword argument is deprecated since 2.0.
**********************************************************************

which is solved by #23696.

comment:3 Changed 3 years ago by dimpase

  • Dependencies set to #23689

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

comment:4 Changed 3 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 3 years ago by jdemeyer

  • Dependencies changed from #23689 to #23689, #23711

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

comment:6 Changed 3 years ago by strogdon

  • Cc strogdon added

comment:7 Changed 3 years ago by jdemeyer

  • Dependencies #23689, #23711 deleted

comment:8 Changed 3 years ago by jdemeyer

  • Summary changed from update matplotlib to 2.0.2 to Update matplotlib to 2.1.0

comment:9 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:10 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:11 follow-up: Changed 3 years ago by fbissey

Wondering about the test about arrow. From the file

        Dashed arrows should have solid arrowheads,
        :trac:`12852`. This test saves the plot of a dashed arrow to
        an EPS file. Within the EPS file, ``stroke`` will be called
        twice: once to draw the line, and again to draw the
        arrowhead. We check that both calls do not occur while the
        dashed line style is enabled::
            sage: a = arrow((0,0), (1,1), linestyle='dashed')
            sage: filename = tmp_filename(ext='.eps')
            sage: a.save(filename=filename)
            sage: with open(filename, 'r') as f:
            ....:     contents = f.read().replace('\n', ' ')
            sage: two_stroke_pattern = r'setdash.*stroke.*stroke.*setdash'
            sage: import re
            sage: two_stroke_re = re.compile(two_stroke_pattern)
            sage: two_stroke_re.search(contents) is None
            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 3 years ago by strogdon

Replying to fbissey:

Wondering about the test about arrow. From the file

        Dashed arrows should have solid arrowheads,
        :trac:`12852`. This test saves the plot of a dashed arrow to
        an EPS file. Within the EPS file, ``stroke`` will be called
        twice: once to draw the line, and again to draw the
        arrowhead. We check that both calls do not occur while the
        dashed line style is enabled::
            sage: a = arrow((0,0), (1,1), linestyle='dashed')
            sage: filename = tmp_filename(ext='.eps')
            sage: a.save(filename=filename)
            sage: with open(filename, 'r') as f:
            ....:     contents = f.read().replace('\n', ' ')
            sage: two_stroke_pattern = r'setdash.*stroke.*stroke.*setdash'
            sage: import re
            sage: two_stroke_re = re.compile(two_stroke_pattern)
            sage: two_stroke_re.search(contents) is None
            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 3 years ago by strogdon (previous) (diff)

comment:13 Changed 3 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 3 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 3 years ago by strogdon

  • Report Upstream changed from N/A to Reported upstream. No feedback yet.

The arrow issue has been reported upstream. See https://github.com/matplotlib/matplotlib/issues/9579

comment:16 Changed 3 years ago by strogdon

  • Report Upstream changed from Reported upstream. No feedback yet. to None of the above - read trac for reasoning.

Apparently a not so obvious API change. The following should work for >= 1.5.3. The .eps looks (needs to be confirmed!) OK.

--- a/src/sage/plot/arrow.py
+++ b/src/sage/plot/arrow.py
@@ -437,7 +437,7 @@ class Arrow(GraphicPrimitive):
                             pe1.draw_path(renderer, gc, tpath, affine, rgbFace)
 
             pe1 = ConditionalStroke(CheckNthSubPath(p, 0), [pe.Stroke()])
-            pe2 = ConditionalStroke(CheckNthSubPath(p, 1), [pe.Stroke(linestyle="solid")])
+            pe2 = ConditionalStroke(CheckNthSubPath(p, 1), [pe.Stroke(dashes={'dash_offset': 0, 'dash_list': None})])
             p.set_path_effects([pe1, pe2])
 
         subplot.add_patch(p)

The doctest ./sage -t --long src/sage/plot/arrow.py does pass on vanilla Sage with the above changes with matplotlib-1.5.3. However on sage-on-gentoo with matplotlib-2.0.2 the generated .eps looks OK but the test still fails?

sage -t --long --warn-long 119.2 /usr/lib/python2.7/site-packages/sage/plot/arrow.py
**********************************************************************
File "/usr/lib/python2.7/site-packages/sage/plot/arrow.py", line 364, in sage.plot.arrow.Arrow._render_on_subplot
Failed example:
    two_stroke_re.search(contents) is None
Expected:
    True
Got:
    False

comment:17 follow-up: Changed 3 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 3 years ago by strogdon

Replying to fbissey:

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

Basically the same

sage: two_stroke_re.search(contents)
<_sre.SRE_Match object at 0x7fabe8b8e4a8>

comment:19 Changed 3 years ago by fbissey

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

comment:20 Changed 3 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 3 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 3 years ago by strogdon (previous) (diff)

comment:22 Changed 3 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 3 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])
 
         subplot.add_patch(p)

So I've

1) removed the axes

2) make sure we match setdash.*stroke.*stroke.*setdash

We want to match the above since when successful the .eps looks something like

[7.4 3.2] 0 setdash
0.000 0.000 1.000 setrgbcolor
gsave
434.2 319 10.7 10.7 clipbox
19.049231 16.833846 m
158.203077 119.064615 296.756245 220.85409 434.708734 322.20227 c
stroke
grestore
[] 0 setdash
gsave
434.2 319 10.7 10.7 clipbox
423.689492 320.311157 m
434.708734 322.20227 l
429.610079 312.252209 l
423.689492 320.311157 l
cl
gsave
fill
grestore
stroke

The [] 0 setdash is the bit that draws the solid arrowhead. If that line is removed from the .eps file then the arrowhead is drawn with the dashed linetype. Of course, the description will have to be changed.

comment:24 Changed 3 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 3 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 3 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 3 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])
 
         subplot.add_patch(p)

and perhaps some comments. This also works with version 1.5.3

comment:28 Changed 3 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 3 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 3 years ago by fbissey

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

comment:31 Changed 3 years ago by fbissey

  • Authors set to François Bissey, Steven Trogdon
  • Branch set to u/fbissey/MPL-2.1.0
  • Commit set to 6a0510f35a19f907a719edb832c80dc8ecf6ffea
  • Description modified (diff)
  • Status changed from new to needs_review

All right, it is time to move ahead. Please review.


New commits:

c35f93fupdate matplotlib spkg. Use github in spkg-src instead of pypy (url more stable).
d8630f8matplotlib doesn't like integer in some context, and after 2.0, doesn't seem to cast them.
ecf0cb7fix arrow.py. 1) slight change of interface 2) in a doctest, improve the selection done by regular expression.
6a0510fRemove warning suppression needed with MPL-1.5/numpy-1.13

comment:32 Changed 3 years ago by dimpase

  • Status changed from needs_review to 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 3 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 3 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 3 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 3 years ago by dimpase (previous) (diff)

comment:36 Changed 3 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 3 years ago by dimpase

to complete building Sage I also needed to do

./sage --pip install backports.functools_lru_cache

comment:38 Changed 3 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 3 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 3 years ago by git

  • Commit changed from 6a0510f35a19f907a719edb832c80dc8ecf6ffea to 496ded5808d15c7d1830ca9662082d448a994215

Branch pushed to git repo; I updated commit sha1. New commits:

496ded5add backports_functools_lru_cache a new dependency of MPL. Fix MPL 2.1.0 dependencies.

comment:41 Changed 3 years ago by fbissey

  • Description modified (diff)

Apart from the issues of qhull this should be all for the dependencies.

comment:42 Changed 3 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 3 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 3 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 3 years ago by fbissey

  • Status changed from needs_info to 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 3 years ago by git

  • Commit changed from 496ded5808d15c7d1830ca9662082d448a994215 to f6a9e539143095b1cd6a0e0ba762b850339d2746

Branch pushed to git repo; I updated commit sha1. New commits:

f6a9e53update setuptools_scm for matplotlib-2.1.0

comment:47 Changed 3 years ago by git

  • Commit changed from f6a9e539143095b1cd6a0e0ba762b850339d2746 to 717c3dad0ffd8aed76ba9dd7968c90c7a745252f

Branch pushed to git repo; I updated commit sha1. New commits:

717c3daAdd subprocess32 as a dependency of MPL-2.1.0

comment:48 Changed 3 years ago by fbissey

  • Description modified (diff)
  • Status changed from needs_work to needs_review

There! Apart from the qhull issue that we should tackle separately, this is ready I believe.

comment:49 Changed 3 years ago by fbissey

  • Description modified (diff)

comment:50 Changed 3 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 3 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 3 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 3 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: Changed 3 years ago by fbissey

  • Status changed from needs_review to 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: Changed 3 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 3 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 3 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 3 years ago by git

  • Commit changed from 717c3dad0ffd8aed76ba9dd7968c90c7a745252f to 3508ea33211cc1eb1dff07f4fb4f2fb1fbe9c52a

Branch pushed to git repo; I updated commit sha1. New commits:

2f4fed4Merge branch 'develop' into matplotlib-2.1.0
3508ea3Add new user warnings in the expected output of a couple of tests

comment:59 Changed 3 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:

2f4fed4Merge branch 'develop' into matplotlib-2.1.0
3508ea3Add new user warnings in the expected output of a couple of tests

comment:60 in reply to: ↑ 54 Changed 3 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 3 years ago by fbissey

I need to do a new fresh build.

comment:62 Changed 3 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 3 years ago by strogdon

I see matplotlib is in CDEPEND, so maybe that's it.

comment:64 Changed 3 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 3 years ago by git

  • Commit changed from 3508ea33211cc1eb1dff07f4fb4f2fb1fbe9c52a to e4d80390bfd6dc56e102623aa055bc64b8b1c983

Branch pushed to git repo; I updated commit sha1. New commits:

2902b62WIP: replace deprecated set_linestyle
e4d8039better (correct) fix for userwarnings in plot.py

comment:66 Changed 3 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 3 years ago by git

  • Commit changed from e4d80390bfd6dc56e102623aa055bc64b8b1c983 to 7c028ad5ffaa40fa5333b40d43b0d3d0f547dd82

Branch pushed to git repo; I updated commit sha1. New commits:

7c028adeasy fix

comment:68 Changed 3 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:

7c028adeasy fix

comment:69 follow-up: Changed 3 years ago by git

  • Commit changed from 7c028ad5ffaa40fa5333b40d43b0d3d0f547dd82 to 5d27a242c7dfb8e8d7a9f6f030d07596e42ba5d4

Branch pushed to git repo; I updated commit sha1. New commits:

5d27a24contour 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 3 years ago by strogdon

Replying to git:

Branch pushed to git repo; I updated commit sha1. New commits:

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

Humpf, use label here, label doesn't exist there, can't they make up their mind.

comment:76 Changed 3 years ago by git

  • Commit changed from 5d27a242c7dfb8e8d7a9f6f030d07596e42ba5d4 to 87b33cf3f2cf959e161c857decc2be47ab824437

Branch pushed to git repo; I updated commit sha1. New commits:

87b33cfsupress last warning in arrow.py by adding a label in the right place

comment:77 Changed 3 years ago by egourgoulhon

  • Cc egourgoulhon added

comment:78 in reply to: ↑ 75 Changed 3 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 3 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 3 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 3 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 3 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 3 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: Changed 3 years ago by strogdon

Perhaps scipy here if one can figure out how to use it.

comment:85 in reply to: ↑ 84 Changed 3 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 3 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: Changed 3 years ago by strogdon

The documentation even confuses the terms nearest neighbor and natural neighbor.

comment:88 in reply to: ↑ 87 ; follow-ups: Changed 3 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 3 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.

Is it this one https://en.wikipedia.org/wiki/Natural_neighbor_interpolation ?

Surely Sage has more than one implementation of Voronoi/Delaunay stuff... Could you explain what exactly the issue is here?

comment:90 Changed 3 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 3 years ago by dimpase

There is VoronoiDiagram in Sage, is it enough?

comment:92 in reply to: ↑ 88 Changed 3 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 3 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 3 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 3 years ago by fbissey (previous) (diff)

comment:95 Changed 3 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 3 years ago by fbissey

Changed 3 years ago by fbissey

Changed 3 years ago by fbissey

Changed 3 years ago by fbissey

comment:96 Changed 3 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 3 years ago by strogdon

Changed 3 years ago by strogdon

comment:97 Changed 3 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 3 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 b def list_plot3d_tuples(v, interpolation_type, texture, **kwds): 
    373373        sage: list_plot3d([(1, 2, 3), (0, 1, 3), (2, 1, 4), (1, 0, -2)], texture='yellow', num_points=50)
    374374        Graphics3d Object
    375375    """
    376     from matplotlib import tri, delaunay
     376    from matplotlib import tri
    377377    import numpy
    378378    import scipy
    379379    from random import random
    def list_plot3d_tuples(v, interpolation_type, texture, **kwds): 
    427427                                          #arbitrary choice - assuming more or less a nxn grid of points
    428428                                          # x should have n^2 entries. We sample 4 times that many points.
    429429
    430     if interpolation_type == 'linear':
     430    points=[[x[i],y[i]] for i in range(len(x))]
     431    j = numpy.complex(0, 1)
     432   
     433    if interpolation_type == 'linear' or interpolation_type =='default':
    431434        T = tri.Triangulation(x, y)
    432435        f = tri.LinearTriInterpolator(T, z)
    433436        j = numpy.complex(0, 1)
    def list_plot3d_tuples(v, interpolation_type, texture, **kwds): 
    439442        G._set_extra_kwds(kwds)
    440443        return G
    441444
    442     if interpolation_type == 'nn'  or interpolation_type =='default':
    443 
    444         T=delaunay.Triangulation(x,y)
    445         f=T.nn_interpolator(z)
    446         f.default_value=0.0
    447         j=numpy.complex(0,1)
    448         vals=f[ymin:ymax:j*num_points,xmin:xmax:j*num_points]
     445    if interpolation_type == 'clough':
     446        f = interpolate.CloughTocher2DInterpolator(points,z)
    449447        from .parametric_surface import ParametricSurface
    450         def g(x,y):
    451             i=round( (x-xmin)/(xmax-xmin)*(num_points-1) )
    452             j=round( (y-ymin)/(ymax-ymin)*(num_points-1) )
    453             z=vals[int(j),int(i)]
    454             return (x,y,z)
     448        def g(x, y):
     449            z = f([x, y])
     450            return (x, y, z)
    455451        G = ParametricSurface(g, (list(numpy.r_[xmin:xmax:num_points*j]), list(numpy.r_[ymin:ymax:num_points*j])), texture=texture, **kwds)
    456452        G._set_extra_kwds(kwds)
    457453        return G

comment:99 Changed 3 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 3 years ago by strogdon

OK, I hadn't noticed the interpolate. business. Yet another twist.

comment:101 Changed 3 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 3 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 3 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 3 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 3 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 3 years ago by git

  • Commit changed from 87b33cf3f2cf959e161c857decc2be47ab824437 to d29091bf4a36027761bf08da8b62d06ca94a8aab

Branch pushed to git repo; I updated commit sha1. New commits:

d29091bMerge branch 'develop' into matplotlib-2.1.0

comment:107 Changed 3 years ago by git

  • Commit changed from d29091bf4a36027761bf08da8b62d06ca94a8aab to 594df5633ec94dde7a15cc3a866cba9cfedfde9e

Branch pushed to git repo; I updated commit sha1. New commits:

594df56MPL'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 3 years ago by fbissey

  • Status changed from needs_work to 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 3 years ago by fbissey

  • Milestone changed from sage-8.1 to sage-8.2

comment:110 follow-up: Changed 3 years ago by egourgoulhon

  • Status changed from needs_review to 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 3 years ago by egourgoulhon

Changed 3 years ago by egourgoulhon

Changed 3 years ago by egourgoulhon

comment:111 Changed 3 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 3 years ago by egourgoulhon (previous) (diff)

comment:112 follow-up: Changed 3 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: Changed 3 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: Changed 3 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 3 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 b config = SafeConfigParser() 
    99config.add_section('directories')
    1010config.set('directories', 'basedirlist', os.environ['SAGE_LOCAL'])
    1111
     12config.add_section('rc_options')
     13config.set('rc_options', 'mathtext.fontsel', 'cm')
    1214
    1315
    1416#####################################################################

comment:116 in reply to: ↑ 112 Changed 3 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 3 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 3 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 3 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 3 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 3 years ago by git

  • Commit changed from 594df5633ec94dde7a15cc3a866cba9cfedfde9e to b81b49c8935bf50dd64267ce0931f45542a6ffa9

Branch pushed to git repo; I updated commit sha1. New commits:

b81b49cAdd Steve's suggestion to set the default fonts in matplotlib

comment:122 follow-up: Changed 3 years ago by fbissey

ok can you check again now?

comment:123 in reply to: ↑ 122 ; follow-ups: Changed 3 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: Changed 3 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: Changed 3 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: Changed 3 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 3 years ago by egourgoulhon (previous) (diff)

comment:127 in reply to: ↑ 126 Changed 3 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 3 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 3 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 3 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 3 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: Changed 3 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: Changed 3 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: Changed 3 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 3 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: Changed 3 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 3 years ago by git

  • Commit changed from b81b49c8935bf50dd64267ce0931f45542a6ffa9 to 29a35c2fb62d0bc8ec98bb736b46d6905ecbff6e

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

29a35c2use the classic MPL 1.5 style in sphinx_plot

comment:138 in reply to: ↑ 134 ; follow-up: Changed 3 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: Changed 3 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 3 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 3 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: Changed 3 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 3 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: Changed 3 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 3 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 3 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: Changed 3 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 3 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 3 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 3 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 3 years ago by git

  • Commit changed from 29a35c2fb62d0bc8ec98bb736b46d6905ecbff6e to 8f4ca6bfe57c467b5a13594f6699782068cf86c2

Branch pushed to git repo; I updated commit sha1. New commits:

91749f5Merge branch 'develop' into matplotlib-2.1.0
8f4ca6bconvert all instances of interpolation='nn' to 'clough'

comment:152 Changed 3 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 3 years ago by strogdon

This is the pull request that was merged which breaks the arc.py doctest.

comment:154 Changed 3 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 3 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 3 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 3 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 3 years ago by strogdon

comment:158 Changed 3 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 b class Arc(GraphicPrimitive): 
    301301        """
    302302        from sage.plot.bezier_path import BezierPath
    303303        from sage.plot.graphics import Graphics
     304        from matplotlib.path import Path
     305        import numpy as np
    304306        ma = self._matplotlib_arc()
    305         transform = ma.get_transform().get_matrix()
     307        def theta_stretch(theta, scale):
     308            theta = np.deg2rad(theta)
     309            x = np.cos(theta)
     310            y = np.sin(theta)
     311            return np.rad2deg(np.arctan2(scale * y, x))
     312        theta1 = theta_stretch(ma.theta1, ma.width / ma.height)
     313        theta2 = theta_stretch(ma.theta2, ma.width / ma.height)
     314
     315        pa = ma
     316        pa._path = Path.arc(theta1, theta2)
     317        transform = pa.get_transform().get_matrix()
    306318        cA, cC, cE = transform[0]
    307319        cB, cD, cF = transform[1]
    308320        points = []
    309         for u in ma._path.vertices:
     321        for u in pa._path.vertices:
    310322            x, y = list(u)
    311323            points += [(cA * x + cC * y + cE, cB * x + cD * y + cF)]
    312324        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: Changed 3 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 3 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 3 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 3 years ago by strogdon

Changed 3 years ago by strogdon

comment:162 Changed 3 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: Changed 3 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 3 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 3 years ago by strogdon

comment:165 Changed 3 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 3 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 3 years ago by git

  • Commit changed from 8f4ca6bfe57c467b5a13594f6699782068cf86c2 to 0db703433170a8a266151229194693242499da89

Branch pushed to git repo; I updated commit sha1. New commits:

07b732cMerge branch 'develop' into matplotlib-2.1.0
0db7034Add Steve's corrections to arc.py

comment:168 Changed 3 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: Changed 3 years ago by fbissey

  • Status changed from needs_work to 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 3 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 3 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: Changed 3 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 3 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 3 years ago by git

  • Commit changed from 0db703433170a8a266151229194693242499da89 to 41c21c6ac7a6fd7f2595129b33421ce5fb2c7521

Branch pushed to git repo; I updated commit sha1. New commits:

ca9f8e2make the classic style (from 1.5.3) the default style for now.
41c21c6Add a comment on enforcing the classics style in sphinx_plot for sage-on-distro

comment:175 follow-up: Changed 3 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 3 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 3 years ago by egourgoulhon

  • Reviewers set to Eric Gourgoulhon, Dima Pasechnik
  • Status changed from needs_review to 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 3 years ago by vbraun

  • Status changed from positive_review to 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 3 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 3 years ago by fbissey

OK something changed in one of the latest revisions. The matching pattern doesn't work anymore.

comment:181 follow-up: Changed 3 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 3 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 3 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 3 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: Changed 3 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 3 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 3 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: Changed 3 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: Changed 3 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: Changed 3 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: Changed 3 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 3 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: Changed 3 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 3 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 3 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 3 years ago by fbissey

  • Branch changed from u/fbissey/MPL-2.1.0 to u/fbissey/MPL-2.1.0b
  • Commit changed from 41c21c6ac7a6fd7f2595129b33421ce5fb2c7521 to 9b90efadd3a8aa3dfc2ecd77c3f8e538f20cf424
  • Work issues set to 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:

3c5d53cRebase MPL-2.1.0 upgrade branch
d109b75WIP on inserting matplotlib style sheet as a plot option and defaulting it to classic
9b90efaSet matplotlib options in sphinx_plot so that 3D plots look nice again.

comment:197 Changed 3 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 b class Arrow(GraphicPrimitive): 
    437437                            pe1.draw_path(renderer, gc, tpath, affine, rgbFace)
    438438
    439439            pe1 = ConditionalStroke(CheckNthSubPath(p, 0), [pe.Stroke()])
    440             pe2 = ConditionalStroke(CheckNthSubPath(p, 1), [pe.Stroke(dashes={'dash_offset': 0, 'dash_list': None})])
     440            pe2 = ConditionalStroke(CheckNthSubPath(p, 1), [pe.Stroke()])
    441441            p.set_path_effects([pe1, pe2])
    442442
    443443        subplot.add_patch(p)

There may be a better way to do this.

comment:198 Changed 3 years ago by git

  • Commit changed from 9b90efadd3a8aa3dfc2ecd77c3f8e538f20cf424 to 4005ec3dae5ab697d215d084d1e7bddea7ff110e

Branch pushed to git repo; I updated commit sha1. New commits:

4005ec3Fix the arrow matching pattern

comment:199 Changed 3 years ago by git

  • Commit changed from 4005ec3dae5ab697d215d084d1e7bddea7ff110e to 3ea7b1646ea138caf8e6b3651509c4711089cea0

Branch pushed to git repo; I updated commit sha1. New commits:

3ea7b16build qhull with cmake instead of custom script. This install can then be used by matplotlib.

comment:200 follow-ups: Changed 3 years ago by fbissey

  • Status changed from needs_work to needs_review
  • Work issues fix arrow.py and move qhull to cmake deleted

Bonus, building with optional qhull spkg now works. So one more time in review!

comment:201 in reply to: ↑ 200 Changed 3 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 b def sphinx_plot(plot): 
    2626    from sage.misc.temporary_file import tmp_filename
    2727    import matplotlib.pyplot as plt
    2828    import matplotlib as mpl
    29     mpl.rcParams['image.interpolation'] = 'bilinear'
    30     mpl.rcParams['image.resample'] = False
    3129    if os.environ.get('SAGE_SKIP_PLOT_DIRECTIVE', 'no') != 'yes':
     30        mpl.rcParams['image.interpolation'] = 'bilinear'
     31        mpl.rcParams['image.resample'] = False
     32        mpl.rcParams['figure.figsize'] = [8.0, 6.0]
     33        mpl.rcParams['figure.dpi'] = 80
     34        mpl.rcParams['savefig.dpi'] = 100
    3235        fn = tmp_filename(ext=".png")
    3336        plot.plot().save(fn)
    3437        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 3 years ago by egourgoulhon (previous) (diff)

comment:202 follow-up: Changed 3 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 3 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 3 years ago by egourgoulhon (previous) (diff)

comment:204 Changed 3 years ago by git

  • Commit changed from 3ea7b1646ea138caf8e6b3651509c4711089cea0 to 3a6792ea300002114fc701acfb14ea52f72fe040

Branch pushed to git repo; I updated commit sha1. New commits:

3a6792eSetting more parameters in sphinx_plot for better 3D documentation

comment:205 follow-up: Changed 3 years ago by egourgoulhon

  • Status changed from needs_review to 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: Changed 3 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: Changed 3 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 3 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 3 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 3 years ago by git

  • Commit changed from 3a6792ea300002114fc701acfb14ea52f72fe040 to 929509edcfe5278337087f280222fc9b7bb10bbd
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

929509euse helper scripts in qhull

comment:211 Changed 3 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 3 years ago by dimpase (previous) (diff)

comment:212 follow-up: Changed 3 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 3 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 3 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 3 years ago by dimpase (previous) (diff)

comment:215 Changed 3 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: Changed 3 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 3 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 3 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: Changed 3 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 3 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 3 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 3 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 3 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 3 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 3 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 3 years ago by fbissey (previous) (diff)

comment:226 Changed 3 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: Changed 3 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 3 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 3 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: Changed 3 years ago by fbissey

+1 to that. Do you have a clue where that should be set.

comment:231 in reply to: ↑ 230 Changed 3 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 3 years ago by git

  • Commit changed from 929509edcfe5278337087f280222fc9b7bb10bbd to 115842500d43cc3866c27292cf5680a317b5da3f

Branch pushed to git repo; I updated commit sha1. New commits:

1158425Make sure agg backend is set in matplotlib when doctests are run

comment:233 Changed 3 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 3 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 3 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 3 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 3 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 3 years ago by git

  • Commit changed from 115842500d43cc3866c27292cf5680a317b5da3f to 8dcadff0d0dcc86aff0111749f85082f888afc7a

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

8dcadffMake sure agg backend is set in matplotlib when doctests are run and put the call in the right spot

comment:239 Changed 3 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 3 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 3 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 3 years ago by dimpase (previous) (diff)

comment:242 Changed 3 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: Changed 3 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 3 years ago by fbissey

Replying to dimpase:

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

A laudable goal but not always reachable it seems https://github.com/sagemath/sage/blob/master/src/sage/structure/graphics_file.py#L175

I'll be getting code in the branch shortly (first day back to work).

comment:245 Changed 3 years ago by git

  • Commit changed from 8dcadff0d0dcc86aff0111749f85082f888afc7a to e0534a8c68bc60cf2495acf32598096d64579184

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

d7db6e1Merge branch 'develop' into MPL-2.1.0b
e0534a8Make all doctests set agg as the backend for matplotlib.

comment:246 Changed 3 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: Changed 3 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: Changed 3 years ago by strogdon

Replying to fbissey:

I am feeling like I am playing a game of "whack a mole". I may abandon the most general way to just care for the particular as it is clearly less exhausting.

I agree with this. There is clearly something awry with OSX that maybe should be on another ticket as long as the specific case here is addressed.

comment:249 in reply to: ↑ 248 Changed 3 years ago by dimpase

Replying to strogdon:

Replying to fbissey:

I am feeling like I am playing a game of "whack a mole". I may abandon the most general way to just care for the particular as it is clearly less exhausting.

I agree with this. There is clearly something awry with OSX that maybe should be on another ticket as long as the specific case here is addressed.

Well, the change I am proposing should actually as well take care of the problem that Debian people are patching, no? And this is actually a noninvasive way; otherwise Sage starts looking a bit like that famous VW dieselgate code (for running their diesel engines in test mode differently) should have looked...

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

comment:250 follow-up: Changed 3 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 3 years ago by dimpase

Replying to fbissey:

Testing something different in test/startup.py? That misses the point the test was introduced for. https://github.com/sagemath/sage/commit/bff6915f037ef1d2acbda426f96cb6060509901c last file on that commit.

I am fine with doing tests for numpy and pyparsing the IPython way, as you proposed. One example how to do this in the normal setting (with libgap) looks sufficient.

comment:252 Changed 3 years ago by git

  • Commit changed from e0534a8c68bc60cf2495acf32598096d64579184 to d5be931f48b77bb03d2d5750bea3805435e795fd

Branch pushed to git repo; I updated commit sha1. New commits:

d5be931Change the startup test for numpy and pyparsing now that they are imported in the doctesting framework via matplotlib.

comment:253 Changed 3 years ago by fbissey

And I hope it is the last of it.

comment:254 follow-up: Changed 3 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 3 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: Changed 3 years ago by fbissey

Replying to jhpalmieri:

Why not use a test like

    sage: cmd = "print('numpy' in sys.modules)"
    sage: print(test_executable(["sage", "-c", cmd])[0])  # long time
    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 3 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 3 years ago by dimpase

Replying to fbissey:

Replying to jhpalmieri:

Why not use a test like

    sage: cmd = "print('numpy' in sys.modules)"
    sage: print(test_executable(["sage", "-c", cmd])[0])  # long time
    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 3 years ago by git

  • Commit changed from d5be931f48b77bb03d2d5750bea3805435e795fd to ba7f0ab8dbde03cdd935233fc4228451b2cb0c87

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

a5edbfeChange the startup test for numpy and pyparsing now that they are imported in the doctesting framework via matplotlib.
ba7f0abMerge branch 'develop' into MPL-2.1.0b

comment:260 follow-up: Changed 3 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 3 years ago by egourgoulhon

Replying to fbissey:

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 3 years ago by dimpase

  • Status changed from needs_review to positive_review

OK, looks good on OSX too.

comment:263 Changed 3 years ago by egourgoulhon

  • Status changed from positive_review to needs_work

Sorry for removing the positive_review status, but I've just noticed the following doctest errors:

./sage -t --long --warn-long 46.0 src/sage/tests/startup.py
Running doctests with ID 2018-01-31-14-05-27-cae74607.
Git branch: MPL2.1.0
Using --optional=mpir,python2,sage
Doctesting 1 file.
sage -t --long --warn-long 46.0 src/sage/tests/startup.py
**********************************************************************
File "src/sage/tests/startup.py", line 25, in sage.tests.startup
Failed example:
    print(test_executable(["sage", "-c"], cmd)[0])  # long time
Expected:
    False
Got:
    <BLANKLINE>
**********************************************************************
File "src/sage/tests/startup.py", line 28, in sage.tests.startup
Failed example:
    print(test_executable(["sage", "-c"], cmd)[0])  # long time
Expected:
    False
Got:
    <BLANKLINE>
**********************************************************************
1 item had failures:
   2 of  10 in sage.tests.startup
    [9 tests, 2 failures, 4.22 s]
----------------------------------------------------------------------
sage -t --long --warn-long 46.0 src/sage/tests/startup.py  # 2 doctests failed
----------------------------------------------------------------------

This is with Ubuntu 16.04.

comment:264 Changed 3 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 3 years ago by egourgoulhon (previous) (diff)

comment:265 follow-up: Changed 3 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 3 years ago by jhpalmieri (previous) (diff)

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

Replying to jhpalmieri:

Please try the test the way I suggested in comment:254 (["sage", "-c", cmd] all in one set of square brackets). Does that work?

Yes this one works:

sage: from sage.tests.cmdline import test_executable
sage: cmd = "print('numpy' in sys.modules)\n"
sage: print(test_executable(["sage", "-c", cmd])[0])
False

sage: test_executable(["sage", "-c", cmd])
('False\n', '', 0)

comment:267 Changed 3 years ago by fbissey

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

comment:268 Changed 3 years ago by git

  • Commit changed from ba7f0ab8dbde03cdd935233fc4228451b2cb0c87 to ca3b242e6d7ca2ce22d8d23ef62d0cecd95b585f

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

f65ac29Merge with develop
ca3b242correct test in startup.py

comment:269 Changed 3 years ago by fbissey

  • Status changed from needs_work to needs_review

OK, it is corrected.

comment:270 Changed 3 years ago by dimpase

  • Status changed from needs_review to 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 3 years ago by egourgoulhon

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

Many thanks!

comment:272 Changed 3 years ago by vbraun

  • Branch changed from u/fbissey/MPL-2.1.0b to ca3b242e6d7ca2ce22d8d23ef62d0cecd95b585f
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:273 Changed 3 years ago by jdemeyer

  • Commit ca3b242e6d7ca2ce22d8d23ef62d0cecd95b585f deleted

See #24670 for a follow-up.

comment:274 Changed 3 years ago by jdemeyer

Serious breakage at #24671.

comment:275 Changed 3 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:

I'm asking because of this:

sage -t --long --warn-long 10.0 src/sage/plot/plot3d/list_plot3d.py
**********************************************************************
File "src/sage/plot/plot3d/list_plot3d.py", line 143, in sage.plot.plot3d.list_plot3d.list_plot3d
Warning, slow doctest:
    list_plot3d(l, interpolation_type='clough', texture='yellow', num_points=100)
Test ran for 926.47 s
**********************************************************************

926 seconds for a plot which is not even marked # long time?

comment:276 follow-up: Changed 3 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 3 years ago by jdemeyer

Replying to fbissey:

Clearly an oversight on my part. interpolation_type='nn' is gone - and not coming back anytime soon. This was discussed at length in this ticket and I pinged sage-devel. Following deafening silence on the options available, I choose a new alternative that looked promising time wise on the example that I timed. As you just shown I haven't timed every single doctests otherwise I may have noticed that this one was putting the new default under stress.

I'm not blaiming you... I didn't follow this ticket but given the number of comments, I guess it was not easy.

comment:278 Changed 3 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 3 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 3 years ago by jdemeyer

Follow-up at #24862.

comment:281 Changed 3 years ago by jdemeyer

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

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

By the way, matplotlib 2.2.0 has been released.

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

Replying to gh-dimpase:

By the way, matplotlib 2.2.0 has been released.

Is it python-3 only? I remember reading they were getting ready for that at the next "major" release.

comment:284 Changed 3 years ago by dimpase

I guess major should be vers. 3

comment:285 Changed 3 years ago by mmezzarobba

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

Note: See TracTickets for help on using tickets.