Opened 9 months ago

Closed 8 months ago

Last modified 3 months ago

#30176 closed enhancement (fixed)

Update matplotlib to 3.3

Reported by: arojas Owned by:
Priority: major Milestone: sage-9.2
Component: packages: standard Keywords:
Cc: jhpalmieri, fbissey, mkoeppe, gh-timokau, egourgoulhon, arojas, gh-zlscherr Merged in:
Authors: Antonio Rojas Reviewers: Matthias Koeppe
Report Upstream: Fixed upstream, but not in a stable release. Work issues:
Branch: 73af14a (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by arojas)

Brings lots of deprecation warnings on every plot() call, due to OldScalarFormatter? deprecation.

Depends on pillow for all image formats now.

Change History (43)

comment:1 Changed 9 months ago by arojas

  • Branch set to u/arojas/update_matplotlib_to_3_3

comment:2 Changed 9 months ago by git

  • Commit set to cdf20ea78ad840e812ae72ebb0d2e61ad229a797

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

cdf20eaReplace deprecated OldScalarFormatter with ScalarFormatter

comment:3 Changed 9 months ago by git

  • Commit changed from cdf20ea78ad840e812ae72ebb0d2e61ad229a797 to 80f12fa1968173d15da1d6224d3f868fa80b6f70

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

fee97c7Replace deprecated basex and basey parameters
3d41b34Update example
80f12faUpdate metrics for matplotlib 3.3

comment:4 Changed 9 months ago by arojas

  • Authors set to Antonio Rojas
  • Cc jhpalmieri fbissey mkoeppe gh-timokau added
  • Component changed from PLEASE CHANGE to packages: standard
  • Description modified (diff)
  • Type changed from PLEASE CHANGE to enhancement

comment:5 Changed 9 months ago by arojas

Still 3 test failures, all looking like

File "/usr/lib/python3.8/site-packages/sage/geometry/polyhedron/plot.py", line 1030, in sage.geometry.polyhedron.plot.Projection.render_2d
Failed example:
    q1.plot() + q2.plot() + q3.plot() + q4.plot()
Expected:
    Graphics object consisting of 17 graphics primitives
