Opened 6 years ago
Closed 6 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 )
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
:
- class
Graphics
- class
GraphicsArray
- def
show_default
- 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)
Change History (34)
comment:1 Changed 6 years ago by
- Component changed from PLEASE CHANGE to plotting
comment:2 Changed 6 years ago by
- Description modified (diff)
comment:3 Changed 6 years ago by
- Cc kcrisman added
comment:4 Changed 6 years ago by
- Description modified (diff)
comment:5 Changed 6 years ago by
- Description modified (diff)
- Priority changed from major to blocker
comment:6 Changed 6 years ago by
comment:7 Changed 6 years ago by
- Component changed from plotting to graphics
- Description modified (diff)
- Status changed from new to needs_review
comment:8 follow-up: ↓ 9 Changed 6 years ago by
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: ↓ 20 Changed 6 years ago by
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 6 years ago by
- 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 6 years ago by
- 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 6 years ago by
- Status changed from needs_review to needs_work
Okay, I'll check the links.
comment:13 Changed 6 years ago by
- 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: ↓ 15 Changed 6 years ago by
- 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 6 years ago by
- 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 6 years ago by
Feel free to implement alternative solutions to the problem that plot.py
takes too long to doctest.
comment:17 Changed 6 years ago by
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 6 years ago by
comment:18 Changed 6 years ago by
sage/schemes/elliptic_curves/*
is okay since it uses:
import sage.plot.all as plot
comment:19 Changed 6 years ago by
- Status changed from needs_info to needs_review
- Work issues address deprecation concerns deleted
comment:20 in reply to: ↑ 9 Changed 6 years ago by
comment:21 Changed 6 years ago by
- 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 6 years ago by
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: ↓ 24 Changed 6 years ago by
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 6 years ago by
comment:24 in reply to: ↑ 23 Changed 6 years ago by
Replying to kcrisman:
Your patch needs to have a useful commit message.
Oups !
Also, after
sage: g2.show()
you should have a blank, likesage: 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: ↓ 29 Changed 6 years ago by
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: ↓ 27 Changed 6 years ago by
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 6 years ago by
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 6 years ago by
All doctests pass when applied to 5.0.beta13.
comment:29 in reply to: ↑ 25 Changed 6 years ago by
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: ↓ 31 Changed 6 years ago by
- 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 6 years ago by
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 6 years ago by
- Merged in set to sage-5.0.rc0
- Resolution set to fixed
- Status changed from positive_review to closed
The attached patch is a rough approximation, but mostly does it.