#7889: revolution plot
-----------------------------------------+----------------------------------
Reporter: olazo | Owner: olazo
Type: enhancement | Status: needs_work
Priority: minor | Milestone: sage-4.3.3
Component: graphics | Keywords: revolution,plot
Author: Oscar Gerardo Lazo Arjona | Upstream: N/A
Reviewer: Karl-Dieter Crisman | Merged:
Work_issues: |
-----------------------------------------+----------------------------------
Changes (by kcrisman):
* status: needs_review => needs_work
Comment:
Just a few more small things; since this will be a nice new file and
functionality, we can afford to start off being very picky.
The optional parameters should be clearly marked as optional, as well as
the defaults - again, other docstrings should have good examples.
Please make sure the docstring lines are only a given length. For
whatever reason, we don't let them wrap around (except with error messages
or very long output, of course). I think 80 characters?
Don't need a double colon in line 38 since the examples come later.
Line 33 is confusing, and perhaps contradicts the opening statements of
usage (maybe I'm just confused on that, though):
{{{
- ``axis`` - A 2-tuple that specifies the position of the revolution axis
given that it is parallel to parallel_axis. If parallel_axis is 'z' then
axis the a point in which the revolution axis intersects the `x y` plane.
If parallel_axis is 'x' axis is a point in the `y z` plane. And if
parallel_axis is 'y' axis is a point in the `x z` plane.
}}}
In fact, one thing people often do is like
{{{
- ``axis`` - (default: 'z') Specifies position of...
- 'z': The axis is parallel to the `z` axis, intersecting the `x y`
plane at the specified point
- etc.
}}}
Notice the spaces between the options for readability. This might even be
required in the Sage developer guidelines, I'm not sure.
Lines 65 and 69 should be capitalized?
Look at the plot3d files to see what we decided the convention was for 3d
- I can't remember if it's "3D" or "3d" or "3-D" or ...
One should of course be able to input a 2 OR 3 tuple for phirange; I
didn't mean you should completely get rid of that option, since it's an
option in parametric_plot3d. But it still bothers me that we are
creating random new variables called 'phi' or 'fi'... the fix makes
sense, but what if phi meant something ''else'' in the current Sage
session? Maybe you can avoid this using lambda functions (check timings,
hopefully would be similar or better... look at the documentation for the
third way to call parametric_plot3d:
{{{
#. We draw a parametric surface using 3 Python functions (defined
using lambda):
::
sage: f = (lambda u,v: cos(u), lambda u,v: sin(u)+cos(v),
lambda u,v: sin(v))
sage: parametric_plot3d(f, (0, 2*pi), (-pi, pi))
}}}
So here we do not need to have a three-tuple for phirange, or indeed
trange. Of course, they now have to be Python or 'callable' functions.
Stylistically you could (optionally, obviously not necessary but does
improve readability)
{{{
from sage.plot.plot3d.parametric_plot3d import parametric_plot3d
...
curveplot = parametric_plot3d...
}}}
And feel free to disagree with any comments; after all, you wrote it! I
think these all make sense, though.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/7889#comment:9>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en.
