Opened 12 years ago

Closed 12 years ago

#9066 closed enhancement (fixed)

Improve documentation in shapes2.py

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

Status badges

Description

plot.plot3d.shapes2.py was a mess, including having several weird helper functions that didn't really need doctests, such as

    def avg(a,b):
        return (a+b)/2.0

This cleans up the situation here.

Attachments (3)

trac_9066-shapes2-doc.patch (18.5 KB) - added by kcrisman 12 years ago.
Based on 4.4.2
trac_9066-reviewer.patch (11.0 KB) - added by mvngu 12 years ago.
trac_9066-doctest-fix.patch (1.5 KB) - added by kcrisman 12 years ago.
Apply this last.

Download all attachments as: .zip

Change History (15)

Changed 12 years ago by kcrisman

Based on 4.4.2

comment:1 Changed 12 years ago by kcrisman

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

This also improves the copyright situation, for which reason I have added robertwb and was to this, in case they have any emendations to it. I figured they were mostly responsible, though :)

Also, I'm not sure what purpose the ruler functions serve. They were never documented before, and do not show up anywhere else in the HG repository than in this file! So one could remove them if they were intended for something else that is now handled elsewhere; either way is fine.

Changed 12 years ago by mvngu

comment:2 follow-up: Changed 12 years ago by mvngu

  • Reviewers set to Minh Van Nguyen

I'm OK with kcrisman's patch, except for a few points which I have added in my reviewer patch. Changes in the reviewer patch include:

  • Use the new style of raising exceptions, e.g. use TypeError as if it's a function, not a statement. This new style is more consistent with Python 3.x. When it comes time to switch to using Python 3.x, there would be less work involved in making the transition to Python 3.x.
  • Put exception tests into a TESTS block.
  • Fix up docstring in bezier3d so it renders nicely in the reference manual.
  • Some miscellaneous typo fixes.

If my patch gets a positive review, the whole ticket is good to go.

comment:3 in reply to: ↑ 2 Changed 12 years ago by kcrisman

  • Use the new style of raising exceptions, e.g. use TypeError as if it's a function, not a statement. This new style is more consistent with Python 3.x. When it comes time to switch to using Python 3.x, there would be less work involved in making the transition to Python 3.x.

Okay, as long as this still works in Python 2.6 or whatever.

  • Put exception tests into a TESTS block.

Ok.

  • Fix up docstring in bezier3d so it renders nicely in the reference manual.

I'm a minimalist about this sort of thing, as you know :)

  • Some miscellaneous typo fixes.

So did we say 3-D and not 3D? I purposely put 3D because I thought that was the default... I never know.

comment:4 Changed 12 years ago by kcrisman

  • Status changed from needs_review to positive_review

Docs look nice when built, passes tests. Let's get this in!

comment:5 Changed 12 years ago by davidloeffler

This patch and #9088 seem to conflict -- if I apply both, I get a doctest failure:

sage -t  "devel/sage-reviewing/sage/plot/plot3d/shapes2.py"
**********************************************************************
File "/storage/masiao/sage-4.5.alpha1/devel/sage-reviewing/sage/plot/plot3d/shapes2.py", line 700:
    sage: P.tachyon_repr(P.default_render_params())
Expected:
    'Sphere center 1.0 2.0 3.0 Rad 0.015 texture84'
Got:
    'Sphere center 1.0 2.0 3.0 Rad 0.015 texture85'
**********************************************************************
File "/storage/masiao/sage-4.5.alpha1/devel/sage-reviewing/sage/plot/plot3d/shapes2.py", line 717:
    sage: P.obj_repr(P.default_render_params())[0][0:2]
Expected:
    ['g obj_1', 'usemtl texture86']
Got:
    ['g obj_1', 'usemtl texture87']
**********************************************************************
File "/storage/masiao/sage-4.5.alpha1/devel/sage-reviewing/sage/plot/plot3d/shapes2.py", line 825:
    sage: L.tachyon_repr(L.default_render_params())[0]
Expected:
    'FCylinder base 1.0 0.0 0.0 apex 0.999950000417 0.00999983333417 0.0001 rad 0.005 texture126'
Got:
    'FCylinder base 1.0 0.0 0.0 apex 0.999950000417 0.00999983333417 0.0001 rad 0.005 texture127'
**********************************************************************
3 items had failures:
   1 of   4 in __main__.example_13
   1 of   4 in __main__.example_14
   1 of   4 in __main__.example_19
***Test Failed*** 3 failures.

This doesn't come up if I apply either one of the patches on its own.

comment:6 follow-up: Changed 12 years ago by kcrisman

I'm not exactly sure how the texture numbers get decided, but I think there is some linear order involved with their names. Anyway, this is easy enough to fix. We should definitely apply this one first because it is much more annoying, and then it would be very simple to add a reviewer patch to #9088 to fix these trivialities.

comment:7 in reply to: ↑ 6 Changed 12 years ago by ddrake

  • Status changed from positive_review to needs_work

Replying to kcrisman:

We should definitely apply this one first because it is much more annoying, and then it would be very simple to add a reviewer patch to #9088 to fix these trivialities.

Oops, it's now too late for that. Applying these patches to 4.5.2.alpha0, I get the doctest errors mentioned above. We've already merged #9088, so we'll need to fix the problem here.

comment:8 follow-up: Changed 12 years ago by kcrisman

Unfortunately I am not a queueing expert, so in order to fix this I will have to add yet another doctest fix patch. Is that okay with the release managers?

comment:9 in reply to: ↑ 8 Changed 12 years ago by ddrake

Replying to kcrisman:

Unfortunately I am not a queueing expert, so in order to fix this I will have to add yet another doctest fix patch. Is that okay with the release managers?

That's no problem at all.

Changed 12 years ago by kcrisman

Apply this last.

comment:10 follow-up: Changed 12 years ago by kcrisman

  • Status changed from needs_work to needs_review

This should do it. Hopefully a release manager can very easily review this. Apply in order doc, reviewer, then doctest-fix.

comment:11 in reply to: ↑ 10 Changed 12 years ago by ddrake

  • Status changed from needs_review to positive_review

Replying to kcrisman:

This should do it. Hopefully a release manager can very easily review this. Apply in order doc, reviewer, then doctest-fix.

Looks good.

comment:12 Changed 12 years ago by ddrake

  • Merged in set to sage-4.5.2.alpha1
  • Resolution set to fixed
  • Status changed from positive_review to closed

Merged all three patches in 4.5.2.alpha1.

Note: See TracTickets for help on using tickets.