Parametrization of (metric) surfaces in 3D euclidean space

[4756944c-1c3a-4a3c-ad39-9cccb4f357db]

This patch aims to implement a class for embedded surfaces in the three dimensional euclidean space described by a parametrization. Its focus on the computation of metric invariants (first and second fundamental forms, Gaussian curvature, ...) and more involved geometry (geodesics, ...).

Apply attachment: trac_10132_revised_sage-5.13.beta2.patch

Depends on #11549 Depends on #12737

CC: @jvkersch @sagetrac-mhampton @nilesjohnson @jasongrout @novoselt @sagetrac-maldun @qed777 @videlec

Component: geometry

Keywords: differential geometry, parametrized surface

Author: Mikhail Malakhaltsev, Joris Vankerschaver

Reviewer: Vincent Delecroix

Merged: sage-5.13.beta5

Issue created by migration from https://trac.sagemath.org/ticket/10132

[4756944c-1c3a-4a3c-ad39-9cccb4f357db]

Attachment: mikarm_ticket_files.zip

parametrized_curve3d class and example worksheets

[jasongrout]

comment:1

CCing some people that probably are interested in these developments (see #9650)

It would be good to post a message to sage-devel to coordinate efforts (pun intended! :)

[novoselt]

comment:2

I think it would be great to have more differential geometry in Sage! It may be nice also to have some unification of differential and algebraic geometry, so that it is easy to switch from working with a smooth variety to working with a manifold and back.

[jvkersch]

comment:3

I've been playing with the worksheet for a little while now and it looks really good. My own interest in using Sage for differential geometry is more in the area of symbolic computations (if I never have to manipulate a Riemann tensor ever again I will be a happy man), but I think curves and surfaces should be developed too and would form an important selling point for Sage.

Some comments:

• From personal experience, it turns out to be a bit of work to "sagify" a python class (make sure that it coerces properly, doctests pass, etc.) I could help with that.

• There is a differential forms class at #9650 that is being merged into Sage. I see that the representation of tensors in your class is very similar to the internal representation of forms. As a long term goal (i.e. not for this ticket) it would be good to have interoperability with this class and (future) classes for Riemannian geometry in Sage.

[4756944c-1c3a-4a3c-ad39-9cccb4f357db]

comment:4

jason,jverksch, novoselt, thank you very much for your support and information on the differential form class.

Of course, it would be very good to coordinate efforts in the development of differential geometry in SAGE. So, according to your advice, I will post a message concerning this topic in the SAGE-devel.

to jvkersch. I would highly appreciate it if you could explain me how to ``sagify'' a python class, it is really my weak point.

The interoperability between future classes in the differential geometry is one of the most important point in development. Therefore, it would be good to have like a coordination center of the differential geometric part of SAGE, but I do not know how to organize it. Any ideas?

Also, in my point of view, the natural way to work on differential geometry in SAGE is to start from elementary differential geometry (curves and surfaces), then Riemannian geometry, then connections in bundles, etc.
Though may be it is a long way, what do you think?

[novoselt]

comment:5

Replying to @sagetrac-mikarm:

The interoperability between future classes in the differential geometry is one of the most important point in development. Therefore, it would be good to have like a coordination center of the differential geometric part of SAGE, but I do not know how to organize it. Any ideas?

There are some wiki pages maintained, e.g. for graph development or http://wiki.sagemath.org/combinat

Also, in my point of view, the natural way to work on differential geometry in SAGE is to start from elementary differential geometry (curves and surfaces), then Riemannian geometry, then connections in bundles, etc.
Though may be it is a long way, what do you think?

It may be useful to consider creating some classes for "generic manifolds" and then specialize them to curves and surfaces. This can be useful to ensure uniform interface in the future and have a possibility right from the start to put "generic methods" into appropriate places. (I am not sure which ones, but like is_compact or maybe chart access can be made general.)

[jvkersch]

comment:6

I had a great time playing with this code during the weekend and I am really looking forward to the time where I'll be able to take a book on curves and surfaces and work through the examples using Sage. I didn't really look at the mathematics or the usability (both seem fine for now), but I went ahead and made this into a Sage class (following the instructions at this page). Please see the attached patch for details. What it does is the following:

1. Create a new directory riemann, containing the parametrized surface class that Mikhail uploaded in the zip file, together with the vector functions class.

2. Updates setup.py and all.py to make Sage aware of these additions.

I deliberately did not want to change any code if I could help it, but here are the modifications that I made:

1. Changed the capitalization to ParametrizedSurface3D to conform to the Sage standard;
2. Removed the references to RR and CC since these already have standard meanings in Sage;
3. I added a class GenericManifold as per the suggestions of Andrey. I think this is a good idea (even if we don't implement anything generic now), as it will keep us focused on the need to tie this in with more general differential geometric classes.
4. Throughout the code, I added various import statements where it was necessary. I ended up changing some of the constructions in the functions principal_curvatures, geodesics_numerical and parallel_translation_numerical as these relied on some constructions that weren't pure Python. As you will see, the code that I added is quite hideous and just serves to make things work -- definitely needs to be streamlined.

With these changes, my version of Sage (4.6.alpha3 without any other patches) built cleanly and I was able to replicate all of the constructions in the Sage worksheet. So hopefully the uploaded patch can provide a good starting point for further work.

Here are a few remarks that I came across while tweaking the code.

1. Sometimes the terminology "vector" is used where "vector field" would be more appropriate.
2. It seems as if this class would benefit from having a small template for a generic tensor class. Right now, many of the member functions return tensors as lists (which is fine), but there are also member functions which take an optional argument to return only one component of that tensor. This code then needs to check whether the optional argument is valid, adding some complexity to the code. Maybe we could define a simple tensor class which just has __getitem__ and __setitem__ and then let this class deal with components, input checking, etc.
3. I think that some of the functions in the vector_functions module have Sage alternatives, which should be used instead.
4. We need to think on providing an interface to parametric_plot3d, so that these kind of surfaces can easily be plotted.

As Mikhail remarked in his original post, the code hasn't been optimized for speed. I thought this could be addressed by the following (could be a longer term goal for this patch).

1. Caching the result of computing the fundamental forms, the Christoffel symbols, etc. This would make subsequent calls to the components of these tensors much faster, at the expense of some memory.
2. The numerical routines should be optimized for float operations using fast_float or similar.

This is just my 2 cents. I'm looking forward to hearing your comments and I would definitely like to help in implementing some of these (or other) suggestions.

[jvkersch]

Code of Mikhail as a Sage patch

[4756944c-1c3a-4a3c-ad39-9cccb4f357db]

Description changed:

--- 
+++ 
@@ -1,20 +1 @@
-I am working in the field of differential geometry (research and teaching) and use SAGE for two or three years in the research work and in the teaching as well. 

-SAGE includes rather developed methods in algebra, number theory, algebraic geometry, etc. but lacks the differential geometry. Therefore I would like to contribute to the SAGE development in this direction.
-
-To start with, I wrote a class 'parametrized_surface3d', and made a worksheet "Ellipsoid via SAGE" which demonstrates the methods of the class in work.  I attached the corresponding files to this ticket.
-Note that, at this moment, the class is not optimized from the programing point of view (calculation time, catching exeptions, etc), but somehow it works.
-
-I plan to write also the similar classes for curves on the plane and in the 3-dimensional space, as well as a class for a surface given implicitly. The further plans are Riemannian geometry, etc, though the life is short :) so I do not know if I will be able to complete this program. Anyway, I hope I will write a text-book "Elementary differential geometry via SAGE" which in some sense will be similar to A.Gray's book "Differential geometry via Mathematica".
-
-So I would be thankful if I could get answers on the following questions:
-
-1) What do you think, is it important to include the differential geometric stuff to SAGE? 
-
-2) Are there other people who work in the same direction? It would be good if I could cooperate with them.
-
-3) What recommendations can you give concerning the code of the class for I could take into account in my future work.
-
-  
-
-

