Refactor GraphicsArray, fixing various issues

[Eric Gourgoulhon]

This ticket makes GraphicsArray derive from a new class, MultiGraphics, which is more generic (arbitrary positions of graphics objects on a canvas) and paves the way towards graphics insets in #27866.

GraphicsArray is now endowed with a matplotlib() function (inheritated from MultiGraphics). This fixes the issues reported in #10466, #10657, #11160 and #12591 (see :comment:3 and comment:5 below for a summary). Moreover, this simplifies sphinx_plot() (defined in src/doc/common/conf.py), avoiding code duplication.

Beside introducing matplotlib() at the graphics array level, two bugs had to be corrected in Graphics.matplotlib():

• _set_scale was called on the whole figure, while it should be called on the current subplot
• tick_params was called on figure.get_axes()[0], i.e. the first subplot of the figure, while it should be called on the current subplot.

The documentation of Graphics.matplotlib() is also improved in this ticket.

Another bug has been corrected in Graphics.save(): contrary to what was claimed in the documentation g.save("filename") with "filename" having no extension did not save in a sobj file but yielded to an error.

Another modification is the introduction of the helper function _parse_figsize() in src/sage/plot/graphics.py to avoid code duplication: it is used in Graphics.matplotlib(), MultiGraphics.matplotlib() and sphinx_plot().

Since the file src/sage/plot/graphics.py was already quite long (3736 lines!), the class GraphicsArray was moved to the new file src/sage/plot/multigraphics.py, which contains the definition of the base class MultiGraphics. This makes things more consistent:

• src/sage/plot/graphics.py: only Graphics objects
• src/sage/plot/multigraphics.py: only MultiGraphics objects

Besides the bugs reported in the tickets #10466, #10657, #11160 and #12591, this ticket fixes two other bugs:

• if any plot in the array had a log scale, the latter was erroneously applied to the first plot of the array

• the computation of nrows and ncols in the function graphics_array() (defined in src/sage/plot/plot.py) yielded unnecessary large numbers in certain cases. For instance we have in Sage 8.7:

  sage: graphics_array([plot(sin)]*4, ncols=4)
  Graphics Array of size 2 x 4
  

  with the second raw that is entirely blank. With the code in this ticket the output is

  Graphics Array of size 1 x 4
  

  This fix explains the change of doctest outputs in src/sage/categories/finite_posets.py and src/sage/repl/rich_output/pretty_print.py

