Opened 14 years ago

Closed 14 years ago

#5767 closed defect (fixed)

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

Reported by: Robert Bradshaw Owned by: Michael Abshoff
Priority: major Milestone: sage-4.0
Component: doctest coverage Keywords:
Cc: Jason Grout, William Cauchois Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description


Attachments (6)

5767-plot3d-base-doctests.patch (53.9 KB) - added by Robert Bradshaw 14 years ago.
5767-referee.patch (5.9 KB) - added by William Cauchois 14 years ago.
5767-plot3d-base-doctests2.patch (2.6 KB) - added by Robert Bradshaw 14 years ago.
apply after other two
5767-plot3d-base-doctests-whitespace.patch (46.0 KB) - added by Robert Bradshaw 14 years ago.
5767-referee2.patch (1.0 KB) - added by William Cauchois 14 years ago.
5767-all.patch (61.7 KB) - added by William Cauchois 14 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 14 years ago by Michael Abshoff

Milestone: sage-3.4.1sage-3.4.2

Bouncing to 3.4.2.

Cheers,

Michael

comment:2 Changed 14 years ago by Robert Bradshaw

Summary: Bring coverage of plot3d/base.pyx up to 100%[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 14 years ago by Robert Bradshaw

comment:3 Changed 14 years ago by Jason Grout

Cc: Jason Grout added

Changed 14 years ago by William Cauchois

Attachment: 5767-referee.patch added

comment:4 Changed 14 years ago by William Cauchois

Summary: [with patch; needs review] Bring coverage of plot3d/base.pyx up to 87%[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 14 years ago by William Cauchois

Cc: William Cauchois added

comment:6 in reply to:  4 Changed 14 years ago by Michael Abshoff

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 Changed 14 years ago by William Cauchois

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 14 years ago by Michael Abshoff

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 14 years ago by Robert Bradshaw

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 14 years ago by Robert Bradshaw

apply after other two

Changed 14 years ago by Robert Bradshaw

comment:10 in reply to:  4 Changed 14 years ago by Robert Bradshaw

Summary: [with patch; needs work] Bring coverage of plot3d/base.pyx up to 87%[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 14 years ago by William Cauchois

Attachment: 5767-referee2.patch added

comment:11 Changed 14 years ago by William Cauchois

Summary: [with patch; needs review] Bring coverage of plot3d/base.pyx up to 87%[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 Changed 14 years ago by Robert Bradshaw

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

Changed 14 years ago by William Cauchois

Attachment: 5767-all.patch added

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

comment:13 in reply to:  12 Changed 14 years ago by William Cauchois

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 14 years ago by Robert Bradshaw

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 14 years ago by Michael Abshoff

Resolution: fixed
Status: newclosed

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

Cheers,

Michael

Note: See TracTickets for help on using tickets.