Split off Graphics class from plot.py

[jdemeyer]

The file sage/plot/plot.py is getting huge. One consequence is that doctesting this file (without --long) takes double the time of any other file. blocker because it regularly causes doctest timeouts.

The following will be moved to a new file graphics.py:

1. class Graphics
2. class GraphicsArray
3. def show_default
4. def is_Graphics

A few doctests consistently taking at least 2 seconds on sage.math have been marked # long time.

Before this patch on sage.math:

sage -t  "devel/sage/sage/plot/plot.py"
         [133.6 s]

After this patch on sage.math:

sage -t  "devel/sage/sage/plot/graphics.py"
         [51.5 s]
sage -t  "devel/sage/sage/plot/plot.py"
         [67.9 s]

The patch was created by first copying plot.py to graphics.py and then removing the duplicate code in either plot.py or graphics.py.

Apply :

• attachment:12857_split_graphics.patch
• attachment:trac_12857-split_graphics_review-fh.patch

[jdemeyer]

The attached patch is a rough approximation, but mostly does it.

[kcrisman]

Quite the patch bomb! I won't be able to look at this this weekend for sure. But I would ask you whether you might try building the documentation with this patch; I have a suspicion that there are cross-links that won't work now (probably not too many, but some).

Thanks for cutting the rough draft on this.

[jdemeyer]

Replying to kcrisman:

Quite the patch bomb! I won't be able to look at this this weekend for sure. But I would ask you whether you might try building the documentation with this patch; I have a suspicion that there are cross-links that won't work now (probably not too many, but some).

Building the documentation works. Is there an automatic way to check for broken cross-links?

[jdemeyer]

The "patch bomb" is not as bad as it looks: large parts of the patch simply remove duplicate code from plot.py xor graphics.py. The rest is mostly obvious changing of imports.

[benjaminfjones]