[4756944c-1c3a-4a3c-ad39-9cccb4f357db]

comment:7

Attachment: trac_10132_differential_geometry_sage.patch.gz

to Joris

The work you did is really great. Thank you also for the reference to the page with instructions. I have just come back to Bogota, so I could not look in details in the code improvements you did. I will carefully study them and then comment in details. Anyway, it is incredible help.

Now concerning the remarks:

Sometimes the terminology "vector" is used where "vector field" would be more appropriate.

Yes, sure, we will change this.

It seems as if this class would benefit from having a small template for a generic tensor class. Right now, many of the member functions return tensors as lists (which is fine), but there are also member functions which take an optional argument to return only one component of that tensor. This code then needs to check whether the optional argument is valid, adding some complexity to the code. Maybe we could define a simple tensor class which just has __getitem__ and __setitem__ and then let this class deal with components, input checking, etc.

Also agree.

I think that some of the functions in the vector_functions module have Sage alternatives, which should be used instead.

It is interesting, I could not find these alternatives, maybe because I do not know Sage well enough. If such alternatives exist, surely we need to use them.

We need to think on providing an interface to parametric_plot3d, so that these kind of surfaces can easily be plotted.

I also had this idea, for example for plotting vector fields along the surface (see examples in the worksheet). However, I decided to use Unix way: small programs from which the user organizes something like a pipe to get the result. The reason is that it is easier to tune small stones, than to handle big rocks :) May be I am wrong, we should think about this possibility: an interface to plotting routines.

