Adding coordinate transformations to plot3d

[olazo]

While developing a command called transform_plot3d, that generalized ploting in diferent coordinate systems, Jason Grout suggested to me that such a command would be better implemented within plot3d.

I agreed to that, so I propose an adition to plot3d's syntax: "plot3d(function,var1range,var2range,transformation=None,kwds)" where transformation is a 4-tuple containing 3 functions of arity 3, and a variable which is to be interpreted as the function to be ploted. Like this (r*cos(fi),r*sin(fi),z,r), so the function will be ploted as r.

Examples can be found in  http://www.sagenb.org/home/pub/1328/.

I've just added a patch.

[wcauchois]

Hi! William directed me to your thread on sage-devel. Since I have experience working on 3D plotting, I think I could help integrate this code into Sage.

[wcauchois]

based on sage 4.3.1.alpha1

[wcauchois]

The patch I just attached represents my initial work on this issue. I added Oscar's code to plot3d() to implement the transformation keyword parameter, and I added the two new plotting functions spherical_plot3d and cylindrical_plot3d.

I made some minor changes to the original code. I moved the formulae for the transformations for spherical and cylindrical coordinates out of the body of plot3d, and into the bodies of spherical_plot3d and cylindrical_plot3d, since I thought that was a better place for them. I also added some error handling; the urange and vrange given to plot3d may be 2-tuples, in which case the variables are implicit -- but we expect urange[0] and vrange[0] to be the plot variables.

We need to add some documentation for the new features. I think I can draw upon the examples Oscar provided in his published worksheets to create doctests for the new functions. Oscar, could you help me come up with descriptions of cylindrical_plot3d and spherical_plot3d for use in their docstrings? And more examples are always helpful.

[olazo]

More examples now available at the bottom of:  http://sagenb.org/home/pub/1328/. I'll do the descriptions later.

[olazo]

I have attatced a proposal for a docstring in spherical_plot #7850 . Also, I checked your code. Do you think it is best to use "spherical_plot3d" instead of "spherical_plot"? I find the 3d redundant, and I like to type less. Also, I noticed that your implementation will not allow adaptative refinement to be used together with "transformation". I have never used that option and I don't know whether it is important. When my plots are not sufficiently rounded I use plot_points, and that usually does the trick.

[olazo]

I've just added a patch with some improvements as well as the docstrings. However, the docstrings of spherical_plot3d and cylindrical_plot3d are impropperly formated. I could not manage format them right. Please review!

[wcauchois]

I will be uploading a patch very soon with some improvements to Oscar's code.

[wcauchois]

based on sage 4.3.1.rc0

[wcauchois]

So here is what I've been working on: a very unambiguous system for specifying predefined transformations, that will work with symbolic expressions and Python callables alike.

It has turned out to be a lot of work.

Jason, could you take a look and tell me what you think of the code and the documentation I've added?

[jason]

I like how you incorporated olazo's work and your work. Hooray for collaborative development.

In the first reading through, it looks great to me. olazo: what do you think?

[olazo]

It seems fantastic to me! Many thanks to both of you!

[kcrisman]

I haven't been able to do a thorough review yet, but in general I agree that this looks very nice! Everything below should be interpreted as nitpicking; however, because of the last item (doctests) I must put this as 'needs work'.

In spherical_plot3d, it is a 'drop' of water, not a 'drom'.

In plot3d where the transformations are added, should it be

elif adaptive:

or

if adaptive:

That is, does transformation exclude adaptive? If so, this should be documented. Also, shouldn't

+    from sage.symbolic.callable import is_CallableSymbolicExpression

be done only if transformation is not None?

The @interact doctests don't really make sense in the command-line format. I tried them nonetheless, and it popped up a bunch of jmol windows - looked nice! But there must be a better way to doctest this; perhaps just A;B;C;D;E or something.

Also, why are Cylindrical and Spherical imported in plot3d/all.py? It's not clear to me why someone would want that available but not just use e.g. spherical_plot3d or just import Spherical if they really needed it?

Also, doctests are needed for the abstract classes, and (for readability) a blank line between methods wouldn't hurt.

[wcauchois]

Replying to kcrisman:

First of all, thanks for taking a look at this. Your comments are very thorough and I will respond to each of them in turn.

I haven't been able to do a thorough review yet, but in general I agree that this looks very nice! Everything below should be interpreted as nitpicking; however, because of the last item (doctests) I must put this as 'needs work'.

In spherical_plot3d, it is a 'drop' of water, not a 'drom'.

Yes, I think that's correct.

In plot3d where the transformations are added, should it be

elif adaptive:

or

if adaptive:

That is, does transformation exclude adaptive? If so, this should be documented. Also, shouldn't

I think transformation excludes adaptive since parametric_plot3d doesn't support adaptive plots. I'll make sure to document this.

