Opened 11 years ago

Closed 11 years ago

#5767 closed defect (fixed)

[with patch; positive review] Bring coverage of plot3d/base.pyx up to 87%

Reported by: robertwb Owned by: mabshoff
Priority: major Milestone: sage-4.0
Component: doctest coverage Keywords:
Cc: jason, wcauchois Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description


Attachments (6)

5767-plot3d-base-doctests.patch (53.9 KB) - added by robertwb 11 years ago.
5767-referee.patch (5.9 KB) - added by wcauchois 11 years ago.
5767-plot3d-base-doctests2.patch (2.6 KB) - added by robertwb 11 years ago.
apply after other two
5767-plot3d-base-doctests-whitespace.patch (46.0 KB) - added by robertwb 11 years ago.
5767-referee2.patch (1.0 KB) - added by wcauchois 11 years ago.
5767-all.patch (61.7 KB) - added by wcauchois 11 years ago.
this patch incorporates ALL of the changes; apply to sage 3.4.2

Download all attachments as: .zip

Change History (21)

comment:1 Changed 11 years ago by mabshoff

  • Milestone changed from sage-3.4.1 to sage-3.4.2

Bouncing to 3.4.2.

Cheers,

Michael

comment:2 Changed 11 years ago by robertwb

  • Summary changed from Bring coverage of plot3d/base.pyx up to 100% to [with patch; needs review] Bring coverage of plot3d/base.pyx up to 87%

Before

SCORE sage/plot/plot3d/base.pyx: 5% (4 of 78)

After

SCORE sage/plot/plot3d/base.pyx: 87% (69 of 79)

I don't know when I'll have time to work on this again, so I think we should at least get these ones in.

Changed 11 years ago by robertwb

comment:3 Changed 11 years ago by jason

  • Cc jason added

Changed 11 years ago by wcauchois

comment:4 follow-ups: Changed 11 years ago by wcauchois

  • Summary changed from [with patch; needs review] Bring coverage of plot3d/base.pyx up to 87% to [with patch; needs work] Bring coverage of plot3d/base.pyx up to 87%

REFEREE REPORT

Excellent work Robert! This patch applies to Sage 3.4.1 and the doctests are all valid. There were a number of misspellings which I corrected in 5767-referee.patch (I'm sorry I accidentally attached it twice; use either one). Besides that, I have a few concerns:

  • It looks like there are some typos due to copy-and-pasing:
    • On line 446 the documentation for tachyon() says it returns "an x3d input file".
    • On line 1202 the documentation for obj_repr() says it returns "the x3d representation of a group".
    • On line 1230 the documentation for jmol_repr() says it returns "the x3d representation of a group".
  • The documentation for jmol_repr on line 657 is somewhat confusing for me, especially when you say that jmol uses the strings to "construct self". Could you replace that with something like "construct a 3D mesh representing this object"? The same concern applies to the documentation for tachyon_repr and obj_repr.
  • Do you think it would improve the readability of the documentation to replace "self" with "`self!`" -- that is, to apply preformatting to it?
  • What's up with the trailing spaces on every line?
  • On line 730 you use the word "preable". Is this a typo?

comment:5 Changed 11 years ago by wcauchois

  • Cc wcauchois added

comment:6 in reply to: ↑ 4 Changed 11 years ago by mabshoff

Replying to wcauchois:

REFEREE REPORT

Excellent work Robert! This patch applies to Sage 3.4.1 <SNIP>

Hi Bill, you should really always apply against the latest development release. 3.4.1->3.4.2.rc0 was not a large release, but in many other cases there is a rather large, i.e. non-zero chance the patch would either not apply any more or be broken by other changes. There is always a sage.math binary of the latest release development snapshot, so you can use that. Another alternative is to have a review version of Sage that you upgrade from development snapshot to development snapshot.

Cheers,

Michael

comment:7 follow-up: Changed 11 years ago by wcauchois

OK, I will do that. I'm sorry for the inconvenience I've caused you! The thing is, I'm building 3.4.2.rc0 right now and its taking a while -- so I figured I'd do this review and then once we addressed the issues we could handle rebasing. But I understand the importance of ensuring compatibility with the latest release.

comment:8 in reply to: ↑ 7 Changed 11 years ago by mabshoff

Replying to wcauchois:

OK, I will do that. I'm sorry for the inconvenience I've caused you!

I have no clue if this is actually a problem, I just wanted to point out how to avoid problems since this has been an issue with your reviews in the past ;)