Caching the result of computing the fundamental forms, the Christoffel symbols, etc. This would make subsequent calls to the components of these tensors much faster, at the expense of some memory.

Sometimes, it is needed to find one Christoffel symbol, or a coordinate of a metric tensor (we should take into account the fact that calculations in differential geometry may be very very long). So, from my point of view, it possible to use two approaches (as it was done in Maxima). If a user needs all the information on the surface in order to solve some problem, the class calculates everything (first and second fundamental forms, curvatures, etc) and caches it. But also user may say that he needs only one thing, then the class returns exactly the required object.

The numerical routines should be optimized for float operations using fast_float or similar.

Completely agree, we should do it.

To Andrei: Wiki, it is a good idea, thank you! I will look how to do it, and surely this is the decision.

It may be useful to consider creating some classes for "generic manifolds" and then specialize them to curves and surfaces. This can be useful to ensure uniform interface in the future and have a possibility right from the start to put "generic methods" into appropriate places.

Certainly, there are such methods, for example, find Christoffel symbols of a metric, find a geodesic, etc. which can be included in the more general class of Riemannian manifold.

I submitted a request to join the group Sage-devel, but there is no response up to now. When I manage to join, I will post the information in this group as well.

[jvkersch]

comment:10

I've been a little busy over the last few days, but I made some changes to the patch which I wanted to show you. The new patch is attached to this comment.

The most significant is that I think that the member functions such as natural_frame, first_fundamental_form_coefficients, second_order_natural_frame, etc. should always just return a dictionary/list with all of the components. The current situation is that these functions take an optional argument, and when that argument is specified only that component is returned.

1. By returning a dictionary/list we avoid having to check whether the index is in the appropriate range.
2. We also avoid code duplication (now there is often the same code, once to compute all the coefficients at once and once to compute individual components);
3. It is slightly counter intuitive to have a function (say) natural_frame return a vector rather than a basis. If the user is only interested in one component, they can still write

  sage: surface = ParametrizedCurve3D(...)
  sage: f = surface.natural_frame()
  sage: print f[0]

1. For the functions like natural_frame that return a frame, it would be more in line with current Sage code (see for instance the basis member function for vector spaces) to return a full frame as a list.

However, as I'm proposing a fairly large change, I wanted to ask your opinion before going ahead. Just to see what would happen, though, I made the changes listed above and it doesn't break anything -- the code as-is doesn't rely on computing the individual components of all those tensors.

[jvkersch]

Worksheet to illustrate caching for the computation of geodesics

[jvkersch]

comment:11

Attachment: Parametrized Curve_ Computing Geodesics.sws.gz

Replying to @jvkersch:

However, as I'm proposing a fairly large change, I wanted to ask your opinion before going ahead. Just to see what would happen, though, I made the changes listed above and it doesn't break anything -- the code as-is doesn't rely on computing the individual components of all those tensors.

I needed to add that as an illustration of what I wrote I've updated the functions natural_frame and first_fundamental_form_coefficients, but I've left the other functions unchanged.

[jvkersch]

comment:12

To see whether caching would speed up things, I've also added a method geodesics_numerical_fast, which optimizes the numerical calculation of geodesics. This is a bit faster than the previous implementation, but the speedup comes when you repeatedly call this function. Subsequent function calls are very fast. I've also uploaded a worksheet to illustrate this.

[4756944c-1c3a-4a3c-ad39-9cccb4f357db]