+    from sage.symbolic.callable import is_CallableSymbolicExpression

be done only if transformation is not None?

Sure. I don't see how that really matters though, since its just an import statement.

The @interact doctests don't really make sense in the command-line format. I tried them nonetheless, and it popped up a bunch of jmol windows - looked nice! But there must be a better way to doctest this; perhaps just A;B;C;D;E or something.

How about show(A + B + C + D + E) in the TESTS section below?

Also, why are Cylindrical and Spherical imported in plot3d/all.py? It's not clear to me why someone would want that available but not just use e.g. spherical_plot3d or just import Spherical if they really needed it?

This is one of the major features of the transformation system. You can use spherical_plot3d, which will graph a function r in terms of phi and theta, or you can specify transformation=Spherical() which lets you choose the dependent and independent variables.

For example,

plot3d(..., transformation=Spherical('phi', ['r', 'theta']))

will graph a function phi in terms of r and theta. So basically you get more flexibility by using Spherical(). Is there a way I could make that clearer in the documentation?

Also, doctests are needed for the abstract classes, and (for readability) a blank line between methods wouldn't hurt.

Okay.

[wcauchois]

based on sage 4.3.1.rc0

[wcauchois]

based on sage 4.3.1

[wcauchois]

Please take a look at the changes in trac_7872_new-referee.patch. You can apply trac_7872_new-all.patch to a new sage branch to get all of the changes.

[kcrisman]

Also, why are Cylindrical and Spherical imported in plot3d/all.py? It's not clear to me why someone would want that available but not just use e.g. spherical_plot3d or just import Spherical if they really needed it?

This is one of the major features of the transformation system. You can use spherical_plot3d, which will graph a function r in terms of phi and theta, or you can specify transformation=Spherical() which lets you choose the dependent and independent variables.

For example,

plot3d(..., transformation=Spherical('phi', ['r', 'theta']))

will graph a function phi in terms of r and theta. So basically you get more flexibility by using Spherical(). Is there a way I could make that clearer in the documentation?

Most of the changes seem fine. I am still confused about this, though. Isn't

sage: spherical_plot3d(e^-y,(x,0,2*pi),(y,0,pi))

already allowing one to use a different name for the variables?

Ah, I think I see. There are six different possibilities for order of independent variables and the dependent variable, and you want to allow all of these - it that it? Maybe you should make it clearer that this is what is going on by saying that it's not the names, but rather which one is the dependent variable, you are changing - in fact, that the names look the same only to preserve the usual terminology - and then adding to

      sage: Spherical('theta', ['r', 'phi']) 
      Spherical coordinate system (theta in terms of r, phi) 

by actually plotting something with this.

Also, in line 623, I think it should be

   sage: var('r,u,v')

instead of r, u, u; I am surprised this doesn't give a doctest failure (I didn't get a chance to run that right now).

Is it ok if I make it still 'needs work' to just clarify these? For most users it will all be irrelevant, of course, but if we are really going to import Spherical() at the top level, then it should be very clear what the point is.

[wcauchois]

Replying to kcrisman:

Ah, I think I see. There are six different possibilities for order of independent variables and the dependent variable, and you want to allow all of these - it that it? Maybe you should make it clearer that this is what is going on by saying that it's not the names, but rather which one is the dependent variable, you are changing - in fact, that the names look the same only to preserve the usual terminology - and then adding to

      sage: Spherical('theta', ['r', 'phi']) 
      Spherical coordinate system (theta in terms of r, phi) 

by actually plotting something with this.

In the docstring on line 254, I do just that.

Also, in line 623, I think it should be

   sage: var('r,u,v')

instead of r, u, u; I am surprised this doesn't give a doctest failure (I didn't get a chance to run that right now).

Good catch!

Is it ok if I make it still 'needs work' to just clarify these? For most users it will all be irrelevant, of course, but if we are really going to import Spherical() at the top level, then it should be very clear what the point is.

I've done my best on the documentation, but it still might not be entirely clear on how to use Spherical and Cylindrical. Most users, of course, can simply use spherical_plot3d and cylindrical_plot3d. Can you make any concrete suggestions on how to improve the documentation? Does anyone have any ideas for sentences to add?

[jason]

In your definition of spherical coordinates, is your second angle the *elevation*, or the *inclination*. I think it might be the inclination, rather than what is stated in the docstring ("elevation"). Inclination is measured with 0 corresponding to the positive z-axis, and has a range of [0,pi]. Elevation is measured from the xy-plane, and has a range of [-pi/2,pi/2], if I understand things correctly.

[jason]

Also, given the confusion surrounding different conventions and variable names for spherical coordinates, I think it would be better to use "radius", "azimuth", and "inclination" (or "elevation"), instead of "r", "phi", and "theta". Then it's unambiguous what

