Opened 13 years ago
Closed 13 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: |
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)
Change History (15)
Changed 13 years ago by
Attachment: | trac_9066-shapes2-doc.patch added |
---|
comment:1 Changed 13 years ago by
Authors: | → Karl-Dieter Crisman |
---|---|
Status: | new → 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 13 years ago by
Attachment: | trac_9066-reviewer.patch added |
---|
comment:2 follow-up: 3 Changed 13 years ago by
Reviewers: | → 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 Changed 13 years ago by
- 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 13 years ago by
Status: | needs_review → positive_review |
---|
Docs look nice when built, passes tests. Let's get this in!
comment:5 Changed 13 years ago by
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: 7 Changed 13 years ago by
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 Changed 13 years ago by
Status: | positive_review → 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: 9 Changed 13 years ago by
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 Changed 13 years ago by
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.
comment:10 follow-up: 11 Changed 13 years ago by
Status: | needs_work → 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 Changed 13 years ago by
Status: | needs_review → 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 13 years ago by
Merged in: | → sage-4.5.2.alpha1 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
Merged all three patches in 4.5.2.alpha1.
Based on 4.4.2