comment:13

Joris,

I really appreciate that you participate actively in the development of the package. Thank you very much, I am sure that together we will go much faster and efficiently. These days I also had a lot of work, but hope at the weekend I will have time to try new version of the package you submitted.

One thing I should say concerning your idea: "The most significant is that I think that the member functions such as natural_frame, first_fundamental_form_coefficients, second_order_natural_frame, etc. should always just return a dictionary/list with all of the components. The current situation is that these functions take an optional argument, and when that argument is specified only that component is returned." Certainly I agree that this change simplifies code and goes "along the Sage line". However, my experience in calculations in differential geometry (by hand and using a computer programs), is that often the calculations are really hard (imagine that we deal with a surface whose equations are given by special functions or something else of this type rather than elementary surfaces like the ellipsoid, the catenoid, etc. In addition, in my opinion, computer calculations are to be combined with some theoretical arguments, calculation by hand, etc. So, it is very real the situation when the user needs to calculate only one component of a tensor and does not need to spend time on the calculation of the whole thing. At the same time, another user may wish to have the complete information, because he needs it for some reasons. The way out of this contradiction is, I think, to have two algorithms within one package. One is to calculate everything and to cash it, and then use. Another is to calculate one thing on user's request (then in order to find other, e.g. components of a tensor, he will spend more time than if he would use the first algorithm, but it is his choice :).

Maybe you have other solutions which are more efficient than this one.

Anyway, at the weekend I plan to play with the improved version of the package you submitted here, and will comment on it.

Again thank you, it is more interesting to work together!

Mikhail

[jvkersch]

comment:14

Hi Mikhail,

Having read your reply, I agree with your point of view. Certainly it would make the package more flexible to have a method to return individual components, and one to return all components of the tensor at once. I was too hasty in talking about the "Sage way".

It turns out there is a very easy way to cache the results of these functions, by preceeding them with the decorator @cached_method. I have done a few preliminary tests, and it looks both easy to implement and powerful. For most of the methods, this is the only modification that needs to be made.

I have made a few simplifications to what I implemented previously, and I will try to upload a new patch during the weekend, where I just add the caching and exceptions. I will leave in the original methods so that we can compare both approaches.

I just have one practical question for now: did you have a specific reason for referring to the components of vectors and tensors by means of indices starting from 1 instead of 0? It does not matter much for tensors (since they are a dictionary anyway), but for vectors it would be easier to start indexing from 0 instead of 1.

[jvkersch]

comment:15

I've uploaded a new version of the patch, together with Mikhail's original worksheet. The main changes are in the functions that compute invariants, such as natural_frame, first_fundamental_form, etc.

I thought it would be better to have, for each set of invariants, three member functions with specific aims. To compute the first fundamental form (for instance) the hard work is done in a cached member function _compute_first_fundamental_form_coefficient which computes one component.

The interaction with the user is done through two other member functions: first, there is first_fundamental_form_coefficient_new which does some sanity checking on the index of the component to compute, raises an exception if the index is out of bounds, and calls _compute_... to do the work. Secondly there is also a member function first_fundamental_form_coefficients_new which creates a dictionary with all the components.

I think that this "division of labor" makes it easier to optimize the result, make sure that the arguments are valid, and above all avoids code duplication. At the same time, I did not want to overhaul the design of the entire code, so I've replaced only a few functions in this vein. If you agree that this is a good design, we can tackle the remaining functions.

For the sake of comparison, I've labeled my additions with the suffix _new and I've left the original implementations in the file as well. In the worksheet, I've added some timing commands to see the difference in timing between the original and new implementations.

[jvkersch]

New version of the patch

[jvkersch]

Attachment: trac_10132_differential_geometry_sage_v2.patch.gz

[4756944c-1c3a-4a3c-ad39-9cccb4f357db]

comment:16

Attachment: ParametrizedSurface3D_ Examples.sws.gz

Joris,

I carefully read the last version of the class you uploaded (the new version of patch_v2), and insert my comments. I uploaded this file, there are no changes in the code, only comments. I looked how it works, and see that the changes you did improved the class much, especially the speed of calculation. And, of course, the code you wrote is much more professional than mine, so I can learn from it how to do such things. In part, I find very interesting how you deal the with calculation of first_fundamental_form, etc, and think that it is the best way to calculate the components of tensors, connection coefficients, and other objects of this type.

I think that now we can do the following things: 1) apply cache_method throughout the class everywhere we can, in part, for calculating the connection coefficients. 2) rewrite the parallel transport method as you did for geodesics 3) improve the method for finding principal curvatures 4) test the class on a wider variety of surfaces