Spherical('radius', ['azimuth', 'inclination'])

represents.

[jason]

Similarly, I think cylindrical coordinate components should be spelled out:

class Cylindrical(sage.plot.plot3d.plot3d._CoordTrans):
    all_vars = ['radius', 'azimuth', 'height']

    _name = 'Cylindrical (radius, azimuth, height) coordinate system'

    def gen_transform(self, radius=None, azimuth=None, height=None):
        return (radius * cos(azimuth),
                radius * sin(azimuth),
                height)

[jason]

One more point that I thought of when trying to use this feature. Why should I have to specify the names of variables more than once? Introspection should be able to get these from my gen_transform function. It seems like I should be able to do this:

class Cylindrical(sage.plot.plot3d.plot3d._CoordTrans):
    """
    Some docs for the mathematical definitions of the variables
    """
    def gen_transform(self, radius=None, azimuth=None, height=None):
        return (radius * cos(azimuth),
                radius * sin(azimuth),
                height)

and things should just work. all_vars can be inferred from the keyword arguments to gen_transform. It's pretty easy to come up with a good default _name (i.e., 'name of class (variables) coordinate system').

[jason]

Another polishing point: when given this syntax

spherical=(w*cos(u)*sin(v),w*sin(u)*sin(v),w*cos(v),w) 
plot3d(2,(u,-pi,pi),(v,0,pi),transformation=spherical,plot_points=[100,100])

why do I have to specify the fourth argument in spherical (i.e., "w")? In the plot, you know that I'm specifying u and v to be numeric ranges, so it should be easy for Sage to realize that the function variable is w and the independent variables are u and v.

[jason]

The current code might have a problem if I do:

spherical=(w*cos(u)*sin(v),w*sin(u)*sin(v),w*cos(v),w) 
plot3d(lambda x,y: 2,(-pi,pi),(0,pi),transformation=spherical,plot_points=[100,100])

because the current strategy does not specify if u or v is the first coordinate range.

[jason]

Indeed, with the patch:

sage: var('u,v,w')
sage: spherical=(w*cos(u)*sin(v),w*sin(u)*sin(v),w*cos(v),w) 
sage: plot3d(lambda x,y: 2,(-pi,pi),(0,pi),transformation=spherical,plot_points=[100,100])
Traceback (most recent call last):
...
ValueError: expected 3-tuple for urange and vrange

[jason]

apply on top of trac_7872_new-all.patch; based on 4.3.2.

[jason]

I've addressed every single one of my comments in the trac-7872-polish.patch. I think this is great functionality, and definitely ready for review!

[wcauchois]

Wow, great work Jason. Thank you so much. I've had a really tough quarter in school and I really didn't have time to work on this. I'm also excited to see this feature in Sage. Thanks to everyone who helped make this happen.

[jason]

Replying to wcauchois:

Wow, great work Jason. Thank you so much. I've had a really tough quarter in school and I really didn't have time to work on this. I'm also excited to see this feature in Sage. Thanks to everyone who helped make this happen.

I totally understand. Now I am talking about transformations in my calc 3 class, so I could take some time to finish this up. I'm hoping it gets in fairly quickly, though I'll probably apply the patch to our class server anyway.

I positive review your code (well, except for the changes I made :). Can you review my code?

[wcauchois]

Jason, I've taken a look at your code and you've done a great job of cleaning this up! I can't find any fault with it. Apply the following patches to Sage 4.3.3: trac-7872-polish.patch, trac_7872_new-all.patch. Applies fine and passes all doctests.

Now which one of us should mark this as a positive review, or should we bring in a 3rd party?

[jason]

Replying to wcauchois:

Jason, I've taken a look at your code and you've done a great job of cleaning this up! I can't find any fault with it. Apply the following patches to Sage 4.3.3: trac-7872-polish.patch, trac_7872_new-all.patch. Applies fine and passes all doctests.

Apply the patches in this order, though: trac_7872_new-all.patch, trac-7872-polish.patch.

There is precedent for two people collaboratively writing and reviewing each other's code as being okay. Since you've passed off on my code, and I've passed off on your code, I'll mark this as positive review. olazo: can you look at this as well, and if you find a problem, change it back to needs work?

[jason]

(actually, in this case, it was three people because olazo wrote the initial implementation!)

[olazo]

Replying to jason:

There is precedent for two people collaboratively writing and reviewing each other's code as being okay. Since you've passed off on my code, and I've passed off on your code, I'll mark this as positive review. olazo: can you look at this as well, and if you find a problem, change it back to needs work?

I can't find anything wrong with the code, it seems quite powerful already :) ! Thanks to you two for taking interest in this. I couldn't do any more recently because of school... It's been great so far though!

[mvngu]

Merged in this order:

1.  trac_7872_new-all.patch
2.  trac-7872-polish.patch