Opened 11 months ago

Closed 9 months ago

Last modified 9 months ago

#27865 closed defect (fixed)

Refactor GraphicsArray, fixing various issues

Reported by: egourgoulhon Owned by:
Priority: major Milestone: sage-8.9
Component: graphics Keywords: graphics_array, matplotlib
Cc: kcrisman, chapoton, dkrenn, jdemeyer, embray Merged in:
Authors: Eric Gourgoulhon Reviewers: Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: 36c0f17 (Commits) Commit: 36c0f178353017ac12573ff038c40ed55a24ba2d
Dependencies: Stopgaps:

Description (last modified by egourgoulhon)

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.

Attachments (2)

graphics_array_bug.png (42.9 KB) - added by egourgoulhon 11 months ago.
graphics_array_ok.png (55.7 KB) - added by egourgoulhon 11 months ago.

Download all attachments as: .zip

Change History (24)

comment:1 Changed 11 months ago by egourgoulhon

  • Authors set to Eric Gourgoulhon
  • Branch set to public/graphics/multi_graphics
  • Commit set to 3a5ef55af4d104c840107f46abbcba90fa401a18

Last 10 new commits:

4bbdf88Add helper function _parse_figsize in module graphics and use it in matplotlib() methods of classes Graphics and MultiGraphics
788a53dAdd documentation in module multigraphics
7d61e86Improve documentation of GraphicsArray
af6d312More documentation in classes MultiGraphics and GraphicsArray
58b3ca3Completed documentation of MultiGraphics and GraphicsArray
1bf8ad1Small cleaning in MultiGraphics
ccff60cFix some doctests after the improved computation of nrows and ncols in graphics_array
26180abSome cleaning in MultiGraphics.matplotlib
3bd6a56Minor cleaning in GraphicsArray
3a5ef55Change index range in GraphicsArray._add_subplot()

comment:2 Changed 11 months ago by egourgoulhon

  • Description modified (diff)

Changed 11 months ago by egourgoulhon

Changed 11 months ago by egourgoulhon

comment:3 Changed 11 months ago by egourgoulhon

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.

Last edited 11 months ago by egourgoulhon (previous) (diff)

comment:4 Changed 11 months ago by egourgoulhon

  • Cc kcrisman chapoton dkrenn jdemeyer added
  • Status changed from new to needs_review

comment:5 Changed 11 months ago by egourgoulhon

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.

comment:6 Changed 11 months ago by egourgoulhon

  • Description modified (diff)

comment:7 follow-ups: Changed 11 months ago by kcrisman

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.

comment:8 in reply to: ↑ 7 Changed 11 months ago by egourgoulhon

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.

comment:9 Changed 11 months ago by egourgoulhon

  • Cc embray added

comment:10 Changed 11 months ago by egourgoulhon

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) ?
Last edited 11 months ago by egourgoulhon (previous) (diff)

comment:11 follow-up: Changed 10 months ago by chapoton

some failing doctests, see patchbot reports

comment:12 Changed 10 months ago by git

  • Commit changed from 3a5ef55af4d104c840107f46abbcba90fa401a18 to af2bf46ee0fc9cc5d24a696d55f4a1af1cc45f52

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

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

comment:13 in reply to: ↑ 11 ; follow-up: Changed 10 months ago by egourgoulhon

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.

Last edited 10 months ago by egourgoulhon (previous) (diff)

comment:14 Changed 10 months ago by git

  • Commit changed from af2bf46ee0fc9cc5d24a696d55f4a1af1cc45f52 to 36c0f178353017ac12573ff038c40ed55a24ba2d

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

36c0f17Error message valid for py3 only in a multigraphics doctest

comment:15 in reply to: ↑ 13 Changed 10 months ago by egourgoulhon

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.

comment:16 in reply to: ↑ 7 ; follow-up: Changed 10 months ago by egourgoulhon

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.

comment:17 Changed 10 months ago by egourgoulhon

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

comment:18 in reply to: ↑ 16 Changed 10 months ago by 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.

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!

comment:19 follow-up: Changed 10 months ago by chapoton

  • Reviewers set to Frédéric Chapoton
  • Status changed from needs_review to positive_review

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.

comment:20 in reply to: ↑ 19 Changed 10 months ago by egourgoulhon

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!

comment:21 Changed 9 months ago by vbraun

  • Branch changed from public/graphics/multi_graphics to 36c0f178353017ac12573ff038c40ed55a24ba2d
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:22 Changed 9 months ago by embray

  • Milestone changed from sage-8.8 to sage-8.9

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.

Note: See TracTickets for help on using tickets.