And after that I suppose we can submit the class to Sage.

[4756944c-1c3a-4a3c-ad39-9cccb4f357db]

version 2 of parametrized_surface3d.py with comments

[jvkersch]

comment:17

Attachment: parametrized_surface3d.py.gz

Hi Mikhail,

Thanks for your comments. I agree with the outline of the necessary work, and I've gone ahead removing the old version of the methods and checking whether the code still works with Sage 4.6 (it does). I've also made a start on caching the remaining methods (I worked on natural_frame) and the numerical treatment of parallel transport.

With parallel transport, I noticed something weird: as a test, I asked the code to do parallel transport along a great circle on the sphere, and I found that the numerical equations blow up! I can't see any theoretical reason why this should be a difficult numerical problem, and the code seems fine too. So I'm a bit puzzled...

As for the remaining issues, I don't know how to deal with the assumption in the computation of the mean curvature, but I'll look at it again during the weekend.

I agree that it would be good to deal with a greater variety of surfaces. I wanted to implement Costa's surface (just as a preliminary test), but I didn't know how to deal with the Weierstrass p-functions in the parametric representation...

[4756944c-1c3a-4a3c-ad39-9cccb4f357db]

comment:18

Attachment: trac_10132_differential_geometry_sage_v3.patch.gz

Hi Joris,

The "blow up" was caused apparently by a small mistake in _create_pt_ode_system, see the comments in the parametric_surface3d.py. Also this problem appears if the curve goes out the coordinate system (see the worksheet parallel_translation).

Now I will try to do something on calculation of the principal curvatures. Also, maybe I will manage to do the example of the surface you wrote about.

Mikhail

[4756944c-1c3a-4a3c-ad39-9cccb4f357db]

parametrized_surface3d.py with small changes in parallel_translation_numeric_new

[4756944c-1c3a-4a3c-ad39-9cccb4f357db]

Attachment: parametrized_surface3d.2.py.gz

Attachment: parallel_translation.sws.gz

an example of usage of the method parallel_translation_numerical_new

[jvkersch]

comment:19

Yes! You are quite right about the bug (which I must have introduced while making the code into a patch the first time). I was worried about the fact that parallel transport did not seem to preserve lengths, but then I falsely convinced myself that was due to the fact that maybe the curves used weren't parametrized by arc length. I'm quite glad to see that this is solved now.

The problem with the code that I wrote is the following: when writing

        C_1 = self.connection_coefficients()
        for coef in C_1:
            C_1[coef] = C_1[coef].subs({u1: curve[0], u2: curve[1]})

C_1 will just be a reference to the connection coefficients dictionary, and not a genuine copy. So when we change it on the next lines, we are in fact changing the global dictionary of connection coefficients. Changing the first line to

        C_1 = self.connection_coefficients().copy()

also fixes the problem.

I have deleted the old version of the code and uploaded a new version of the patch. Apart from a few changes here and there, I think the code is in great shape! If I have time this week, I will work on writing some doc tests for the new methods.

I noticed from the worksheet that your version of the code is in a .py file in your home directory -- are you familiar with applying patches through the hg version control system? I'm just asking since this would be a good way of coordinating our work if we work on different areas in the file.

[jasongrout]

comment:20

Attachment: trac_10132_differential_geometry_sage_v3.2.patch.gz

Replying to @jvkersch:

Yes! You are quite right about the bug (which I must have introduced while making the code into a patch the first time). I was worried about the fact that parallel transport did not seem to preserve lengths, but then I falsely convinced myself that was due to the fact that maybe the curves used weren't parametrized by arc length. I'm quite glad to see that this is solved now.

The problem with the code that I wrote is the following: when writing

        C_1 = self.connection_coefficients()
        for coef in C_1:
            C_1[coef] = C_1[coef].subs({u1: curve[0], u2: curve[1]})

C_1 will just be a reference to the connection coefficients dictionary, and not a genuine copy. So when we change it on the next lines, we are in fact changing the global dictionary of connection coefficients.