The thing is, I'm building 3.4.2.rc0 right now and its taking a while -- so I figured I'd do this review and then once we addressed the issues we could handle rebasing. But I understand the importance of ensuring compatibility with the latest release.

Cool.

Cheers,

Michael

comment:9 Changed 11 years ago by robertwb

Wow, looks like I wasn't able to spell that day... thanks for looking at this, I'll address the issues you mentioned and post a patch shortly.

Changed 11 years ago by robertwb

apply after other two

Changed 11 years ago by robertwb

comment:10 in reply to: ↑ 4 Changed 11 years ago by robertwb

  • Summary changed from [with patch; needs work] Bring coverage of plot3d/base.pyx up to 87% to [with patch; needs review] Bring coverage of plot3d/base.pyx up to 87%

Thanks for looking into this.

Replying to wcauchois:

REFEREE REPORT

Excellent work Robert! This patch applies to Sage 3.4.1 and the doctests are all valid. There were a number of misspellings which I corrected in 5767-referee.patch (I'm sorry I accidentally attached it twice; use either one). Besides that, I have a few concerns:

  • It looks like there are some typos due to copy-and-pasing:
    • On line 446 the documentation for tachyon() says it returns "an x3d input file".
    • On line 1202 the documentation for obj_repr() says it returns "the x3d representation of a group".
    • On line 1230 the documentation for jmol_repr() says it returns "the x3d representation of a group".

Fixed.

  • The documentation for jmol_repr on line 657 is somewhat confusing for me, especially when you say that jmol uses the strings to "construct self". Could you replace that with something like "construct a 3D mesh representing this object"? The same concern applies to the documentation for tachyon_repr and obj_repr.

I clarified it.

  • Do you think it would improve the readability of the documentation to replace "self" with "`self!`" -- that is, to apply preformatting to it?

I'm not sure it's worth it.

  • What's up with the trailing spaces on every line?

Sorry, I attached another patch that removes this (but I'm not sure if it'll apply cleanly, if not it's probably not worth it).

  • On line 730 you use the word "preable". Is this a typo?

Yep, I meant preamble. Fixed.

Changed 11 years ago by wcauchois

comment:11 Changed 11 years ago by wcauchois

  • Summary changed from [with patch; needs review] Bring coverage of plot3d/base.pyx up to 87% to [with patch; positive review] Bring coverage of plot3d/base.pyx up to 87%

REFEREE REPORT:

Looking over the code again, I noticed a few other instances where "concatenate" was spelled incorrectly. I fixed these in 5767-refree2.patch. With this and Robert's changes, positive review.

(By the way Robert: Mercurial queues is awesome! I made 5767-all.patch with qfold :).)

comment:12 follow-up: Changed 11 years ago by robertwb

Note that your 5767-all.patch lost all authorship information...

Changed 11 years ago by wcauchois

this patch incorporates ALL of the changes; apply to sage 3.4.2

comment:13 in reply to: ↑ 12 Changed 11 years ago by wcauchois

Replying to robertwb:

Note that your 5767-all.patch lost all authorship information...

Oh shoot! OK, I fixed it manually and it works. I think that MQ unfortunately does not preserve the metadata usually associated with a changeset.

comment:14 Changed 11 years ago by robertwb

I'm pretty sure queues can be used to preserve metadata using qfold, but that's fine. Thanks for looking at this.

  • Robert

comment:15 Changed 11 years ago by mabshoff

  • Resolution set to fixed
  • Status changed from new to closed

Merged 5767-all.patch in Sage 4.0.alpha0.

Cheers,

Michael

Note: See TracTickets for help on using tickets.