The patch applies cleanly to 5.0.beta12 and all tests pass on sage/plot/*.py. I also looked through the live documentation in the 2d/3d graphics sections and everything looks good to me. I didn't look for broken links, though.

[jdemeyer]

Okay, I'll check the links.

[jdemeyer]

Fixed some more links. I think the documentation is okay now, except for one huge problem: the module sage.combinat.e_one_star is documented as UNABLE TO IMPORT MODULE, see my sage-devel post.

[jdemeyer]

The UNABLE TO IMPORT MODULE seems like a Sphinx bug, or a bug in how Sage calls Sphinx.

I did a diff of the old and new documentation and, as far as I could see, all links still work. Needs review.

[niles]

Replying to jdemeyer:

The UNABLE TO IMPORT MODULE seems like a Sphinx bug, or a bug in how Sage calls Sphinx.

I did a diff of the old and new documentation and, as far as I could see, all links still work. Needs review.

Shouldn't it be necessary to issue a deprecation warning for at least a few Sage releases before we change the location of something? If I understand correctly, this patch will break all uses of sage.plot.plot.Graphics. To me, this seems like a bad idea for 5.0. Doing search_src('plot.Graphics') brings up a few more places Graphics and GraphicsArray are used in this way, but not addressed by this patch. For example, graphs/graph_generators.py and shemes/elliptic_curves/ell_generic.py.

I'm switching this to "needs info", and someone can overrule me if there is good reason to skip the deprecation process for this ticket.

[jdemeyer]

Feel free to implement alternative solutions to the problem that plot.py takes too long to doctest.

[jdemeyer]

Actually, sage.plot.plot.Graphics and sage.plot.plot.GraphicsArray still work since Graphics and GraphicsArray are imported from sage.plot.plot. So there isn't really a problem...

[jdemeyer]

sage/schemes/elliptic_curves/* is okay since it uses:

import sage.plot.all as plot

[hivert]

Replying to jdemeyer:

Building the documentation works. Is there an automatic way to check for broken cross-links?

Yes ! There is an option "--warn-links" to docbuild recently added by #9128.

I didn't yet check the documentation but I've a small reviewer patch which silent sage -coverage.

Florent

[hivert]

Good news ! No broken links were reported ! So upto doctesting the whole Sage lib, I'm positive review with the patch. Please review my review patch.

[hivert]

A ptestlong reported

All tests passed!

with the two patch applied. So it just remains to review my reviewer patch which should be quite trivial.

Florent

[kcrisman]

Your patch needs to have a useful commit message. Also, after sage: g2.show() you should have a blank, like

    sage: g2.show() 

::

    sage: isinstance(g2, Graphics)

so that people can see the graphic. In this case, an empty one with just axes, but it does work.

Otherwise this reviewer patch seems ok, modulo that I haven't tested whether anything applies - taking your word for it.

[hivert]

Replying to kcrisman:

Your patch needs to have a useful commit message.

Oups !

Also, after sage: g2.show() you should have a blank, like

    sage: g2.show() 

::

    sage: isinstance(g2, Graphics)

so that people can see the graphic. In this case, an empty one with just axes, but it does work.

Otherwise this reviewer patch seems ok, modulo that I haven't tested whether anything applies - taking your word for it.

The new uploaded patch should adress you comments.

[kcrisman]

The reviewer patch is now fine as far as it goes.

---

I'm still a little nervous about such a big change (I know it's not really big in that sense, but it does make a large change to where things are located) in an rc. I bet some outside links to the Sage doc will break, and the biggest problem is that all those helpful show options, which are only documented there, are now not in the same place as plot. Although in principle redistributing like this is good and we should aim to fix timeouts, it does seem that this ticket was created in response to a somewhat contrived problem (timeout on busy systems). Presumably someone can always make a system busy enough that Sage doctests will time out. I'm just not sure that the last release candidate is the place for something that should be used "in practice" at least a little while.

I don't have time to come up with any other solutions, and if Ben checked things out and DOCTEST_MODE and EMBEDDED_MODE and so forth still work (and whatever other constants like DPI there are) without trouble in the same ways as before, then I guess that's okay. But it does seems like (said the person who has never done a release, nor has the skills to do one) that we should have had a Sage 4.9 a few betas ago so this kind of thing wouldn't have happened...

[jhpalmieri]

It sounds to me as though we should give this a positive review so that it can go into a prerelease and get lots of testing, get lots of people looking at it. (I would suggest the same for #11616, to make it easy to test it on different platforms.)

[benjaminfjones]

Replying to jhpalmieri:

It sounds to me as though we should give this a positive review so that it can go into a prerelease and get lots of testing, get lots of people looking at it. (I would suggest the same for #11616, to make it easy to test it on different platforms.)

I agree with @jhpalmieri. I've looked over the patch again and the reviewer patch. This gets a positive review from me. I'm running a patchbot to check the doctests again. It's pending.

[benjaminfjones]

All doctests pass when applied to 5.0.beta13.

[jdemeyer]

Replying to kcrisman:

it does seem that this ticket was created in response to a somewhat contrived problem (timeout on busy systems

It is not a "contrived problem". It is a real issue happening in practice.

If somebody comes up with a different solution to these timeouts, that's fine for me. I just think that the timeouts should be fixed in sage-5.0 and splitting plot.py seemed a reasonable solution.

[hivert]

Despite kcrisman comments which are by the way quite sound, and seconded by jhpalmieri and benjaminfjones, I'm giving positive review. I trust Jeroen for knowing the risk we take by merging such a big change in and RC. He certainly known better that us the balance risks/advantages of merging this one.

Florent

[kcrisman]

Replying to hivert:

Despite kcrisman comments which are by the way quite sound, and seconded by jhpalmieri and benjaminfjones, I'm giving positive review. I trust Jeroen for knowing the risk we take by merging such a big change in and RC. He certainly known better that us the balance risks/advantages of merging this one.

Okay, I'm just glad there was some good discussion on this, and with several more eyes this seems reasonable. I'm not suggesting at all that there is anything wrong with the patch, just that we have been hit in the past by unintended moving of things.