Opened 5 years ago

Closed 5 years ago

#12857 closed enhancement (fixed)

Split off Graphics class from plot.py

Reported by: jdemeyer Owned by: tbd
Priority: blocker Milestone: sage-5.0
Component: graphics Keywords:
Cc: kcrisman Merged in: sage-5.0.rc0
Authors: Jeroen Demeyer Reviewers: Benjamin Jones, Florent Hivert
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by hivert)

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 :

Attachments (2)

12857_split_graphics.patch (229.6 KB) - added by jdemeyer 5 years ago.
trac_12857-split_graphics_review-fh.patch (1.9 KB) - added by hivert 5 years ago.

Download all attachments as: .zip

Change History (34)

comment:1 Changed 5 years ago by jdemeyer

  • Component changed from PLEASE CHANGE to plotting

comment:2 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 5 years ago by jdemeyer

  • Cc kcrisman added

comment:4 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:5 Changed 5 years ago by jdemeyer

  • Description modified (diff)
  • Priority changed from major to blocker

comment:6 Changed 5 years ago by jdemeyer

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

comment:7 Changed 5 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Component changed from plotting to graphics
  • Description modified (diff)
  • Status changed from new to needs_review

comment:8 follow-up: Changed 5 years ago by 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.

comment:9 in reply to: ↑ 8 ; follow-up: Changed 5 years ago by 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?

comment:10 Changed 5 years ago by jdemeyer

  • Description modified (diff)

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.

comment:11 Changed 5 years ago by benjaminfjones

  • Reviewers set to Benjamin Jones

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.

comment:12 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Okay, I'll check the links.

comment:13 Changed 5 years ago by jdemeyer

  • Work issues set to sage.combinat.e_one_star documentation

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.

comment:14 follow-up: Changed 5 years ago by jdemeyer

  • Status changed from needs_work to needs_review
  • Work issues sage.combinat.e_one_star documentation deleted

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.

comment:15 in reply to: ↑ 14 Changed 5 years ago by niles

  • Status changed from needs_review to needs_info
  • Work issues set to address deprecation concerns

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.

comment:16 Changed 5 years ago by jdemeyer

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

comment:17 Changed 5 years ago by 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...

Changed 5 years ago by jdemeyer

comment:18 Changed 5 years ago by jdemeyer

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

import sage.plot.all as plot

comment:19 Changed 5 years ago by jdemeyer

  • Status changed from needs_info to needs_review
  • Work issues address deprecation concerns deleted

comment:20 in reply to: ↑ 9 Changed 5 years ago by 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

comment:21 Changed 5 years ago by hivert

  • Description modified (diff)
  • Reviewers changed from Benjamin Jones to Benjamin Jones, 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.

comment:22 Changed 5 years ago by 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

comment:23 follow-up: Changed 5 years ago by 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.

Changed 5 years ago by hivert

comment:24 in reply to: ↑ 23 Changed 5 years ago by 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.

comment:25 follow-up: Changed 5 years ago by 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...

comment:26 follow-up: Changed 5 years ago by 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.)

comment:27 in reply to: ↑ 26 Changed 5 years ago by 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.

comment:28 Changed 5 years ago by benjaminfjones

All doctests pass when applied to 5.0.beta13.

comment:29 in reply to: ↑ 25 Changed 5 years ago by 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.

comment:30 follow-up: Changed 5 years ago by hivert

  • Status changed from needs_review to positive_review

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

comment:31 in reply to: ↑ 30 Changed 5 years ago by 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.

comment:32 Changed 5 years ago by jdemeyer

  • Merged in set to sage-5.0.rc0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.