Are you sure? In the code (the latest patch), it appears that the connection_coefficients return value is constructed as a new dictionary each time the function is called.

[jvkersch]

comment:21

Replying to @jasongrout:

Are you sure? In the code (the latest patch), it appears that the connection_coefficients return value is constructed as a new dictionary each time the function is called.

Hi Jason,

I think the problem is that when the method is cached, changing the return value will change the cached copy, and hence when the method is called again, this modified copy will be returned, e.g,

from sage.misc.cachefunc import cached_method

class A:
    @cached_method
    def get_dict(self):
        return {1: 'a', 2: 'b', 3: 'c'}

a = A()
d = a.get_dict()

print d 
d[3] = 'something else'
print a.get_dict()

prints

{1: 'a', 2: 'b', 3: 'c'}
{1: 'a', 2: 'b', 3: 'something else'}

instead of two copies of the same.

I don't really get why this would cause the problems in _create_pt_ode_system, but addressing it does seem to solve the problem...

[4756944c-1c3a-4a3c-ad39-9cccb4f357db]

comment:22

Hi Joris,

I played a little this weekend with the class and find that it works sufficiently stable and fast (see the worksheet I upload). Also I added two methods: shape_operator_coefficients and shape operator: sometimes it is needed :) So I think that now it generally is ready for usage by other people. Now we need to clean the file from commentaries and change the documentation strings so that they correspond to the methods in their present state. What do you think: have we almost done the job with parametrized surfaces?

I noticed from the worksheet that your version of the code is in a .py file in your home directory -- are you familiar with applying patches through the hg version control system? I'm > just asking since this would be a good way of coordinating our work if we work on different areas in the file.

Yes, you are right, it is so. Unfortunately, I am not familiar with this VCS. I have to learn many things in python, and in CVS as well. Hope I will manage to do it in the process of our work :) For example, from you I learned about cached methods...

Mikhail

[4756944c-1c3a-4a3c-ad39-9cccb4f357db]

Attachment: parametrized_surface3d.3.py.gz

Next version of ParametrizedSurface3D class

[4756944c-1c3a-4a3c-ad39-9cccb4f357db]

Examples of calculations for minimal surfaces and some calculations for the surfaces of revolution in general form

[jvkersch]

comment:23

Attachment: Minimal_surfaces_and_surface_of_revolution.sws.gz

Hi Mikhail,

My apologies for the long delay. I agree that the class is in pretty good shape. In the meantime, I've gone ahead and added two new methods: _latex_ and _view_, which print a latex and a string representation of the surface, respectively. Feel free to modify the output of these methods, if they are not accurate. I've also gone ahead and updated some of the doctests. In the new version of a patch I've indicated the point up to where the doctests are working fine.

I'm a very big fan of your idea of putting latex formulas in the doctests -- in this way, going over the implementation proved to be a good reminder of differential geometry. The HTML generated by these doctests will look pretty nice, and will form a nice addition to the book idea that you described on sage-devel.

As for Mercurial, it is really not hard to learn, and for this class it is necessary, since the patches that I've been uploading also modify some core Sage files (among other things to register the class so that it is available when Sage starts up). Moreover, the doctests might fail if you just copy the parametrized_surface3d.py file. I just follow the instructions on http://www.sagemath.org/doc/developer/walk_through.html#modifying-sage-source-code to set up Mercurial and to import and generate patches, and I didn't learn any of the more advanced stuff.

J.

[jvkersch]

comment:24

I've updated the file with some more working doctests, and I've added it to the documentation index, so that HTML docs can be created.

[5d2aaf09-c963-473a-bf79-1f742a72700f]

comment:25

So if I want to check this out, do I just apply the patch:

trac_10132_diff_geom_sage_doctests.patch

?

Or are there other required pieces?

Thanks, Marshall

[jvkersch]

comment:26

Replying to @sagetrac-mhampton:

So if I want to check this out, do I just apply the patch:

trac_10132_diff_geom_sage_doctests.patch

Hi Marshall,

Yes, this is the right version. Keep in mind that half of the doctests still aren't updated though (but the functionality is there).

J.

[jvkersch]

comment:27

The version of the patch which I've uploaded passes all doctests (./sage -t devel/sage/sage/riemann/parametrized_surface3d.py) and is at 100% coverage. I've also added a method plot3d which allows for straightforward plotting of the surface.

