#7889: revolution plot
-----------------------------------------+----------------------------------
Reporter: olazo | Owner: olazo
Type: enhancement | Status: needs_work
Priority: minor | Milestone: sage-4.3.2
Component: graphics | Keywords: revolution,plot
Author: Oscar Gerardo Lazo Arjona | Upstream: N/A
Reviewer: Karl-Dieter Crisman | Merged:
Work_issues: |
-----------------------------------------+----------------------------------
Changes (by kcrisman):
* cc: jason (added)
* reviewer: => Karl-Dieter Crisman
* status: needs_review => needs_work
Comment:
Overall this patch is a great, needed addition to functionality. However,
it still needs some work, though nothing structural, as far as I can tell.
Trivial typos:
{{{
Return a plot of a revoved curve. # should be revolved
- ``axis`` - A 2-tuple that ... (paralel to the... # should be
parallel
}}}
and here also, where 'plotted' and 'length' are needed:
{{{
#this if-else provides a vector v to be ploted
#if curve is a tuple or a list of lenght 2, it is interpreted as a
parametric curve
#in the x-z plane.
#if it is of lenght 3 it is interpreted as a parametric curve in
3d space
}}}
Also in the docs, see plot/contour_plot.py and
plot/plot3d/parametric_plot3d.py for an idea of how long to make docstring
lines. For some reason we don't make them wrap around, but cut them off
at some number of characters.
I am uncomfortable with
{{{
def findvar(expr):#find the dependent variable of the curve
try:
vart=curve.args()[0]
except:
vart=var('t')
return vart
}}}
because even if there isn't a default choice, t could conceivably mean
something else, but var() injects it into the global namespace (I think?).
Also, there appear to be several places where
{{{
vart=findvar(curve[0])
}}}
but then is not used to make the curveplot.
I think that using isinstance is generally preferred, also. The timing
isn't really that important here, but I think it is more "Pythonic" and:
{{{
sage: %timeit str(type(curve)) == "<type 'tuple'>"
625 loops, best of 3: 869 ns per loop
sage: %timeit isinstance(curve,tuple)
625 loops, best of 3: 264 ns per loop
}}}
Just curious - why the xz-plane and not the xy-plane for the default
location of the axis of rotation? Obviously it doesn't "really" matter,
but at least in the US most texts start rotating from there, so if this is
intended for pedagogical purposes it could be confusing. I don't know
what Mathematica does, though, nor whether this is standard for industrial
or non-US uses of revolution plots.
Next,
{{{
from sage.symbolic.constants import pi
from sage.functions.other import sqrt
from sage.functions.trig import sin
from sage.functions.trig import cos
}}}
It looks like you probably need sin and cos to be symbolic, though I'm not
sure whether you need to import them. But I think your use of pi and sqrt
would be sufficient to import from the Python/C library math, since they
are only being used to compute actual numbers, not symbolics, correct?
Like this
{{{
from math import pi, sqrt
}}}
Please document phirange with examples - they are so cool! You should
probably also allow a tuple like (-pi,pi/2) in phirange, since there is
absolutely no ambiguity (check len(phirange)==2 or ==3 to do this) and
then people don't have to create a new variable
{{{
sage: var('phi')
sage: revolution_plot3d...(phi,-pi,pi/2)...
}}}
which would be quite annoying, as I just discovered when trying to get a
phirange other than 0 to 2*pi.
You may also want 'curve' to be able to be a Vector object (symbolic), but
I am not quite sure whether/how that is supported in such cases. Jason
will know, since he has done stuff with this.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/7889#comment:6>
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.