A preview (including the functionalities introduced in #27866) is available ​here.

[Eric Gourgoulhon]

Last 10 new commits:

​4bbdf88	Add helper function _parse_figsize in module graphics and use it in matplotlib() methods of classes Graphics and MultiGraphics
​788a53d	Add documentation in module multigraphics
​7d61e86	Improve documentation of GraphicsArray
​af6d312	More documentation in classes MultiGraphics and GraphicsArray
​58b3ca3	Completed documentation of MultiGraphics and GraphicsArray
​1bf8ad1	Small cleaning in MultiGraphics
​ccff60c	Fix some doctests after the improved computation of nrows and ncols in graphics_array
​26180ab	Some cleaning in MultiGraphics.matplotlib
​3bd6a56	Minor cleaning in GraphicsArray
​3a5ef55	Change index range in GraphicsArray._add_subplot()

[Eric Gourgoulhon]

Here is an example summarizing some the GraphicsArray issues. In Sage 8.7, the following code:

sage: g1 = plot(sin(x^2), (x, 0, 6), 
....:           axes_labels=[r'$\theta$', r'$\sin(\theta^2)$'], fontsize=14)
sage: g2 = circle((0,0), 1, fill=True, color='red', alpha=0.4, axes=False) \
....:      + text("This is a disk", (0.9, 1.1), color='red', alpha=0.4, 
....:             fontsize=12)
sage: g3 = plot(x^3, (x, 1, 100), axes_labels=[r'$x$', r'$y$'], 
....:           legend_label=r'$y = x^3$', scale='semilogy', frame=True, 
....:           gridlines='minor') \
....:      + text("This is a semilogy plot", (80, 1e5), color='red', alpha=0.4, 
....:             fontsize=12)
sage: graphics_array([g1, g2, g3])

results in this figure:

We see that

• the semilogy scale asked for g3 is actually applied to g1
• the fontsize asked for g1 is not taken into account (this is #10657)
• the option axes=False asked for g2 is not taken into account (#10466 and #10657)
• the legend of g3 is doubled (#12591)
• the color of the text in g3 is not the same as in g2, while they both defined as color='red', alpha=0.4 (#12591)

With the code introduced in this ticket, the output becomes

We see that all the above issues are gone.

[Eric Gourgoulhon]

Another bug fixed by this ticket regards the options passed to the method GraphicsArray.show(). This bug shows up in the ​2D plotting section of the reference manual. In the example starting with "The basic options for filling a plot", the figure resulting from

sage: p1 = plot(sin(x), -pi, pi, fill='axis')
sage: p2 = plot(sin(x), -pi, pi, fill='min', fillalpha=1)
sage: p3 = plot(sin(x), -pi, pi, fill='max')
sage: p4 = plot(sin(x), -pi, pi, fill=(1-x)/3, fillcolor='blue', fillalpha=.2)
sage: graphics_array([[p1, p2], [p3, p4]]).show(frame=True, axes=False)

has obviously some axes and no frame...

Compare with the figure produced by the code in this ticket ​here.

[Karl-Dieter Crisman]

THIS IS AMAZING!

---

I (as usual the past few years, I am sorry) will not be able to provide code review. But I think this is a great improvement. The multigraphics examples at the end of ​the example page are particularly impressive, and bring us a long ways toward having a "mathematical" way to interface with the stuff matplotlib provides - I could imagine it being particularly helpful with SageTeX. Thank you.

As my only suggestion, I would just urge you to be sure all previously "wrong" behavior shows up in the tests section, and that any errors be tested as well. As an example, you mention

For instance, G[0, 1] will throw an error.

so might as well put in

sage: G[0,1]
IndexError ...

or whatever is appropriate. We've definitely caught subtle things over the years with those kinds of tests.

[Eric Gourgoulhon]

Replying to kcrisman:

As my only suggestion, I would just urge you to be sure all previously "wrong" behavior shows up in the tests section, and that any errors be tested as well. As an example, you mention

For instance, G[0, 1] will throw an error.

so might as well put in

sage: G[0,1]
IndexError ...

or whatever is appropriate. We've definitely caught subtle things over the years with those kinds of tests.

OK, no problem, I'll introduce these tests. And thanks for your comments.

[Eric Gourgoulhon]

During the preparation of this ticket, two questions arose:

• the method Graphics.matplotlib() has the optional argument filename=None, which is not used in the function body; can we get rid of it?
• the method Graphics.save_image() merely falls back to Graphics.save(); can we remove it (after some deprecation period of course) ?

[Frédéric Chapoton]

some failing doctests, see patchbot reports

[git]

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

​ebebd4f	Merge branch 'public/graphics/multi_graphics' of git://trac.sagemath.org/sage into Sage 8.8.beta7.
​af2bf46	Restore 'not tested' flag on MultiGraphics._latex_() + example of error in GraphicsArray.__getitem__()

[Eric Gourgoulhon]

Replying to chapoton:

some failing doctests, see patchbot reports

From all the errors reported bu the patchbots, it seems that only one pertains to this ticket:

File "src/sage/plot/multigraphics.py", line 427, in 
sage.plot.multigraphics.MultiGraphics._latex_
Failed example:
    A._latex_()[:40]
Exception raised:
...
      File "/Users/jpalmier/Desktop/Sage/git/patchbot/sage/local/lib/python2.7/site-packages/matplotlib/backends/backend_pgf.py", line 315, in __init__
        "or error in preamble:\n%s" % stdout)
    LatexError: LaTeX returned an error, probably missing font or error in preamble:
...
    ! Package fontspec Error: The font "Bitstream Vera Serif" cannot be found.

This doctest is passed on my computer (Ubuntu 18.04). It fails on macOS patchbots. The reason seems to be this Matplotlib issue: ​https://github.com/matplotlib/matplotlib/issues/10307

Actually, this doctest was borrowed from the previous method GraphicsArray._latex_(), since now the method _latex_() is defined at the level of the base class MultiGraphics. Its failure had not been revealed previously by the macOS patchbots because the doctest was marked with # not tested. When I transferred the method _latex_() from GraphicsArray to MultiGraphics, I suppressed the marker # not tested because the doctest passed on my computer and I though there was then no reason for such a marker.

I have restored the # not tested marker in the above commit, along with a comment about the Matplotlib issue on macOS.

The patchbots also complain about 4 pyflakes issues. However, these reports are spurious for 2 of them:

• src/doc/common/conf.py:4: 'sage.misc.sagedoc.extlinks' imported but unused
  

  => if one suppresses this import, then the documentation fails to build (stating that :trac: is unknown)

• src/sage/plot/plot.py:585: 'sage.plot.line.line2d' imported but unused
  

  => this line is preceded by the following comment:

  # import of line2d below is only for redirection of imports
  

  so I guess one should keep it.

The other 2 pyflakes errors are

src/sage/repl/rich_output/pretty_print.py:34: 'types' imported but unused
src/sage/repl/rich_output/pretty_print.py:35: 'collections' imported but unused

These imports have not been introduced by the currrent ticket and I am not sure whether one could removed them without any subtle side effect...

Besides, the above commit implements the G[0, 1] error example suggested by Karl-Dieter in comment:7.

[git]

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

​36c0f17

Error message valid for py3 only in a multigraphics doctest

[Eric Gourgoulhon]

Replying to egourgoulhon:

Besides, the above commit implements the G[0, 1] error example suggested by Karl-Dieter in comment:7.

I've prepared the G[0, 1] example with py3 Sage and it turns out that the error message is different in py2. This triggered a new doctest error for the py2 patchbot. The above commit (comment:14) fixes this.

[Eric Gourgoulhon]

Replying to kcrisman:

As my only suggestion, I would just urge you to be sure all previously "wrong" behavior shows up in the tests section

Actually, the previous wrong behaviors were "silent" errors, which showed up only in the figures, as illustrated in comment:3. Their fixes therefore cannot be checked in doctests. I've however chosen the doctests in the EXAMPLES section of sage.plot.plot.graphics_array and sage.plot.multigraphics.GraphicsArray to include most of (if not all) the issues exhibited in comment:3 and comment:5. But at the end, the fix can only be judged by a human eye.

[Eric Gourgoulhon]

It seems there is no longer any failing doctest reported by the patchbots (except for spurious ones, i.e. failures that appear on the ticket 0 as well: ​https://patchbot.sagemath.org/ticket/0/)...

[Karl-Dieter Crisman]

As my only suggestion, I would just urge you to be sure all previously "wrong" behavior shows up in the tests section

Actually, the previous wrong behaviors were "silent" errors, which showed up only in the figures, as illustrated in comment:3. Their fixes therefore cannot be checked in doctests. I've however chosen the doctests in the EXAMPLES section of sage.plot.plot.graphics_array and sage.plot.multigraphics.GraphicsArray to include most of (if not all) the issues exhibited in comment:3 and comment:5. But at the end, the fix can only be judged by a human eye.

Oh, that was all I meant. Yes, at one point we considered trying to automate doctesting the images themselves (!) but especially now that a lot of images are created for the documentation, regressions should be easier to identify, and we would like to have those available. Thanks!

[Frédéric Chapoton]

I am giving a positive review. This is a very nice progress, that fixes a whole lot of long-standing bugs. Fixed tickets will have to be closed later, with appropriate doctests if necessary.

[Eric Gourgoulhon]

Replying to chapoton:

I am giving a positive review. This is a very nice progress, that fixes a whole lot of long-standing bugs. Fixed tickets will have to be closed later, with appropriate doctests if necessary.

Thanks for the review!

[Erik Bray]

Not in Sage 8.8. Let's please to try keep tickets' milestones related to the release in which we actually intend to include them, and in particular the release in which they were actually included, especially when closing tickets.