While going over the code, I had the following questions:

1. Doctesting takes about 1 min. on my Macbook. Is this acceptable, or too long?
2. Now that we have a method to compute the shape operator, why not compute the principal curvatures and directions as the eigenvalues and eigenvectors of the shape operator? The implementation would be simplified and made more robust this way. I've added a method principal_directions_new to illustrate this.

I also made some small changes to some methods (e.g. to connection_coefficient) to make the implementation more readable, but nothing major.

The biggest work will now be in polishing the HTML documentation. If you do

$ cd SAGE_ROOT
$ ./sage -b
$ ./sage -docbuild reference html -j

the docs should be built, and the output will be in SAGE_ROOT/devel/sage/doc/output/html/en/reference/.

[4756944c-1c3a-4a3c-ad39-9cccb4f357db]

comment:28

Hi Joris, I am very sorry but at these days I was at a conference, and when I came back I had exams. That is why I did not response.

As for the principal curvatures and the principal directions, of course, we can compute them using the shape operator. The only reason why I did not do it in this way, that is I tried to avoid reference to additional methods and tried to make everything in a direct way. Here we have dimension two, therefore linear algebra is very straightforward. However, if we think about generalizations to higher dimensions, surely we need to use the shape operator.

This weekend I will try to learn how to use the CVS you told about, then I will post the other comments.

[4756944c-1c3a-4a3c-ad39-9cccb4f357db]

comment:29

Hi Joris,

At this weekend I tried to make the things in the right way. I make a clone of sage (a sandbox), the clone version works in the console, but not with the web interface. I mean that it starts the web browser and opens notebooks, but does not make calculations. Now I do not know why it happens. Then I applied the patch trac_10132_differential_geometry_sage_v3.2.patch and made "./sage -b", this does not report any errors, the files appeared at their places, and it generally works, though there is a mistake in calculating second fundamental form coefficients, because of two methods of finding second order natural frame. Then I applied the patch trac_10132_diff_geom_sage_doctests.patch, and got mistakes, something went wrong. I think it may be because you in fact use some new version of patch. So,

1) can you, please, submit the version of patch you use now? Just now I did not make any change in parametrized_surface3d, to avoid a situation, when we make changes in different files, and after that it will be hard to make the final version with all the changes. 2) if then I make some changes in the file parametrized_surface3d.py, how can I make the patch for sage? I mean the patch which can be understood by the method hg_sage.apply()?

[jvkersch]

comment:30

Hi Mikhail,

Sorry for the late reply -- I too was at a conference, and then I had to grade the exams for my class.

As for the problems with the patch, I think this is due to the fact that you applied both patches simultaneously. Normally, if you start from a clean Sage install, you can just apply trac_10132_diff_geom_sage_doctests.patch and this should have all the changes and build without problems (tested in Sage 4.6).