Got:
    doctest:warning
      File "/usr/bin/sage-runtests", line 177, in <module>
        err = DC.run()
      File "/usr/lib/python3.8/site-packages/sage/doctest/control.py", line 1196, in run
        self.run_doctests()
      File "/usr/lib/python3.8/site-packages/sage/doctest/control.py", line 897, in run_doctests
        self.dispatcher.dispatch()
      File "/usr/lib/python3.8/site-packages/sage/doctest/forker.py", line 2038, in dispatch
        self.parallel_dispatch()
      File "/usr/lib/python3.8/site-packages/sage/doctest/forker.py", line 1933, in parallel_dispatch
        w.start()  # This might take some time
      File "/usr/lib/python3.8/site-packages/sage/doctest/forker.py", line 2205, in start
        super(DocTestWorker, self).start()
      File "/usr/lib/python3.8/multiprocessing/process.py", line 121, in start
        self._popen = self._Popen(self)
      File "/usr/lib/python3.8/multiprocessing/context.py", line 224, in _Popen
        return _default_context.get_context().Process._Popen(process_obj)
      File "/usr/lib/python3.8/multiprocessing/context.py", line 277, in _Popen
        return Popen(process_obj)
      File "/usr/lib/python3.8/multiprocessing/popen_fork.py", line 19, in __init__
        self._launch(process_obj)
      File "/usr/lib/python3.8/multiprocessing/popen_fork.py", line 75, in _launch
        code = process_obj._bootstrap(parent_sentinel=child_r)
      File "/usr/lib/python3.8/multiprocessing/process.py", line 315, in _bootstrap
        self.run()
      File "/usr/lib/python3.8/site-packages/sage/doctest/forker.py", line 2177, in run
        task(self.options, self.outtmpfile, msgpipe, self.result_queue)
      File "/usr/lib/python3.8/site-packages/sage/doctest/forker.py", line 2506, in __call__
        doctests, extras = self._run(runner, options, results)
      File "/usr/lib/python3.8/site-packages/sage/doctest/forker.py", line 2552, in _run
        result = runner.run(test)
      File "/usr/lib/python3.8/site-packages/sage/doctest/forker.py", line 905, in run
        return self._run(test, compileflags, out)
      File "/usr/lib/python3.8/site-packages/sage/doctest/forker.py", line 707, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/usr/lib/python3.8/site-packages/sage/doctest/forker.py", line 1131, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.geometry.polyhedron.plot.Projection.render_2d[8]>", line 1, in <module>
        q1.plot() + q2.plot() + q3.plot() + q4.plot()
      File "/usr/lib/python3.8/site-packages/sage/repl/rich_output/display_manager.py", line 811, in displayhook
        plain_text, rich_output = self._rich_output_formatter(obj, dict())
      File "/usr/lib/python3.8/site-packages/sage/repl/rich_output/display_manager.py", line 625, in _rich_output_formatter
        rich_output = self._call_rich_repr(obj, rich_repr_kwds)
      File "/usr/lib/python3.8/site-packages/sage/repl/rich_output/display_manager.py", line 590, in _call_rich_repr
        warnings.warn(
      File "/usr/lib/python3.8/warnings.py", line 109, in _showwarnmsg
        sw(msg.message, msg.category, msg.filename, msg.lineno,
    :
    sage.repl.rich_output.display_manager.RichReprWarning: Exception in _rich_repr_ while displaying object: The first element of 'code' must be equal to 'MOVETO' (1).  Your first code is 79
    Graphics object consisting of 17 graphics primitives
**********************************************************************

comment:6 Changed 9 months ago by arojas

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

comment:7 Changed 9 months ago by mkoeppe

Upstream seems to have addressed it in https://github.com/matplotlib/matplotlib/pull/17982

comment:8 Changed 9 months ago by git

  • Commit changed from 80f12fa1968173d15da1d6224d3f868fa80b6f70 to dbdecd971e452c5c7f4d77b8a51ea613a98743fc

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

dbdecd9Backport patch to fix the path of degenerate polygons

comment:9 Changed 9 months ago by git

  • Commit changed from dbdecd971e452c5c7f4d77b8a51ea613a98743fc to fd2a9b8ce1bf5198e70592621754426500149a8b

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

fd2a9b8Bump version to account for the added patch

comment:10 Changed 9 months ago by arojas

  • Report Upstream changed from Reported upstream. No feedback yet. to Fixed upstream, but not in a stable release.
  • Status changed from new to needs_review

No more test failures on Arch -> needs review

comment:12 Changed 9 months ago by git

  • Commit changed from fd2a9b8ce1bf5198e70592621754426500149a8b to ad67266c7e84156d8a0ab9f9beb14fada29a3553

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

ad67266Remove merged patch

comment:14 Changed 9 months ago by mkoeppe

Fails on all platforms. For example https://github.com/mkoeppe/sage/runs/901333269

    error: Failed to download FreeType. Please download one of ['https://downloads.sourceforge.net/project/freetype/freetype2/2.6.1/freetype-2.6.1.tar.gz', 'https://download.savannah.gnu.org/releases/freetype/freetype-2.6.1.tar.gz'] and extract it into build/freetype-2.6.1 at the top-level of the source repository.
    Running setup.py install for matplotlib: finished with status 'error'
ERROR: Command errored out with exit status 1: /sage/local/bin/python3 -u -c 'import sys, setuptools, tokenize; sys.argv[0] = '"'"'/tmp/pip-req-build-_2y953_j/setup.py'"'"'; __file__='"'"'/tmp/pip-req-build-_2y953_j/setup.py'"'"';f=getattr(tokenize, '"'"'open'"'"', open)(__file__);code=f.read().replace('"'"'\r\n'"'"', '"'"'\n'"'"');f.close();exec(compile(code, __file__, '"'"'exec'"'"'))' --no-user-cfg install --record /tmp/pip-record-v8gxera1/install-record.txt --single-version-externally-managed --root /sage/local/var/tmp/sage/build/matplotlib-3.3.0.p0/inst --compile --install-headers /sage/local/var/tmp/sage/build/matplotlib-3.3.0.p0/inst/sage/local/include/python3.7m/matplotlib Check the logs for full command output.
Exception information:
Traceback (most recent call last):
  File "/sage/local/lib/python3.7/site-packages/pip/_internal/req/req_install.py", line 841, in install
    req_description=str(self.req),
  File "/sage/local/lib/python3.7/site-packages/pip/_internal/operations/install/legacy.py", line 86, in install
    raise LegacyInstallFailure

comment:15 Changed 9 months ago by mkoeppe

  • Status changed from needs_review to needs_work

comment:16 Changed 9 months ago by mkoeppe

https://github.com/matplotlib/matplotlib/blob/v3.3.x/INSTALL.rst: "By default ... Matplotlib downloads and builds its own copy of FreeType? ... and uses its own copy of Qhull. To force Matplotlib to use a copy of FreeType? or Qhull already installed in your system, create a setup.cfg file ...."

comment:17 follow-ups: Changed 9 months ago by arojas

should we make qhull standard and build matplotlib against it?

comment:18 in reply to: ↑ 17 Changed 9 months ago by mkoeppe

Replying to arojas:

should we make qhull standard and build matplotlib against it?

+1

comment:19 follow-up: Changed 9 months ago by mkoeppe

Of course, dealing with qhull is not very important and could be done on a follow up ticket. But the FreeType dependency needs to be fixed for this upgrade.

comment:20 Changed 9 months ago by git

  • Commit changed from ad67266c7e84156d8a0ab9f9beb14fada29a3553 to 8e489da53591788d8f45c580b5057d022dcc8dd3

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

8e489daBuild with system freetype, use ConfigParser instead of deprecated SafeConfigParser

comment:21 Changed 9 months ago by git

  • Commit changed from 8e489da53591788d8f45c580b5057d022dcc8dd3 to 79249f7592d18afd0d7685298d598406b4e11bda

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

79249f7Update metrics with system freetype

comment:22 in reply to: ↑ 19 ; follow-ups: Changed 9 months ago by arojas

Replying to mkoeppe:

Of course, dealing with qhull is not very important and could be done on a follow up ticket. But the FreeType dependency needs to be fixed for this upgrade.

It would also need cmake to become standard

comment:23 Changed 9 months ago by arojas

  • Status changed from needs_work to needs_review

comment:24 in reply to: ↑ 17 Changed 9 months ago by jhpalmieri

Replying to arojas:

should we make qhull standard and build matplotlib against it?

Why not just keep using matplotlib's version of qhull? Or conditionally do this if qhull is not available on the system?

Last edited 9 months ago by jhpalmieri (previous) (diff)

comment:25 in reply to: ↑ 22 ; follow-up: Changed 9 months ago by mkoeppe

comment:26 in reply to: ↑ 22 ; follow-ups: Changed 9 months ago by mkoeppe

Replying to arojas:

Replying to mkoeppe:

Of course, dealing with qhull is not very important and could be done on a follow up ticket. But the FreeType dependency needs to be fixed for this upgrade.

It would also need cmake to become standard

Is matplotlib using cmake now?

comment:27 in reply to: ↑ 26 Changed 9 months ago by jhpalmieri

Replying to mkoeppe:

Replying to arojas:

Replying to mkoeppe:

Of course, dealing with qhull is not very important and could be done on a follow up ticket. But the FreeType dependency needs to be fixed for this upgrade.

It would also need cmake to become standard

Is matplotlib using cmake now?

No, qhull uses it, or at least our spkg does.

comment:28 in reply to: ↑ 26 Changed 9 months ago by fbissey

Replying to mkoeppe:

Replying to arojas:

Replying to mkoeppe:

Of course, dealing with qhull is not very important and could be done on a follow up ticket. But the FreeType dependency needs to be fixed for this upgrade.

It would also need cmake to become standard

Is matplotlib using cmake now?

qhull is.

comment:29 Changed 9 months ago by fbissey

Replying to jhpalmieri:

Replying to arojas:

should we make qhull standard and build matplotlib against it?

Why not just keep using matplotlib's version of qhull? Or conditionally do this if qhull is not available on the system?

Unbundling is considered a good thing in general. About qhull, being able to use an external version of it in matplotlib has flip-floped. It wasn't possible in some previous releases. Matplotlib is the only package currently using qhull so I am not considering it a priority. If there were more of them, that would be more of a motivation.

comment:30 Changed 9 months ago by jhpalmieri

Although the most recent qhull may not require cmake. It comes with a Makefile, but I am not necessarily the best person to evaluate whether it can be used in place of building with cmake.

comment:31 in reply to: ↑ 25 Changed 9 months ago by mkoeppe

  • Cc egourgoulhon added
  • Reviewers set to Matthias Koeppe
  • Status changed from needs_review to positive_review

Replying to mkoeppe:

Tests run at https://github.com/mkoeppe/sage/actions/runs/181664763

This looks clean on all platforms.

I think it would be better if the changed multigraphics doctest was adjusted to be more tolerant instead of just updating the result.

Nevertheless, a positive review from my side.

comment:32 Changed 9 months ago by vbraun

  • Status changed from positive_review to needs_work

There is numerical noise on various buildbots, e.g.

**********************************************************************
File "src/sage/plot/multigraphics.py", line 1297, in sage.plot.multigraphics.GraphicsArray.position
Failed example:
    G.position(0)  # tol 1.0e-13
Expected:
    (0.025045451349937315,
     0.03415488992713045,
     0.4489880779745068,
     0.9345951100728696)
Got:
    (0.02494779509993732,
     0.03415488992713045,
     0.44913456234950677,
     0.9345951100728696)
Tolerance exceeded in 2 of 4:
    0.025045451349937315 vs 0.02494779509993732, tolerance 4e-3 > 1e-13
    0.4489880779745068 vs 0.44913456234950677, tolerance 4e-4 > 1e-13
**********************************************************************
File "src/sage/plot/multigraphics.py", line 1302, in sage.plot.multigraphics.GraphicsArray.position
Failed example:
    G.position(1)  # tol 1.0e-13
Expected:
    (0.5170637412999687,
     0.20212705964722733,
     0.4489880779745068,
     0.5986507706326758)
Got:
    (0.5170149131749686,
     0.20202940339722741,
     0.44913456234950666,
     0.5988460831326756)
Tolerance exceeded in 4 of 4:
    0.5170637412999687 vs 0.5170149131749686, tolerance 1e-4 > 1e-13
    0.20212705964722733 vs 0.20202940339722741, tolerance 5e-4 > 1e-13
    0.4489880779745068 vs 0.44913456234950666, tolerance 4e-4 > 1e-13
    0.5986507706326758 vs 0.5988460831326756, tolerance 4e-4 > 1e-13
**********************************************************************
1 item had failures:
   2 of   6 in sage.plot.multigraphics.GraphicsArray.position
    [192 tests, 2 failures, 20.75 s]
----------------------------------------------------------------------
sage -t --long --random-seed=0 src/sage/plot/multigraphics.py  # 2 doctests failed
----------------------------------------------------------------------

comment:33 Changed 9 months ago by vbraun

**********************************************************************
File "src/sage/plot/multigraphics.py", line 1297, in sage.plot.multigraphics.GraphicsArray.position
Failed example:
    G.position(0)  # tol 1.0e-13
Expected:
    (0.025045451349937315,
     0.03415488992713045,
     0.4489880779745068,
     0.9345951100728696)
Got:
    (0.02494779509993732,
     0.03415488992713045,
     0.44913456234950677,
     0.9345951100728696)
Tolerance exceeded in 2 of 4:
    0.025045451349937315 vs 0.02494779509993732, tolerance 4e-3 > 1e-13
    0.4489880779745068 vs 0.44913456234950677, tolerance 4e-4 > 1e-13
**********************************************************************
File "src/sage/plot/multigraphics.py", line 1302, in sage.plot.multigraphics.GraphicsArray.position
Failed example:
    G.position(1)  # tol 1.0e-13
Expected:
    (0.5170637412999687,
     0.20212705964722733,
     0.4489880779745068,
     0.5986507706326758)
Got:
    (0.5170149131749686,
     0.20202940339722741,
     0.44913456234950666,
     0.5988460831326756)
Tolerance exceeded in 4 of 4:
    0.5170637412999687 vs 0.5170149131749686, tolerance 1e-4 > 1e-13
    0.20212705964722733 vs 0.20202940339722741, tolerance 5e-4 > 1e-13
    0.4489880779745068 vs 0.44913456234950666, tolerance 4e-4 > 1e-13
    0.5986507706326758 vs 0.5988460831326756, tolerance 4e-4 > 1e-13
**********************************************************************
1 item had failures:
   2 of   6 in sage.plot.multigraphics.GraphicsArray.position
    [192 tests, 2 failures, 27.84 s]
----------------------------------------------------------------------
sage -t --long --random-seed=0 src/sage/plot/multigraphics.py  # 2 doctests failed
----------------------------------------------------------------------

comment:34 Changed 8 months ago by arojas

  • Cc arojas added

comment:35 Changed 8 months ago by git

  • Commit changed from 79249f7592d18afd0d7685298d598406b4e11bda to 20ecd183f2fb656fe97fd1d7b6ce9d3824081d74

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

20ecd18Increase numerical noise tolerance

comment:36 Changed 8 months ago by arojas

  • Status changed from needs_work to positive_review

comment:37 Changed 8 months ago by vbraun

  • Status changed from positive_review to needs_work
**********************************************************************
File "src/sage/plot/multigraphics.py", line 1297, in sage.plot.multigraphics.GraphicsArray.position
Failed example:
    G.position(0)  # tol 1.0e-3
Expected:
    (0.025045451349937315,
     0.03415488992713045,
     0.4489880779745068,
     0.9345951100728696)
Got:
    (0.02494779509993732,
     0.03415488992713045,
     0.44913456234950677,
     0.9345951100728696)
Tolerance exceeded in 1 of 4:
    0.025045451349937315 vs 0.02494779509993732, tolerance 4e-3 > 1e-3
**********************************************************************
1 item had failures:
   1 of   6 in sage.plot.multigraphics.GraphicsArray.position
    [192 tests, 1 failure, 33.68 s]
----------------------------------------------------------------------
sage -t --long --random-seed=0 src/sage/plot/multigraphics.py  # 1 doctest failed
----------------------------------------------------------------------

comment:38 Changed 8 months ago by git

  • Commit changed from 20ecd183f2fb656fe97fd1d7b6ce9d3824081d74 to 73af14a4f1f1453cab53d882f7cb2ac5d7d39d41

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

73af14aAdjust numeric noise tolerance further

comment:39 Changed 8 months ago by arojas

  • Status changed from needs_work to positive_review

comment:40 Changed 8 months ago by vbraun

  • Branch changed from u/arojas/update_matplotlib_to_3_3 to 73af14a4f1f1453cab53d882f7cb2ac5d7d39d41
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:41 Changed 4 months ago by kcrisman

  • Commit 73af14a4f1f1453cab53d882f7cb2ac5d7d39d41 deleted

Hi! Is it possible that this change is responsible for the difference between the following two images?

Notice that not only is the image slightly narrower, but the labels now all have default of one place beyond the decimal point, which in the past was not desired for the evident reasons we see here. That might be good for scientific plotting, but not for mathematical plotting.

comment:42 Changed 4 months ago by kcrisman

(Note that this explanation of the difference doesn't really seem to have any examples relevant to this situation.)

comment:43 Changed 3 months ago by mkoeppe

  • Cc gh-zlscherr added

Follow-up on qhull in #31148

Note: See TracTickets for help on using tickets.