Opened 9 years ago

Closed 9 years ago

#10143 closed enhancement (fixed)

Bring 2D plotting up to 100% doctest coverage (except plot.py)

Reported by: kcrisman Owned by: mvngu
Priority: minor Milestone: sage-4.6.2
Component: documentation Keywords:
Cc: jason, mvngu Merged in: sage-4.6.2.alpha3
Authors: Karl-Dieter Crisman Reviewers: Minh Van Nguyen, Geoffrey Ehrman
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

There are a few functions missing doctesting in sage/plot that aren't in plot3d/. Fixing this is the subject of this ticket. Doing it for plot.py largely is about GraphicsArray objects, so that is a separate ticket - #10144.

With the patches below, the rebuilt reference manual can be found at

http://mvngu.googlecode.com/hg/10143-plot/index.html

Apply:

  1. trac_10143-2dplotcoverage.patch
  2. trac-10143_reviewer.patch

Dependencies: #7981, #8632, #10244

Attachments (2)

trac-10143_reviewer.patch (1.7 KB) - added by mvngu 9 years ago.
trac_10143-2dplotcoverage.patch (12.5 KB) - added by kcrisman 9 years ago.
Based on 4.6.1

Download all attachments as: .zip

Change History (21)

comment:1 Changed 9 years ago by kcrisman

  • Cc jason mvngu added

Patch coming up soon (just checking that build docs still look correct).

comment:2 Changed 9 years ago by kcrisman

  • Authors set to Karl-Dieter Crisman
  • Status changed from new to needs_review

Okay, this adds the plot doc stuff, and also adds the z=0 option for a couple of .plot3d() methods. Needs review, should be fairly straightforward.

comment:3 Changed 9 years ago by kcrisman

  • Description modified (diff)

Changed 9 years ago by mvngu

comment:4 Changed 9 years ago by mvngu

  • Description modified (diff)
  • Reviewers set to Minh Van Nguyen

kcrisman's patch is fine by me. However, I noticed some misformatting and a broken link to the function show(). These are fixed in my reviewer patch. Someone needs to look over that one. I noticed that the function ensure_subs() is removed by kcrisman's patch. This is OK because that function is not used anywhere in the Sage library. Grepping through the whole Sage library only shows that ensure_subs() is defined and imported, but never used. See the ticket description for instructions on which tickets to apply.

comment:5 Changed 9 years ago by kcrisman

Thanks for fixing those - that's why we have reviews!

I thought I explained elsewhere on the ticket why I removed that function, but maybe I didn't? I came to the same conclusion you did; it was used in previous plot behavior, but no longer.

comment:6 Changed 9 years ago by mvngu

I gave kcrisman's patch a positive review. Someone just needs to review my patch for this ticket to be closed as fixed.

comment:7 Changed 9 years ago by gbe

  • Status changed from needs_review to positive_review

Minh's reviewer's patch looks good.

comment:8 Changed 9 years ago by jdemeyer

  • Milestone changed from sage-4.6.1 to sage-4.6.2

comment:9 follow-up: Changed 9 years ago by jdemeyer

  • Status changed from positive_review to needs_work

This needs to be rebased to apply on top of #9907.

comment:10 in reply to: ↑ 9 Changed 9 years ago by kcrisman

Replying to jdemeyer:

This needs to be rebased to apply on top of #9907.

Nuts! I'll try to get to this today. Hopefully it won't conflict with some of the other things going on in plotting...

comment:11 Changed 9 years ago by kcrisman

Okay, now need to be rebased also for #9781, #8236, and #10244.

Changed 9 years ago by kcrisman

Based on 4.6.1

comment:12 Changed 9 years ago by kcrisman

  • Status changed from needs_work to needs_review

The only rebase needed was in removing the ensure_subs function in misc.py. The reviewer patch still applies after this. There are no other changes, and I once again checked docs and tests, so I think positive review restoration is appropriate.

To buildbot: depends on #9781, #8236, and #10244 - apply trac_10143-2dplotcoverage.patch and trac-10143_reviewer.patch

comment:13 Changed 9 years ago by kcrisman

  • Reviewers changed from Minh Van Nguyen to Minh Van Nguyen, Geoffrey Ehrman
  • Status changed from needs_review to positive_review

comment:14 Changed 9 years ago by kcrisman

Just FYI - still applies fine on 4.6.2.alpha0.

comment:15 follow-ups: Changed 9 years ago by novoselt

The first of the current dependencies is closed, the second, I believe, is a mistake and should have been #8632, which is a prerequisite for #10244. So it seems that I can just say

Depends on #10244

comment:16 in reply to: ↑ 15 ; follow-up: Changed 9 years ago by jdemeyer

  • Description modified (diff)

Replying to novoselt:

The first of the current dependencies is closed, the second, I believe, is a mistake and should have been #8632, which is a prerequisite for #10244. So it seems that I can just say

Depends on #10244

Well, specifiying all dependencies is much more useful actually.

comment:17 in reply to: ↑ 16 Changed 9 years ago by novoselt

Replying to jdemeyer:

Well, specifiying all dependencies is much more useful actually.

I completely agree, but it seems to confuse the buildbot which tried to apply patches twice.

comment:18 in reply to: ↑ 15 Changed 9 years ago by kcrisman

The first of the current dependencies is closed, the second, I believe, is a mistake and should have been #8632, which is a prerequisite for #10244. So it seems that I can just say

Humble pie - BOTH the first two were typos. Sorry. The new description is correct.

Depends on #10244

I see what is going on. Useful for buildbot, but not at all useful for people manually applying patches who won't know how many iterations they have to go back! Thanks for the clarification.

comment:19 Changed 9 years ago by jdemeyer

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