In general, I find it easier to work with Mercurial queues (see http://www.sagemath.org/doc/developer/walk_through.html#being-more-efficient-mercurial-queues). After setting up the queue system, you can then apply and unapply patches by simply typing hg qpush and hg qpop.

Just as an illustration, after setting up hg, here is how I would apply the differential geometry patch: in $SAGE_ROOT/devel/sage, do

hg qnew [name that I want to use for myself]
patch -p1 [path to the patch]

This imports the patch. Doing ./sage -b then rebuilds the system. After making changes I would then do the following to create a new patch:

hg qrefresh
hg export tip > [path to the new patch]

While making changes, you can do hg status to see which files have been modified. If you're not happy with the patch or if you want to put development aside while working on something else, you can do hg qpop to temporarily put the patch aside. hg qpush`` then restores the patch (but in each case do./sage -b` to rebuild the system).

I hope this helps -- please let me know if you have further questions!

[jvkersch]

comment:31

Replying to @sagetrac-mikarm:

As for the principal curvatures and the principal directions, of course, we can compute them using the shape operator. The only reason why I did not do it in this way, that is I tried to avoid reference to additional methods and tried to make everything in a direct way. Here we have dimension two, therefore linear algebra is very straightforward. However, if we think about generalizations to higher dimensions, surely we need to use the shape operator.

I will leave the decision to you, but I think that regardless of the method used, it would be good if the output of the method would be consistent with other commands in Sage. I'll leave the code to you for now, and focus on the documentation.

[jasongrout]

comment:32

You can use queues to automatically import the patch directly from the trac website, which preserves the author and message.

hg qimport https://github.com/sagemath/sage-prod/files/10651189/trac_10132_diff_geom_sage_doctests.patch.gz
hg qpush

[jvkersch]

Attachment: trac_10132_diff_geom_sage_doctests.patch.gz

Latest version of the patch, with 100% coverage and doctests in place.

[jvkersch]

comment:33

I wanted to get this last update in before the start of the new year. I've updated the docstrings for all the methods, and in some cases I've added a one-line piece of LaTeX explaining what is being computed.

My questions for now are the following:

1. Where should this code go in the Sage root? Currently the patch creates a new folder riemann, but maybe there is a subfolder somewhere that would be more appropriate.

2. What do we do with the computation of principal curvatures? I think that computing the eigenvalues of the shape operator (now that it is implemented in the surface class) is a quick and easy way of assembling the principal curvatures/directions, and moreover deals graciously with the case of equal principal curvatures, but I'm open for other ideas.

Other than that, I think the code is good (up to the bugs that Mikhail found, and which I didn't try to address), documentation looks good, and all that needs to be done is to write a nice overview of the class methods.

[4756944c-1c3a-4a3c-ad39-9cccb4f357db]

comment:34

Joris,

1) Possibly we can put the class into the folder "elementary_differential_geometry". In the same folder we will put the classes of curves in R^2 and R^3.

2) I agree that we can compute the principal curvatures and the principal directions via the shape operator, in fact you are right, in this way we can deal with the case of the equal principal curvatures.

Also, I agree that the code is good and is almost ready. Just now I get mistakes when applying the last patch, however I think I will manage with it in few days.

Joris, happy new year to you, wish you health and good luck, and thank you for your support which plays the crucial role in the development of the class.

[4756944c-1c3a-4a3c-ad39-9cccb4f357db]

comment:35

Hi Joris, I think I got how to use the Mercurial queues. However, the last patch gives me errors. These are:

---

applying trac_10132_diff_geom_sage_doctests.patch patching file doc/en/reference/index.rst Hunk #1 FAILED at 91 1 out of 1 hunks FAILED -- saving rejects to file doc/en/reference/index.rst.rej patching file sage/all.py Hunk #1 FAILED at 138 1 out of 1 hunks FAILED -- saving rejects to file sage/all.py.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir errors during apply, please fix and refresh trac_10132_diff_geom_sage_doctests.patch

---

There are two problems: in the file doc/references/index.rst it seems there is no line tensor which is assumed to be by the patch. In the file sage/all.py there is no line from sage.tensor.all import * which is also assumed to be by the patch. However, I made the changes from the patch by hand, then ./sage -b and it works fine. I checked the worksheet on the ellipsoid, everything including geodesics, works well. So I will proceed to check everything carefully and to complete the work. After that I will submit the result :)

[jvkersch]

comment:36

Hi Mikhail,

I am glad to hear that everything works (modulo the changes), and I also wish you the very best for this year. It was great playing with this patch -- it reminded me again of why differential geometry has always been my favorite subject!

As for the patch rejects, could this be due to an older version of Sage? The tensor package made it into Sage 4.6, so if you use any version of Sage before that, I guess there can be problems applying the patch. However, as our class doesn't depend on tensor (though maybe it will in the future?) you can just apply the patch the way you described, though upgrading wouldn't hurt.

[4756944c-1c3a-4a3c-ad39-9cccb4f357db]

comment:37

Yes Joris, I have downloaded and compiled sage-4.6 and your patch applied well, without any mistakes. Thank you!

[4756944c-1c3a-4a3c-ad39-9cccb4f357db]

comment:38

Joris, I submit the final version of the patch. I think it is ok, it passes through the tests well, the only thing I have not done yet is the "long tests". Please, look through, may be you have some improvements, or comments.

[4756944c-1c3a-4a3c-ad39-9cccb4f357db]

Attachment: trac_10132_final.patch.gz

[4756944c-1c3a-4a3c-ad39-9cccb4f357db]

Description changed:

--- 
+++ 
@@ -1 +1,5 @@
+Joris.
+Now the patch is ready and passes through all tests successfully.
+So I changed the status for "Needs review".
+Thanks a lot for your cooperation!