Sage: Ticket #10132: Parametrization of (metric) surfaces in 3D euclidean space
https://trac.sagemath.org/ticket/10132
<p>
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, ...).
</p>
<p>
Apply <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/10132/trac_10132_revised_sage-5.13.beta2.patch" title="Attachment 'trac_10132_revised_sage-5.13.beta2.patch' in Ticket #10132">trac_10132_revised_sage-5.13.beta2.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/10132/trac_10132_revised_sage-5.13.beta2.patch" title="Download"></a>
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/10132
Trac 1.1.6mikarmFri, 15 Oct 2010 13:33:26 GMTattachment set
https://trac.sagemath.org/ticket/10132
https://trac.sagemath.org/ticket/10132
<ul>
<li><strong>attachment</strong>
set to <em>mikarm_ticket_files.zip</em>
</li>
</ul>
<p>
parametrized_curve3d class and example worksheets
</p>
TicketjasonFri, 15 Oct 2010 15:36:59 GMTcc set
https://trac.sagemath.org/ticket/10132#comment:1
https://trac.sagemath.org/ticket/10132#comment:1
<ul>
<li><strong>cc</strong>
<em>jvkersch</em> <em>mhampton</em> <em>niles</em> <em>jason</em> added
</li>
</ul>
<p>
CCing some people that probably are interested in these developments (see <a class="closed ticket" href="https://trac.sagemath.org/ticket/9650" title="enhancement: Adding support for differential forms (closed: fixed)">#9650</a>)
</p>
<p>
It would be good to post a message to sage-devel to coordinate efforts (pun intended! :)
</p>
TicketnovoseltFri, 15 Oct 2010 18:38:40 GMTcc changed
https://trac.sagemath.org/ticket/10132#comment:2
https://trac.sagemath.org/ticket/10132#comment:2
<ul>
<li><strong>cc</strong>
<em>novoselt</em> added
</li>
</ul>
<p>
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.
</p>
TicketjvkerschFri, 15 Oct 2010 18:43:35 GMT
https://trac.sagemath.org/ticket/10132#comment:3
https://trac.sagemath.org/ticket/10132#comment:3
<p>
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.
</p>
<p>
Some comments:
</p>
<ul><li>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.
</li></ul><ul><li>There is a differential forms class at <a class="closed ticket" href="https://trac.sagemath.org/ticket/9650" title="enhancement: Adding support for differential forms (closed: fixed)">#9650</a> 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.
</li></ul>
TicketmikarmSat, 16 Oct 2010 13:54:30 GMT
https://trac.sagemath.org/ticket/10132#comment:4
https://trac.sagemath.org/ticket/10132#comment:4
<p>
jason,jverksch, novoselt, thank you very much for your support and information on the differential form class.
</p>
<p>
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.
</p>
<p>
to jvkersch. I would highly appreciate it if you could explain me how to <code></code>sagify<em> a python class, it is really my weak point.
</em></p>
<p>
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?
</p>
<p>
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?
</p>
TicketnovoseltSun, 17 Oct 2010 00:54:35 GMT
https://trac.sagemath.org/ticket/10132#comment:5
https://trac.sagemath.org/ticket/10132#comment:5
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10132#comment:4" title="Comment 4">mikarm</a>:
</p>
<blockquote class="citation">
<p>
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?
</p>
</blockquote>
<p>
There are some wiki pages maintained, e.g. for graph development or <a class="ext-link" href="http://wiki.sagemath.org/combinat"><span class="icon"></span>http://wiki.sagemath.org/combinat</a>
</p>
<blockquote class="citation">
<p>
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?
</p>
</blockquote>
<p>
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 <code>is_compact</code> or maybe chart access can be made general.)
</p>
TicketjvkerschMon, 18 Oct 2010 05:12:27 GMT
https://trac.sagemath.org/ticket/10132#comment:6
https://trac.sagemath.org/ticket/10132#comment:6
<p>
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 <a href="http://www.sagemath.org/doc/developer/coding_in_python.html#creating-a-new-directory">this page</a>). Please see the attached patch for details. What it does is the following:
</p>
<ol><li>Create a new directory <code>riemann</code>, containing the parametrized surface class that Mikhail uploaded in the zip file, together with the vector functions class.
</li></ol><ol start="2"><li>Updates <code>setup.py</code> and <code>all.py</code> to make Sage aware of these additions.
</li></ol><p>
I deliberately did not want to change any code if I could help it, but here are the modifications that I made:
</p>
<ol><li>Changed the capitalization to <code>ParametrizedSurface3D</code> to conform to the Sage standard;
</li><li>Removed the references to <code>RR</code> and <code>CC</code> since these already have standard meanings in Sage;
</li><li>I added a class <code>GenericManifold</code> 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.
</li><li>Throughout the code, I added various import statements where it was necessary. I ended up changing some of the constructions in the functions <code>principal_curvatures</code>, <code> geodesics_numerical</code> and <code>parallel_translation_numerical</code> 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.
</li></ol><p>
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.
</p>
<p>
Here are a few remarks that I came across while tweaking the code.
</p>
<ol><li>Sometimes the terminology "vector" is used where "vector field" would be more appropriate.
</li><li>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 <code>__getitem__</code> and <code>__setitem__</code> and then let this class deal with components, input checking, etc.
</li><li>I think that some of the functions in the <code>vector_functions</code> module have Sage alternatives, which should be used instead.
</li><li>We need to think on providing an interface to <code>parametric_plot3d</code>, so that these kind of surfaces can easily be plotted.
</li></ol><p>
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).
</p>
<ol><li>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.
</li><li>The numerical routines should be optimized for float operations using <code>fast_float</code> or similar.
</li></ol><p>
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.
</p>
TicketjvkerschMon, 18 Oct 2010 05:13:39 GMTattachment set
https://trac.sagemath.org/ticket/10132
https://trac.sagemath.org/ticket/10132
<ul>
<li><strong>attachment</strong>
set to <em>trac_10132_differential_geometry_sage.patch</em>
</li>
</ul>
<p>
Code of Mikhail as a Sage patch
</p>
TicketmikarmWed, 20 Oct 2010 00:27:25 GMTdescription changed
https://trac.sagemath.org/ticket/10132#comment:7
https://trac.sagemath.org/ticket/10132#comment:7
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/10132?action=diff&version=7">diff</a>)
</li>
</ul>
<p>
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.
</p>
<p>
Now concerning the remarks:
</p>
<blockquote class="citation">
<p>
Sometimes the terminology "vector" is used where "vector field" would be more appropriate.
</p>
</blockquote>
<p>
Yes, sure, we will change this.
</p>
<blockquote class="citation">
<p>
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 <span class="underline">getitem</span> and <span class="underline">setitem</span> and then let this class deal with components, input checking, etc.
</p>
</blockquote>
<p>
Also agree.
</p>
<blockquote class="citation">
<p>
I think that some of the functions in the vector_functions module have Sage alternatives, which should be used instead.
</p>
</blockquote>
<p>
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.
</p>
<blockquote class="citation">
<p>
We need to think on providing an interface to parametric_plot3d, so that these kind of surfaces can easily be plotted.
</p>
</blockquote>
<p>
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.
</p>
<blockquote class="citation">
<p>
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.
</p>
</blockquote>
<p>
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.
</p>
<blockquote class="citation">
<p>
The numerical routines should be optimized for float operations using fast_float or similar.
</p>
</blockquote>
<p>
Completely agree, we should do it.
</p>
<p>
To Andrei:
Wiki, it is a good idea, thank you! I will look how to do it, and surely this is the decision.
</p>
<blockquote class="citation">
<p>
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.
</p>
</blockquote>
<p>
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.
</p>
<p>
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.
</p>
TicketmaldunWed, 20 Oct 2010 19:11:47 GMTcc changed
https://trac.sagemath.org/ticket/10132#comment:8
https://trac.sagemath.org/ticket/10132#comment:8
<ul>
<li><strong>cc</strong>
<em>maldun</em> added
</li>
</ul>
TicketmpatelWed, 20 Oct 2010 21:43:43 GMTcc changed
https://trac.sagemath.org/ticket/10132#comment:9
https://trac.sagemath.org/ticket/10132#comment:9
<ul>
<li><strong>cc</strong>
<em>mpatel</em> added
</li>
</ul>
TicketjvkerschTue, 26 Oct 2010 05:35:44 GMT
https://trac.sagemath.org/ticket/10132#comment:10
https://trac.sagemath.org/ticket/10132#comment:10
<p>
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.
</p>
<p>
The most significant is that I think that the member functions such as <code>natural_frame</code>, <code>first_fundamental_form_coefficients</code>, <code>second_order_natural_frame</code>, 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.
</p>
<ol><li>By returning a dictionary/list we avoid having to check whether the index is in the appropriate range.
</li><li>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);
</li><li>It is slightly counter intuitive to have a function (say) <code>natural_frame</code> return a vector rather than a basis. If the user is only interested in one component, they can still write
<pre class="wiki"> sage: surface = ParametrizedCurve3D(...)
sage: f = surface.natural_frame()
sage: print f[0]
</pre></li><li>For the functions like <code>natural_frame</code> that return a frame, it would be more in line with current Sage code (see for instance the <code>basis</code> member function for vector spaces) to return a full frame as a list.
</li></ol><p>
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.
</p>
TicketjvkerschTue, 26 Oct 2010 05:37:44 GMTattachment set
https://trac.sagemath.org/ticket/10132
https://trac.sagemath.org/ticket/10132
<ul>
<li><strong>attachment</strong>
set to <em>Parametrized Curve_ Computing Geodesics.sws</em>
</li>
</ul>
<p>
Worksheet to illustrate caching for the computation of geodesics
</p>
TicketjvkerschTue, 26 Oct 2010 05:45:40 GMT
https://trac.sagemath.org/ticket/10132#comment:11
https://trac.sagemath.org/ticket/10132#comment:11
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10132#comment:10" title="Comment 10">jvkersch</a>:
</p>
<blockquote class="citation">
<p>
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.
</p>
</blockquote>
<p>
I needed to add that as an illustration of what I wrote I've updated the functions <code>natural_frame</code> and <code>first_fundamental_form_coefficients</code>, but I've left the other functions unchanged.
</p>
TicketjvkerschTue, 26 Oct 2010 05:48:17 GMT
https://trac.sagemath.org/ticket/10132#comment:12
https://trac.sagemath.org/ticket/10132#comment:12
<p>
To see whether caching would speed up things, I've also added a method <code>geodesics_numerical_fast</code>, 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.
</p>
TicketmikarmWed, 27 Oct 2010 03:28:12 GMT
https://trac.sagemath.org/ticket/10132#comment:13
https://trac.sagemath.org/ticket/10132#comment:13
<p>
Joris,
</p>
<p>
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.
</p>
<p>
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 :).
</p>
<p>
Maybe you have other solutions which are more efficient than this one.
</p>
<p>
Anyway, at the weekend I plan to play with the improved version of the package you submitted here, and will comment on it.
</p>
<p>
Again thank you, it is more interesting to work together!
</p>
<p>
Mikhail
</p>
TicketjvkerschThu, 28 Oct 2010 03:35:23 GMT
https://trac.sagemath.org/ticket/10132#comment:14
https://trac.sagemath.org/ticket/10132#comment:14
<p>
Hi Mikhail,
</p>
<p>
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".
</p>
<p>
It turns out there is a very easy way to cache the results of these functions, by preceeding them with the decorator <code>@cached_method</code>. 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.
</p>
<p>
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.
</p>
<p>
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.
</p>
TicketjvkerschSun, 31 Oct 2010 23:16:31 GMT
https://trac.sagemath.org/ticket/10132#comment:15
https://trac.sagemath.org/ticket/10132#comment:15
<p>
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 <code>natural_frame</code>, <code>first_fundamental_form</code>, etc.
</p>
<p>
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 <code>_compute_first_fundamental_form_coefficient</code> which computes one component.
</p>
<p>
The interaction with the user is done through two other member functions: first, there is <code>first_fundamental_form_coefficient_new</code> 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 <code>_compute_...</code> to do the work. Secondly there is also a member function <code>first_fundamental_form_coefficients_new</code> which creates a dictionary with all the components.
</p>
<p>
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.
</p>
<p>
For the sake of comparison, I've labeled my additions with the suffix <code>_new</code> 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.
</p>
TicketjvkerschSun, 31 Oct 2010 23:29:35 GMTattachment set
https://trac.sagemath.org/ticket/10132
https://trac.sagemath.org/ticket/10132
<ul>
<li><strong>attachment</strong>
set to <em>trac_10132_differential_geometry_sage_v2.patch</em>
</li>
</ul>
<p>
New version of the patch
</p>
TicketjvkerschSun, 31 Oct 2010 23:29:54 GMTattachment set
https://trac.sagemath.org/ticket/10132
https://trac.sagemath.org/ticket/10132
<ul>
<li><strong>attachment</strong>
set to <em>ParametrizedSurface3D_ Examples.sws</em>
</li>
</ul>
TicketmikarmMon, 01 Nov 2010 02:28:49 GMT
https://trac.sagemath.org/ticket/10132#comment:16
https://trac.sagemath.org/ticket/10132#comment:16
<p>
Joris,
</p>
<p>
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.
</p>
<p>
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
</p>
<p>
And after that I suppose we can submit the class to Sage.
</p>
TicketmikarmMon, 01 Nov 2010 02:34:23 GMTattachment set
https://trac.sagemath.org/ticket/10132
https://trac.sagemath.org/ticket/10132
<ul>
<li><strong>attachment</strong>
set to <em>parametrized_surface3d.py</em>
</li>
</ul>
<p>
version 2 of parametrized_surface3d.py with comments
</p>
TicketjvkerschFri, 05 Nov 2010 05:46:21 GMT
https://trac.sagemath.org/ticket/10132#comment:17
https://trac.sagemath.org/ticket/10132#comment:17
<p>
Hi Mikhail,
</p>
<p>
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 <code>natural_frame</code>) and the numerical treatment of parallel transport.
</p>
<p>
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...
</p>
<p>
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.
</p>
<p>
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...
</p>
TicketjvkerschFri, 05 Nov 2010 05:46:58 GMTattachment set
https://trac.sagemath.org/ticket/10132
https://trac.sagemath.org/ticket/10132
<ul>
<li><strong>attachment</strong>
set to <em>trac_10132_differential_geometry_sage_v3.patch</em>
</li>
</ul>
TicketmikarmSat, 06 Nov 2010 14:10:36 GMT
https://trac.sagemath.org/ticket/10132#comment:18
https://trac.sagemath.org/ticket/10132#comment:18
<p>
Hi Joris,
</p>
<p>
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).
</p>
<p>
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.
</p>
<p>
Mikhail
</p>
TicketmikarmSat, 06 Nov 2010 14:12:13 GMTattachment set
https://trac.sagemath.org/ticket/10132
https://trac.sagemath.org/ticket/10132
<ul>
<li><strong>attachment</strong>
set to <em>parametrized_surface3d.2.py</em>
</li>
</ul>
<p>
parametrized_surface3d.py with small changes in parallel_translation_numeric_new
</p>
TicketmikarmSat, 06 Nov 2010 14:14:08 GMTattachment set
https://trac.sagemath.org/ticket/10132
https://trac.sagemath.org/ticket/10132
<ul>
<li><strong>attachment</strong>
set to <em>parallel_translation.sws</em>
</li>
</ul>
<p>
an example of usage of the method parallel_translation_numerical_new
</p>
TicketjvkerschMon, 08 Nov 2010 02:31:08 GMT
https://trac.sagemath.org/ticket/10132#comment:19
https://trac.sagemath.org/ticket/10132#comment:19
<p>
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.
</p>
<p>
The problem with the code that I wrote is the following: when writing
</p>
<pre class="wiki"> C_1 = self.connection_coefficients()
for coef in C_1:
C_1[coef] = C_1[coef].subs({u1: curve[0], u2: curve[1]})
</pre><p>
<code>C_1</code> 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
</p>
<pre class="wiki"> C_1 = self.connection_coefficients().copy()
</pre><p>
also fixes the problem.
</p>
<p>
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.
</p>
<p>
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.
</p>
TicketjvkerschMon, 08 Nov 2010 02:31:42 GMTattachment set
https://trac.sagemath.org/ticket/10132
https://trac.sagemath.org/ticket/10132
<ul>
<li><strong>attachment</strong>
set to <em>trac_10132_differential_geometry_sage_v3.2.patch</em>
</li>
</ul>
TicketjasonMon, 08 Nov 2010 16:14:43 GMT
https://trac.sagemath.org/ticket/10132#comment:20
https://trac.sagemath.org/ticket/10132#comment:20
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10132#comment:19" title="Comment 19">jvkersch</a>:
</p>
<blockquote class="citation">
<p>
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.
</p>
<p>
The problem with the code that I wrote is the following: when writing
</p>
<pre class="wiki"> C_1 = self.connection_coefficients()
for coef in C_1:
C_1[coef] = C_1[coef].subs({u1: curve[0], u2: curve[1]})
</pre><p>
<code>C_1</code> 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.
</p>
</blockquote>
<p>
Are you sure? In the code (the latest patch), it appears that the <code>connection_coefficients</code> return value is constructed as a new dictionary each time the function is called.
</p>
TicketjvkerschFri, 12 Nov 2010 05:40:17 GMT
https://trac.sagemath.org/ticket/10132#comment:21
https://trac.sagemath.org/ticket/10132#comment:21
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10132#comment:20" title="Comment 20">jason</a>:
</p>
<blockquote class="citation">
<p>
Are you sure? In the code (the latest patch), it appears that the <code>connection_coefficients</code> return value is constructed as a new dictionary each time the function is called.
</p>
</blockquote>
<p>
Hi Jason,
</p>
<p>
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,
</p>
<pre class="wiki">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()
</pre><p>
prints
</p>
<pre class="wiki">{1: 'a', 2: 'b', 3: 'c'}
{1: 'a', 2: 'b', 3: 'something else'}
</pre><p>
instead of two copies of the same.
</p>
<p>
I don't really get why this would cause the problems in <code>_create_pt_ode_system</code>, but addressing it does seem to solve the problem...
</p>
TicketmikarmMon, 15 Nov 2010 03:38:32 GMT
https://trac.sagemath.org/ticket/10132#comment:22
https://trac.sagemath.org/ticket/10132#comment:22
<p>
Hi Joris,
</p>
<p>
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?
</p>
<blockquote class="citation">
<p>
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.
</p>
</blockquote>
<p>
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...
</p>
<p>
Mikhail
</p>
TicketmikarmMon, 15 Nov 2010 03:39:28 GMTattachment set
https://trac.sagemath.org/ticket/10132
https://trac.sagemath.org/ticket/10132
<ul>
<li><strong>attachment</strong>
set to <em>parametrized_surface3d.3.py</em>
</li>
</ul>
<p>
Next version of ParametrizedSurface3D class
</p>
TicketmikarmMon, 15 Nov 2010 03:41:10 GMTattachment set
https://trac.sagemath.org/ticket/10132
https://trac.sagemath.org/ticket/10132
<ul>
<li><strong>attachment</strong>
set to <em>Minimal_surfaces_and_surface_of_revolution.sws</em>
</li>
</ul>
<p>
Examples of calculations for minimal surfaces and some calculations for the surfaces of revolution in general form
</p>
TicketjvkerschWed, 24 Nov 2010 19:35:11 GMTstatus changed
https://trac.sagemath.org/ticket/10132#comment:23
https://trac.sagemath.org/ticket/10132#comment:23
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_work</em>
</li>
</ul>
<p>
Hi Mikhail,
</p>
<p>
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: <code>_latex_</code> and <code>_view_</code>, 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.
</p>
<p>
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.
</p>
<p>
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 <code>parametrized_surface3d.py</code> file. I just follow the instructions on <a href="http://www.sagemath.org/doc/developer/walk_through.html#modifying-sage-source-code">http://www.sagemath.org/doc/developer/walk_through.html#modifying-sage-source-code</a> to set up Mercurial and to import and generate patches, and I didn't learn any of the more advanced stuff.
</p>
<p>
J.
</p>
TicketjvkerschThu, 25 Nov 2010 08:03:49 GMT
https://trac.sagemath.org/ticket/10132#comment:24
https://trac.sagemath.org/ticket/10132#comment:24
<p>
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.
</p>
TicketmhamptonThu, 25 Nov 2010 18:19:41 GMT
https://trac.sagemath.org/ticket/10132#comment:25
https://trac.sagemath.org/ticket/10132#comment:25
<p>
So if I want to check this out, do I just apply the patch:
</p>
<p>
trac_10132_diff_geom_sage_doctests.patch
</p>
<p>
?
</p>
<p>
Or are there other required pieces?
</p>
<p>
Thanks,
Marshall
</p>
TicketjvkerschFri, 26 Nov 2010 15:27:53 GMT
https://trac.sagemath.org/ticket/10132#comment:26
https://trac.sagemath.org/ticket/10132#comment:26
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10132#comment:25" title="Comment 25">mhampton</a>:
</p>
<blockquote class="citation">
<p>
So if I want to check this out, do I just apply the patch:
</p>
<p>
trac_10132_diff_geom_sage_doctests.patch
</p>
</blockquote>
<p>
Hi Marshall,
</p>
<p>
Yes, this is the right version. Keep in mind that half of the doctests still aren't updated though (but the functionality is there).
</p>
<p>
J.
</p>
TicketjvkerschMon, 29 Nov 2010 06:09:12 GMT
https://trac.sagemath.org/ticket/10132#comment:27
https://trac.sagemath.org/ticket/10132#comment:27
<p>
The version of the patch which I've uploaded passes all doctests (<code></code>./sage -t devel/sage/sage/riemann/parametrized_surface3d.py<code></code>) and is at 100% coverage. I've also added a method <code></code>plot3d<code></code> which allows for straightforward plotting of the surface.
</p>
<p>
While going over the code, I had the following questions:
</p>
<ol><li> Doctesting takes about 1 min. on my Macbook. Is this acceptable, or too long?
</li><li> 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 <code></code>principal_directions_new<code></code> to illustrate this.
</li></ol><p>
I also made some small changes to some methods (e.g. to <code></code>connection_coefficient<code></code>) to make the implementation more readable, but nothing major.
</p>
<p>
The biggest work will now be in polishing the HTML documentation. If you do
</p>
<div class="wiki-code"><div class="code"><pre><span class="nv">$ </span><span class="nb">cd </span>SAGE_ROOT
<span class="nv">$ </span>./sage -b
<span class="nv">$ </span>./sage -docbuild reference html -j
</pre></div></div><p>
the docs should be built, and the output will be in <code></code>SAGE_ROOT/devel/sage/doc/output/html/en/reference/<code></code>.
</p>
TicketmikarmFri, 10 Dec 2010 02:32:36 GMT
https://trac.sagemath.org/ticket/10132#comment:28
https://trac.sagemath.org/ticket/10132#comment:28
<p>
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.
</p>
<p>
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.
</p>
<p>
This weekend I will try to learn how to use the CVS you told about, then I will post the other comments.
</p>
TicketmikarmSun, 12 Dec 2010 16:16:08 GMT
https://trac.sagemath.org/ticket/10132#comment:29
https://trac.sagemath.org/ticket/10132#comment:29
<p>
Hi Joris,
</p>
<p>
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,
</p>
<p>
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()?
</p>
TicketjvkerschWed, 22 Dec 2010 02:36:27 GMT
https://trac.sagemath.org/ticket/10132#comment:30
https://trac.sagemath.org/ticket/10132#comment:30
<p>
Hi Mikhail,
</p>
<p>
Sorry for the late reply -- I too was at a conference, and then I had to grade the exams for my class.
</p>
<p>
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 <code>trac_10132_diff_geom_sage_doctests.patch</code> and this should have all the changes and build without problems (tested in Sage 4.6).
</p>
<p>
In general, I find it easier to work with Mercurial queues (see <a href="http://www.sagemath.org/doc/developer/walk_through.html#being-more-efficient-mercurial-queues">http://www.sagemath.org/doc/developer/walk_through.html#being-more-efficient-mercurial-queues</a>). After setting up the queue system, you can then apply and unapply patches by simply typing <code>hg qpush</code> and <code>hg qpop</code>.
</p>
<p>
Just as an illustration, after setting up hg, here is how I would apply the differential geometry patch: in <code>$SAGE_ROOT/devel/sage</code>, do
</p>
<pre class="wiki">hg qnew [name that I want to use for myself]
patch -p1 [path to the patch]
</pre><p>
This imports the patch. Doing <code>./sage -b</code> then rebuilds the system. After making changes I would then do the following to create a new patch:
</p>
<pre class="wiki">hg qrefresh
hg export tip > [path to the new patch]
</pre><p>
While making changes, you can do <code>hg status</code> 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 <code>hg qpop</code> to temporarily put the patch aside. <code>hg qpush</code><code> then restores the patch (but in each case do </code>./sage -b` to rebuild the system).
</p>
<p>
I hope this helps -- please let me know if you have further questions!
</p>
TicketjvkerschWed, 22 Dec 2010 02:38:28 GMT
https://trac.sagemath.org/ticket/10132#comment:31
https://trac.sagemath.org/ticket/10132#comment:31
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10132#comment:28" title="Comment 28">mikarm</a>:
</p>
<blockquote class="citation">
<p>
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.
</p>
</blockquote>
<p>
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.
</p>
TicketjasonWed, 22 Dec 2010 02:40:55 GMT
https://trac.sagemath.org/ticket/10132#comment:32
https://trac.sagemath.org/ticket/10132#comment:32
<p>
You can use queues to automatically import the patch directly from the trac website, which preserves the author and message.
</p>
<pre class="wiki">hg qimport http://trac.sagemath.org/sage_trac/raw-attachment/ticket/10132/trac_10132_diff_geom_sage_doctests.patch
hg qpush
</pre>
TicketjvkerschThu, 30 Dec 2010 19:21:28 GMTattachment set
https://trac.sagemath.org/ticket/10132
https://trac.sagemath.org/ticket/10132
<ul>
<li><strong>attachment</strong>
set to <em>trac_10132_diff_geom_sage_doctests.patch</em>
</li>
</ul>
<p>
Latest version of the patch, with 100% coverage and doctests in place.
</p>
TicketjvkerschThu, 30 Dec 2010 19:32:41 GMT
https://trac.sagemath.org/ticket/10132#comment:33
https://trac.sagemath.org/ticket/10132#comment:33
<p>
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.
</p>
<p>
My questions for now are the following:
</p>
<ol><li>Where should this code go in the Sage root? Currently the patch creates a new folder <code>riemann</code>, but maybe there is a subfolder somewhere that would be more appropriate.
</li></ol><ol><li>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.
</li></ol><p>
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.
</p>
TicketmikarmFri, 31 Dec 2010 17:40:34 GMT
https://trac.sagemath.org/ticket/10132#comment:34
https://trac.sagemath.org/ticket/10132#comment:34
<p>
Joris,
</p>
<p>
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<sup>2 and R</sup>3.
</p>
<p>
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.
</p>
<p>
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.
</p>
<p>
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.
</p>
TicketmikarmMon, 03 Jan 2011 14:23:10 GMT
https://trac.sagemath.org/ticket/10132#comment:35
https://trac.sagemath.org/ticket/10132#comment:35
<p>
Hi Joris,
I think I got how to use the Mercurial queues. However, the last patch gives me errors. These are:
<strong></strong><strong></strong><strong>
applying trac_10132_diff_geom_sage_doctests.patch
patching file doc/en/reference/index.rst
Hunk <a class="closed ticket" href="https://trac.sagemath.org/ticket/1" title="defect: SAGE Notebook leaves dead processes on OS X (closed: fixed)">#1</a> 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 <a class="closed ticket" href="https://trac.sagemath.org/ticket/1" title="defect: SAGE Notebook leaves dead processes on OS X (closed: fixed)">#1</a> 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
</strong><strong></strong><strong></strong><strong></strong><strong>*
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 :)
</strong></p>
TicketjvkerschFri, 07 Jan 2011 01:27:12 GMT
https://trac.sagemath.org/ticket/10132#comment:36
https://trac.sagemath.org/ticket/10132#comment:36
<p>
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!
</p>
<p>
As for the patch rejects, could this be due to an older version of Sage? The <code>tensor</code> 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.
</p>
TicketmikarmSat, 08 Jan 2011 11:32:07 GMT
https://trac.sagemath.org/ticket/10132#comment:37
https://trac.sagemath.org/ticket/10132#comment:37
<p>
Yes Joris,
I have downloaded and compiled sage-4.6 and your patch applied well, without any mistakes. Thank you!
</p>
TicketmikarmWed, 02 Mar 2011 02:35:33 GMT
https://trac.sagemath.org/ticket/10132#comment:38
https://trac.sagemath.org/ticket/10132#comment:38
<p>
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.
</p>
TicketmikarmWed, 02 Mar 2011 02:36:27 GMTattachment set
https://trac.sagemath.org/ticket/10132
https://trac.sagemath.org/ticket/10132
<ul>
<li><strong>attachment</strong>
set to <em>trac_10132_final.patch</em>
</li>
</ul>
TicketmikarmTue, 15 Mar 2011 11:21:48 GMTstatus, description changed
https://trac.sagemath.org/ticket/10132#comment:39
https://trac.sagemath.org/ticket/10132#comment:39
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/10132?action=diff&version=39">diff</a>)
</li>
</ul>
TicketmikarmTue, 15 Mar 2011 11:22:15 GMTowner changed
https://trac.sagemath.org/ticket/10132#comment:40
https://trac.sagemath.org/ticket/10132#comment:40
<ul>
<li><strong>owner</strong>
changed from <em>mhampton</em> to <em>mikarm</em>
</li>
</ul>
TicketmikarmTue, 15 Mar 2011 11:24:04 GMTattachment set
https://trac.sagemath.org/ticket/10132
https://trac.sagemath.org/ticket/10132
<ul>
<li><strong>attachment</strong>
set to <em>trac_10132_final_all_files_done.patch</em>
</li>
</ul>
TicketmikarmTue, 15 Mar 2011 11:25:00 GMTdescription changed
https://trac.sagemath.org/ticket/10132#comment:41
https://trac.sagemath.org/ticket/10132#comment:41
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/10132?action=diff&version=41">diff</a>)
</li>
</ul>
<p>
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!
</p>
TicketmikarmWed, 16 Mar 2011 09:39:55 GMTdescription changed
https://trac.sagemath.org/ticket/10132#comment:42
https://trac.sagemath.org/ticket/10132#comment:42
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/10132?action=diff&version=42">diff</a>)
</li>
</ul>
TicketnovoseltWed, 16 Mar 2011 13:52:00 GMTdescription changed; milestone set
https://trac.sagemath.org/ticket/10132#comment:43
https://trac.sagemath.org/ticket/10132#comment:43
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/10132?action=diff&version=43">diff</a>)
</li>
<li><strong>milestone</strong>
set to <em>sage-4.7</em>
</li>
</ul>
<p>
Made a patchbot-friendly description.
</p>
TicketvdelecroixSun, 24 Apr 2011 21:09:44 GMTstatus, description, summary changed; reviewer set
https://trac.sagemath.org/ticket/10132#comment:44
https://trac.sagemath.org/ticket/10132#comment:44
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>reviewer</strong>
set to <em>vdelecroix</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/10132?action=diff&version=44">diff</a>)
</li>
<li><strong>summary</strong>
changed from <em>Differential Geometry via Sage</em> to <em>Parametrization of (metric) surfaces in 3D euclidean space</em>
</li>
</ul>
<p>
Hello,
</p>
<p>
That's nice to see some Riemannian geometry to come in ! Before I'm playing with the patch and be involved into details, I have some general preliminary remarks
</p>
<p>
0) The description of this patch (on this webpage) should contains few words about what you intend to developp. I wrote few lines of description that you can modify.
</p>
<p>
1) <strong>patch errors</strong>
</p>
<ul><li>the patch trac_10123_final_allfiles_done.patch does not contain the modification of "setup.py" which is needed to take into account modification of Sage sources
</li><li>I get a minor error for patching index.rst in the doc with sage-4.6.2
</li></ul><p>
2) <strong>name logic</strong>
</p>
<ul><li>There is a directory sage/geometry/ which is the directory intended for... geometry. It would be better to put all Riemannian geometry inside.
</li><li>"riemann" is a very ambiguous name as there is the Riemaniann geometry which deals with manifolds with metric, Riemann surface which are complex curves, Riemann hypothesis, ... perhaps riemannian_manifolds (repository are always plural) is safer and more comprehensive.
</li><li>the vector (resp. matrix) manipulation for simplifications of coordinates should be put elsewhere because it has nothing to do with riemannian geometry. The ideal way to do is to implement a new class for vectors on symbolic ring (look at the repertory sage/modules). But that job is a patch in its own. The simplest way I see is that you move this file as sage/modules/vector_symbolic_ring.py and inside its preambule write a word about your needs (ie simplification of coordinates) and make an explicit link to your file. That way, when somebody else comes in to implement the class for vector on symbolic rings he will modify your functions on Riemannian manifolds accordingly.
</li></ul><p>
3) <strong>about a class for generic manifolds:</strong> I do not think the class <a class="missing wiki">GenericManifold?</a> would be interesting. If anyway you intend to add it, it should be put in another file in another place. If you start such generic stuff, your ParametrizedSurface3D should derive from something like <a class="missing wiki">EmbeddedRiemannianManifold?</a>. And if so, you have to implement a class for morphisms between <a class="missing wiki">RiemannianManifolds?</a>... it will be painful !
</p>
<p>
4) In your file you mention "<a class="missing wiki">PatchOfCoordinates?</a>" without any reference to the corresponding file ! I think it would be very useful to use it right now in order to be able to have a parametrization defined on a restricted domain... In a soon future it can be used to write differential equations on the surface itself.
</p>
TicketvdelecroixSun, 24 Apr 2011 21:10:30 GMTdescription changed
https://trac.sagemath.org/ticket/10132#comment:45
https://trac.sagemath.org/ticket/10132#comment:45
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/10132?action=diff&version=45">diff</a>)
</li>
</ul>
TicketjvkerschSun, 15 May 2011 21:35:25 GMT
https://trac.sagemath.org/ticket/10132#comment:46
https://trac.sagemath.org/ticket/10132#comment:46
<p>
Bonjour Vincent,
</p>
<p>
Thanks a lot for your comments, which are very accurate and helpful, and thanks for acting as a reviewer! Sorry also for the long delay in answering.
</p>
<p>
I agree with the suggestions that you made, and I am currently preparing a new version of the patch. In particular, I agree that the symbolic vector functionality should be moved out of the geometry class. This feels like a big change, however, so maybe this should have its own ticket? Please let me know what you prefer.
</p>
<p>
I need a few more days to clean up the code, and I will post an updated patch shortly.
</p>
<p>
With best wishes,
Joris
</p>
TicketjvkerschMon, 16 May 2011 06:36:11 GMT
https://trac.sagemath.org/ticket/10132#comment:47
https://trac.sagemath.org/ticket/10132#comment:47
<p>
Someone should delete my previous message, since I'm just rehashing what Vincent already pointed out. Anyway, I agree with the need for code dealing with simplifying symbolic matrices and vectors, and this is now <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/10552" title="enhancement: Allow more elementwise simplifications for symbolic matrices (needs_work)">#10552</a> and <a class="closed ticket" href="https://trac.sagemath.org/ticket/11335" title="enhancement: Allow symbolic vectors to be simplified elementwise (closed: fixed)">#11335</a>, respectively.
</p>
TicketjvkerschTue, 28 Jun 2011 03:07:21 GMTdependencies set
https://trac.sagemath.org/ticket/10132#comment:48
https://trac.sagemath.org/ticket/10132#comment:48
<ul>
<li><strong>dependencies</strong>
set to <em>#10552</em>
</li>
</ul>
<p>
I removed the simplification code for vectors and matrices, moved the files to a subfolder of <code>sage.geometry</code> and smoothed out some parts of the code. Doctests seem to pass and the documentation builds cleanly.
</p>
TicketjvkerschTue, 28 Jun 2011 03:08:01 GMTattachment set
https://trac.sagemath.org/ticket/10132
https://trac.sagemath.org/ticket/10132
<ul>
<li><strong>attachment</strong>
set to <em>trac_10123_final_allfiles_done.patch</em>
</li>
</ul>
TicketjvkerschTue, 28 Jun 2011 03:08:52 GMTstatus, dependencies changed
https://trac.sagemath.org/ticket/10132#comment:49
https://trac.sagemath.org/ticket/10132#comment:49
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>dependencies</strong>
changed from <em>#10552</em> to <em>#10552, #11549</em>
</li>
</ul>
TicketjvkerschTue, 28 Jun 2011 03:10:54 GMT
https://trac.sagemath.org/ticket/10132#comment:50
https://trac.sagemath.org/ticket/10132#comment:50
<p>
I forgot to add that I based the patch on sage-4.7.1.alpha3. If you use plain 4.7, also apply <a class="closed ticket" href="https://trac.sagemath.org/ticket/11335" title="enhancement: Allow symbolic vectors to be simplified elementwise (closed: fixed)">#11335</a> and <a class="closed ticket" href="https://trac.sagemath.org/ticket/11381" title="enhancement: Add more simplification methods to symbolic vectors. (closed: fixed)">#11381</a>.
</p>
TicketjvkerschTue, 20 Dec 2011 22:45:35 GMTattachment set
https://trac.sagemath.org/ticket/10132
https://trac.sagemath.org/ticket/10132
<ul>
<li><strong>attachment</strong>
set to <em>trac_10132_final_allfiles_done_no_matrix_simplify.patch</em>
</li>
</ul>
TicketjvkerschTue, 20 Dec 2011 22:52:40 GMTdependencies, description, author changed
https://trac.sagemath.org/ticket/10132#comment:51
https://trac.sagemath.org/ticket/10132#comment:51
<ul>
<li><strong>dependencies</strong>
changed from <em>#10552, #11549</em> to <em>#11549</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/10132?action=diff&version=51">diff</a>)
</li>
<li><strong>author</strong>
changed from <em>Mikhail Malakhaltsev</em> to <em>Mikhail Malakhaltsev, Joris Vankerschaver</em>
</li>
</ul>
<p>
I've simplified some of the methods and updated the documentation to be more verbose. I think this addresses all of Vincent's suggestions: especially the vector simplification patches have made their way into Sage by now, and it turns out that we didn't need the matrix simplification routines, so I've simply removed them.
</p>
<p>
As far as code and documentation goes, the patch is now fairly stable. Comments welcome, and if anybody is willing to play around with the patch, that would be great!
</p>
TicketvdelecroixSun, 29 Jan 2012 16:53:58 GMTcc changed
https://trac.sagemath.org/ticket/10132#comment:52
https://trac.sagemath.org/ticket/10132#comment:52
<ul>
<li><strong>cc</strong>
<em>vdelecroix</em> added
</li>
</ul>
<p>
I just found that sympy (which is a Sage spkg) has a module for tensor calculus in a geometric context :
<a class="ext-link" href="http://docs.sympy.org/0.7.1/modules/galgebra/GA/GAsympy.html"><span class="icon"></span>http://docs.sympy.org/0.7.1/modules/galgebra/GA/GAsympy.html</a>
</p>
<p>
I do not claim that it should be used in some places, I just put a reference for future reviews.
</p>
TicketdavidloefflerTue, 13 Mar 2012 13:14:30 GMT
https://trac.sagemath.org/ticket/10132#comment:53
https://trac.sagemath.org/ticket/10132#comment:53
<p>
Apply trac_10132_final_allfiles_done_no_matrix_simplify.patch
</p>
<p>
(for patchbot)
</p>
TicketdavidloefflerSat, 24 Mar 2012 17:18:51 GMTstatus changed; work_issues set
https://trac.sagemath.org/ticket/10132#comment:54
https://trac.sagemath.org/ticket/10132#comment:54
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>work_issues</strong>
set to <em>doctest failure</em>
</li>
</ul>
<p>
There's a failing doctest on the current beta (see patchbot logs) -- seems to be a numerical precision issue:
</p>
<pre class="wiki">**********************************************************************
File "/storage/masiao/sage-5.0.beta10-patchbot/devel/sage-10132/sage/geometry/riemannian_manifolds/parametrized_surface3d.py", line 1333:
sage: ode.function(0.0, (1.0, 0.0, 1.0, 1.0))
Expected:
[1.00000000000000, 1.00000000000000, -0.45464871341284091, 3.1148154493098041]
Got:
[1.00000000000000, 1.00000000000000, -0.4546487134128409, 3.114815449309804]
</pre>
TicketjvkerschSun, 25 Mar 2012 21:04:56 GMT
https://trac.sagemath.org/ticket/10132#comment:55
https://trac.sagemath.org/ticket/10132#comment:55
<p>
I've updated the patch for sage-5.0.beta10.
</p>
TicketjvkerschSun, 25 Mar 2012 21:06:24 GMTstatus, description changed
https://trac.sagemath.org/ticket/10132#comment:56
https://trac.sagemath.org/ticket/10132#comment:56
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/10132?action=diff&version=56">diff</a>)
</li>
</ul>
TicketdavidloefflerSun, 25 Mar 2012 21:08:47 GMT
https://trac.sagemath.org/ticket/10132#comment:57
https://trac.sagemath.org/ticket/10132#comment:57
<p>
Apply trac_10132_sage-5.beta10.patch
</p>
<p>
(for the patchbot). I can tell you in advance that the patchbot is going to complain about trailing whitespace, which it isn't keen on.
</p>
TicketjvkerschFri, 30 Mar 2012 18:00:20 GMTattachment set
https://trac.sagemath.org/ticket/10132
https://trac.sagemath.org/ticket/10132
<ul>
<li><strong>attachment</strong>
set to <em>trac_10132_sage-5.beta10.patch</em>
</li>
</ul>
<p>
Current version of the patch
</p>
TicketjvkerschFri, 30 Mar 2012 18:09:34 GMT
https://trac.sagemath.org/ticket/10132#comment:58
https://trac.sagemath.org/ticket/10132#comment:58
<p>
I removed all trailing whitespace from the patch. I'm a bit worried this might not be sufficient since this is such a simple algorithmic task. Surely the patchbot can do this by itself? Anyway, let me know if any other modifications are needed -- I'm quite keen on finally finishing off this patch.
</p>
<p>
Vincent, are you still able to review the patch? Thanks!
</p>
TicketjvkerschFri, 30 Mar 2012 18:18:46 GMTwork_issues deleted
https://trac.sagemath.org/ticket/10132#comment:59
https://trac.sagemath.org/ticket/10132#comment:59
<ul>
<li><strong>work_issues</strong>
<em>doctest failure</em> deleted
</li>
</ul>
TicketdavidloefflerThu, 05 Apr 2012 14:15:42 GMT
https://trac.sagemath.org/ticket/10132#comment:60
https://trac.sagemath.org/ticket/10132#comment:60
<p>
Apply trac_10132_sage-5.beta10.patch
</p>
<p>
(for the patchbot)
</p>
TicketvdelecroixSat, 21 Apr 2012 10:17:22 GMTstatus changed
https://trac.sagemath.org/ticket/10132#comment:61
https://trac.sagemath.org/ticket/10132#comment:61
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Hello,
</p>
<p>
Sorry for the (very) long delay between the first and the second review.
</p>
<p>
1) I was not able to build the documentation. Launching "sage -docbuild reference html" gives me an error saying that riemannian_manifols is not found (on sage-5.0.beta7). More precisely, I get "WARNING: toctree contains reference to nonexisting document u'sage/geometry/riemannian_manifolds/parametrized_surface3d"
Then the file for parametrized surfaces is not available in the reference manual. Does it work for you ?
</p>
<p>
2) In order to make it more user friendly I suggest to add some predefined surfaces that may be accessible like in the following example
</p>
<pre class="wiki">sage: surfaces.Sphere(center=(0,0,0), radius=2))
...
</pre><p>
A non exhaustive list of constructor would be: sphere, torus, cylinder, ellipsoid, revolution (for surface of revolution). This will simplify the whole documentation as the examples may be built from those constructor. You may take a look at graphs which allow such feature
</p>
<pre class="wiki">sage: graphs.CycleGraph(4)
Cycle graph: Graph on 4 vertices
</pre><p>
Depending on your feeling, it's possible to put this in another ticket (that I can do).
</p>
<p>
3) You do use a lot of cached_method that are sometimes redundant. As an example first_fundamental_form_coefficients and _compute_first_fundamental_form_coefficient are two cached function. But the first one only calls the latter for different values. This remark also applies to second_fundamental_form_coefficients.
</p>
<p>
4) The most interesting part of your patch is the numerical integration for geodesic and parallel transport. But, you did not put any funny example! It would be interesting to have a plotted example of geodesic (on the sphere, ...) and parallel transport (along a loop on a cone, along two paths joinging the same points on the sphere, ...)
</p>
<p>
Anyway, the patch is very nice.
More to come,
Vincent
</p>
TicketjvkerschTue, 05 Jun 2012 06:31:33 GMT
https://trac.sagemath.org/ticket/10132#comment:62
https://trac.sagemath.org/ticket/10132#comment:62
<p>
Hi Vincent,
</p>
<p>
Thanks for the extensive review and the many insightful comments. I was hoping to have a new version ready by now, but other obligations have come in the way. I hope to have a new version of the patch out in a week or so, but if Mikhail or somebody else wants to do this right now, that would also be fine with me, of course.
</p>
TicketjasonFri, 21 Jun 2013 05:15:06 GMT
https://trac.sagemath.org/ticket/10132#comment:63
https://trac.sagemath.org/ticket/10132#comment:63
<p>
I'm curious: what's the status of this? Is it good enough to get in (keeping in mind that perfect is sometimes the enemy of progress)? I'd hate to see this totally stall.
</p>
TicketjvkerschMon, 24 Jun 2013 20:19:25 GMT
https://trac.sagemath.org/ticket/10132#comment:64
https://trac.sagemath.org/ticket/10132#comment:64
<p>
I actually implemented most of the changes suggested by Vincent and over the summer I even worked with a summer student who extended some of the methods implemented here. So there's a large backlog of code floating around, and I should have gotten around to packaging this up sooner, sorry. I'll try to fix this up as soon as possible.
</p>
TicketnovoseltMon, 24 Jun 2013 20:21:08 GMT
https://trac.sagemath.org/ticket/10132#comment:65
https://trac.sagemath.org/ticket/10132#comment:65
<p>
Would be awesome to have something working in a month or so before the next school year ;-)
</p>
TicketjvkerschWed, 03 Jul 2013 20:45:17 GMT
https://trac.sagemath.org/ticket/10132#comment:66
https://trac.sagemath.org/ticket/10132#comment:66
<p>
I've finalized implementing Vincent's comments, and I'm attaching the result here. I've tested everything under Sage 5.10; the documentation builds cleanly and make ptestlong doesn't report any errors. I've also cleaned up numerous typos and inconsistencies that still lingered around. I think I've incorporated every one of Vincent's suggested improvements and I also added a new class, called Surface3DGenerator, to create instances of standard surfaces (such as the Klein bottle, Boy's surface, etc).
</p>
<p>
Please let me know if you have any questions or if you want more info. I know it has taken a while to finish this, my apologies! In my defense, I moved around quite a bit in the intervening time -- I worked on this during 3 subsequent postdocs all over the globe!
</p>
TicketjvkerschWed, 03 Jul 2013 20:47:42 GMTstatus, description changed
https://trac.sagemath.org/ticket/10132#comment:67
https://trac.sagemath.org/ticket/10132#comment:67
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/10132?action=diff&version=67">diff</a>)
</li>
</ul>
TicketvdelecroixSun, 07 Jul 2013 16:38:28 GMTstatus changed
https://trac.sagemath.org/ticket/10132#comment:68
https://trac.sagemath.org/ticket/10132#comment:68
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Hello,
</p>
<p>
I hope we will have this patch soonly integrated into Sage.
</p>
<p>
0) The patch does not apply anymore on sage-5.11.beta3 because conf.py for the documentation has been modified (the list is now automatically built). You can simply remove your modifications to that file. But your file is still not linked in the reference manual, modify the file 'doc/en/reference/geometry/index.rst' accordingly.
</p>
<p>
1) Many tests fail on my computer because of the random (?) order of terms in a symbolic expression. Here is an example
</p>
<pre class="wiki">File "parametrized_surface3d.py", line 79, in sage.geometry.riemannian_manifolds.parametrized_surface3d.ParametrizedSurface3D
Failed example:
ellipsoid = ParametrizedSurface3D(ellipsoid_eq, coords, 'ellipsoid'); ellipsoid
Expected:
Parametrized surface ('ellipsoid') with equation (cos(u1)*cos(u2), 2*sin(u1)*cos(u2), 3*sin(u2))
Got:
Parametrized surface ('ellipsoid') with equation (cos(u1)*cos(u2), 2*cos(u2)*sin(u1), 3*sin(u2))
</pre><p>
2) in all.py instead of using <code>import</code> you should use <code>lazy_import</code> as follows
</p>
<pre class="wiki">from sage.misc.lazy_import import lazy_import
lazy_import('sage.geometry.riemannian_manifolds.parametrized_surface3d', 'ParametrizedSurface3D')
lazy_import('sage.geometry.riemannian_manifolds.surface3d_generators', 'surfaces')
</pre><p>
The advantage is that it does not affect Sage startup time.
</p>
<p>
3) You use the old framework for errors (which will not be compatible with Python 3), just replace
</p>
<pre class="wiki">raise ValueError, "my error message"
</pre><p>
by
</p>
<pre class="wiki">raise ValueError("my error message")
</pre><p>
4) In the surface generator, because <code>self</code> is never invoked in the core of the method it is better to use the decorator @staticmethod (see <a class="ext-link" href="http://docs.python.org/2/library/functions.html#staticmethod"><span class="icon"></span>Python documentation</a>). You only need to replace
</p>
<pre class="wiki"> def my_generator(self, arg1, arg2, ...):
# bla
</pre><p>
by
</p>
<pre class="wiki"> @staticmethod
def my_generator(arg1, arg2, ...)
# bla
</pre><p>
5) Both attributes <code>index_list</code> and <code>index_list_3</code> are *never* modified by your program nor used very often. I think it would be better to remove them. For iteration, you can use
</p>
<pre class="wiki">sage: from itertools import product
sage: for i,j,k in product((1,2),repeat=3):
....: print i,j,k
1 1 1
1 1 2
...
2 2 2
</pre><p>
and for containance test
</p>
<pre class="wiki">sage: t = (1,3,2)
sage: len(t) == 3 and all(i == 1 or i == 2 or i == 3 for i in t)
True
</pre><p>
6) I was not able to build a point in the parametrization. Let say I use the torus surfaces.Torus(), how do I obtain the coordinates (in <code>R^3</code>) of the point with u=.3 and v=.5 ? It is possible through T.eq_callable but then the name is bad (I was obliged to read the code to find it!) and it has no documentation. In the same veine, given (u,v) and a vector x in <code>R^2</code> how do I get the corresponding tangent vector on the surface ?
</p>
<p>
7) Do you agree to add this example in the doc ?
</p>
<pre class="wiki">sage: S = surfaces.Sphere()
sage: g1 = [c[-1] for c in S.geodesics_numerical((0,0),(1,0),(0,2*pi,100))]
sage: g2 = [c[-1] for c in S.geodesics_numerical((0,0),(cos(pi/3),sin(pi/3)),(0,2*pi,100))]
sage: g3 = [c[-1] for c in S.geodesics_numerical((0,0),(cos(2*pi/3),sin(2*pi/3)),(0,2*pi,100))]
sage: (S.plot(opacity=0.3) + line3d(g1,color='red') + line3d(g2,color='red') + line3d(g3,color='red')).show()
</pre><p>
Actually it would be nice to also add some tangent vectors to that picture.
</p>
TicketjvkerschFri, 12 Jul 2013 02:04:05 GMT
https://trac.sagemath.org/ticket/10132#comment:69
https://trac.sagemath.org/ticket/10132#comment:69
<p>
Hi Vincent,
</p>
<p>
Thanks a lot for your swift and incisive comments, which have improved the code a lot! I've implemented all of them, but I noticed that <a class="closed ticket" href="https://trac.sagemath.org/ticket/12737" title="enhancement: Remove simplify_radical() from simplify_full() (closed: fixed)">#12737</a> breaks some of the doctests in this patch, and I'm currently updating the code to take that into account. I should have something ready by the end of the weekend, though.
</p>
TicketjvkerschTue, 16 Jul 2013 04:41:58 GMTstatus, dependencies changed
https://trac.sagemath.org/ticket/10132#comment:70
https://trac.sagemath.org/ticket/10132#comment:70
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>dependencies</strong>
changed from <em>#11549</em> to <em>#11549, #12737</em>
</li>
</ul>
<p>
Hi Vincent, thanks again for your awesome comments. I have implemented all of the changes you suggested, and I briefly describe the most significant changes below.
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10132#comment:68" title="Comment 68">vdelecroix</a>:
</p>
<blockquote class="citation">
<p>
0) ... your file is still not linked in the reference manual, modify the file 'doc/en/reference/geometry/index.rst' accordingly.
</p>
</blockquote>
<p>
I haven't included the files in 'geometry/index.rst' as that file seems to deal with combinatorial geometry mostly. Instead, I've linked from the front page of the documentation, and I've put an entry under 'geometry and topology'.
</p>
<blockquote class="citation">
<p>
1) Many tests fail on my computer because of the random (?) order of terms in a symbolic expression.
</p>
</blockquote>
<p>
I think this is a consequence of some changes in the behavior of multivariate symbolic expressions: I used to have no problems like this for the 3 years that I've been testing this patch, but with a reasonably recent version of Sage I see the same problems that you notice. I've therefore taken the liberty of simply adapting the doctests, trusting that the new ordering of the terms is the default behavior from now on.
</p>
<blockquote class="citation">
<p>
6) I was not able to build a point in the parametrization.
</p>
</blockquote>
<p>
I've added methods <code>point</code> and <code>tangent_vector</code>, which do what you suggested. Adding these methods also had the advantage of making the implementation somewhat clearer: I've now removed <code>eq_callable</code> (which was not very clear to begin with) and replaced its invocation with calls to the method <code>point()</code>.
</p>
<blockquote class="citation">
<p>
7) Do you agree to add this example in the doc ?
</p>
</blockquote>
<p>
This is a very nice and illustrative example; I've added it to the class documentation.
</p>
<p>
Some other changes that I've made include anticipating <a class="closed ticket" href="https://trac.sagemath.org/ticket/12737" title="enhancement: Remove simplify_radical() from simplify_full() (closed: fixed)">#12737</a>, which removes <code>simplify_radical</code> from <code>simplify_full</code>. I found that we actually rely on <code>simplify_radical</code> a lot, among other things to get rid of expressions like <code>sqrt(x^2)/x' for </code>x > 0'. I've implemented a small helper function to call both simplification methods successively. For the rest, I've also made tons of little changes to the doctests.
</p>
<p>
Let me know if you need more information.
</p>
TicketvdelecroixTue, 16 Jul 2013 07:24:40 GMT
https://trac.sagemath.org/ticket/10132#comment:71
https://trac.sagemath.org/ticket/10132#comment:71
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10132#comment:70" title="Comment 70">jvkersch</a>:
</p>
<p>
Sounds cool! I will have a look as soon as possible.
</p>
<p>
Vincent
</p>
TicketjdemeyerTue, 13 Aug 2013 15:35:53 GMTmilestone changed
https://trac.sagemath.org/ticket/10132#comment:72
https://trac.sagemath.org/ticket/10132#comment:72
<ul>
<li><strong>milestone</strong>
changed from <em>sage-5.11</em> to <em>sage-5.12</em>
</li>
</ul>
TicketchapotonFri, 30 Aug 2013 19:34:28 GMT
https://trac.sagemath.org/ticket/10132#comment:73
https://trac.sagemath.org/ticket/10132#comment:73
<p>
for the bot, instructions should be given here as follows:
</p>
<p>
apply only trac_10132_revised_sage-5.10.patch
</p>
TicketvdelecroixSat, 19 Oct 2013 23:12:20 GMTstatus changed
https://trac.sagemath.org/ticket/10132#comment:74
https://trac.sagemath.org/ticket/10132#comment:74
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
Hi,
</p>
<p>
Sorry for the long time between the reviews. I think that the patch is ready to go and has wait too much. The patch applies on 5.12.rc0 and test succeded.
</p>
<p>
I just leave further remarks for next improvement
</p>
<p>
1) I found that you use way too much cached_method (12 occurrences). Their usage is expensive precisely because they cache the result. It should be use only in time critical part of the code. I would suppress it from: rotation, _create_geodesic_ode_system.
</p>
<p>
2) the code here is in its most part compatible with sage-manifolds (<a class="closed ticket" href="https://trac.sagemath.org/ticket/14865" title="enhancement: SageManifolds: a package for differential geometry in Sage (closed: wontfix)">#14865</a>). There should be a merge at some point. Sage manifolds allows to have multiple charts and hence to have more embedded surfaces!
</p>
TicketjdemeyerSun, 20 Oct 2013 21:00:23 GMTdescription changed
https://trac.sagemath.org/ticket/10132#comment:75
https://trac.sagemath.org/ticket/10132#comment:75
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/10132?action=diff&version=75">diff</a>)
</li>
</ul>
TicketjvkerschSun, 20 Oct 2013 21:03:45 GMT
https://trac.sagemath.org/ticket/10132#comment:76
https://trac.sagemath.org/ticket/10132#comment:76
<p>
Hi Vincent, thanks for the review! I will look into the issue with the cached methods soon. A possible merge with sage-manifolds is definitely a good idea, and high on my todo-list, but I haven't had a chance to explore that code in depth just yet.
</p>
TicketjdemeyerMon, 21 Oct 2013 06:29:14 GMTreviewer changed
https://trac.sagemath.org/ticket/10132#comment:77
https://trac.sagemath.org/ticket/10132#comment:77
<ul>
<li><strong>reviewer</strong>
changed from <em>vdelecroix</em> to <em>Vincent Delecroix</em>
</li>
</ul>
TicketjdemeyerThu, 31 Oct 2013 08:20:42 GMTstatus changed
https://trac.sagemath.org/ticket/10132#comment:78
https://trac.sagemath.org/ticket/10132#comment:78
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
There is a problem with plotting on powerpc64 (skynet system <code>silius</code>):
</p>
<pre class="wiki">sage -t --long devel/sage/sage/geometry/riemannian_manifolds/surface3d_generators.py
**********************************************************************
File "devel/sage/sage/geometry/riemannian_manifolds/surface3d_generators.py", line 123, in sage.geometry.riemannian_manifolds.surface3d_generators.SurfaceGenerators.Dini
Failed example:
dini.plot()
Expected nothing
Got:
sh: line 1: 46907 Segmentation fault tachyon /home/buildbot/.sage/temp/silius/46889/tmp_JMygGS.dat -format PNG -o /home/buildbot/.sage/temp/silius/46889/tmp.png -res 10 10 > /dev/null
**********************************************************************
1 item had failures:
1 of 3 in sage.geometry.riemannian_manifolds.surface3d_generators.SurfaceGenerators.Dini
[30 tests, 1 failure, 2.77 s]
sage -t --long devel/sage/sage/rings/finite_rings/hom_finite_field.pyx
[143 tests, 1.19 s]
sage -t --long devel/sage/sage/tests/french_book/float_doctest.py
[74 tests, 1.70 s]
sage -t --long devel/sage/sage/databases/oeis.py
[110 tests, 2.81 s]
sage -t --long devel/sage/sage/geometry/riemannian_manifolds/parametrized_surface3d.py
**********************************************************************
File "devel/sage/sage/geometry/riemannian_manifolds/parametrized_surface3d.py", line 121, in sage.geometry.riemannian_manifolds.parametrized_surface3d.ParametrizedSurface3D
Failed example:
dini.plot(aspect_ratio='automatic')
Expected nothing
Got:
sh: line 1: 46954 Segmentation fault tachyon /home/buildbot/.sage/temp/silius/46888/tmp_gl9WUA.dat -format PNG -o /home/buildbot/.sage/temp/silius/46888/tmp.png -res 10 10 > /dev/null
**********************************************************************
</pre><p>
I also ask you to rebase the patch to a more recent version of Sage, as there is some fuzz when applying the patch.
</p>
TicketvdelecroixThu, 31 Oct 2013 11:35:30 GMT
https://trac.sagemath.org/ticket/10132#comment:79
https://trac.sagemath.org/ticket/10132#comment:79
<p>
Hi Jeroen,
</p>
<p>
I have no clue of what have happend with tachyon... moreover when I launch it on my computer a Jmol applet is launching... are we responsible of Segementation fault ?
</p>
<p>
Thanks
Vincent
</p>
TicketjdemeyerThu, 31 Oct 2013 16:21:16 GMT
https://trac.sagemath.org/ticket/10132#comment:80
https://trac.sagemath.org/ticket/10132#comment:80
<p>
On Solaris SPARC also:
</p>
<pre class="wiki">sage -t --long devel/sage/sage/geometry/riemannian_manifolds/surface3d_generators.py
**********************************************************************
File "devel/sage/sage/geometry/riemannian_manifolds/surface3d_generators.py", line 123, in sage.geometry.riemannian_manifolds.surface3d_generators.SurfaceGenerators.Dini
Failed example:
dini.plot()
Expected nothing
Got:
Bus Error - core dumped
**********************************************************************
</pre><p>
This could very well be a bug in Tachyon, but it's strange that no other 3D plots fail doctests like this.
</p>
TicketjdemeyerThu, 31 Oct 2013 16:33:09 GMT
https://trac.sagemath.org/ticket/10132#comment:81
https://trac.sagemath.org/ticket/10132#comment:81
<pre class="wiki">sage: print surfaces.Dini().plot(aspect_ratio='automatic').tachyon()
</pre><p>
shows a lot of <code>nan</code> values, perhaps this is the problem.
</p>
TicketjdemeyerThu, 31 Oct 2013 16:50:30 GMT
https://trac.sagemath.org/ticket/10132#comment:82
https://trac.sagemath.org/ticket/10132#comment:82
<p>
Feel free to add <code># known bug</code> to those plots...
</p>
TicketjvkerschSat, 16 Nov 2013 22:40:54 GMTattachment set
https://trac.sagemath.org/ticket/10132
https://trac.sagemath.org/ticket/10132
<ul>
<li><strong>attachment</strong>
set to <em>trac_10132_revised_sage-5.10.patch</em>
</li>
</ul>
<p>
Revised version of the patch
</p>
TicketjvkerschSat, 16 Nov 2013 22:47:55 GMTstatus changed
https://trac.sagemath.org/ticket/10132#comment:83
https://trac.sagemath.org/ticket/10132#comment:83
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
Hey guys, thanks for the input. It looks like Dini is the only surface that has any <code>nans</code> in its representation, so I labeled just that one as known bug. I've also rebased the patch against sage-5.13-beta2 (but forgot to update the filename).
</p>
<p>
Vincent, I'm intrigued by what you say about cached methods. As any geometric computation ultimately involves lots of coefficients of the fundamental forms, I found that caching those really speeds up things. Can you clarify a bit?
</p>
<p>
I'll leave the patch as needs-review in the meantime.
</p>
TicketjdemeyerFri, 22 Nov 2013 14:44:01 GMT
https://trac.sagemath.org/ticket/10132#comment:84
https://trac.sagemath.org/ticket/10132#comment:84
<p>
There is still
</p>
<pre class="wiki">sage -t --long devel/sage/sage/geometry/riemannian_manifolds/parametrized_surface3d.py
**********************************************************************
File "devel/sage/sage/geometry/riemannian_manifolds/parametrized_surface3d.py", line 121, in sage.geometry.riemannian_manifolds.parametrized_surface3d.ParametrizedSurface3D
Failed example:
dini.plot(aspect_ratio='automatic')
Expected nothing
Got:
sh: line 1: 33457 Segmentation fault tachyon /home/buildbot/.sage/temp/silius/33443/tmp_2ZapEy.dat -format PNG -o /home/buildbot/.sage/temp/silius/33443/tmp.png -res 10 10 > /dev/null
**********************************************************************
</pre>
TicketjdemeyerFri, 22 Nov 2013 17:46:15 GMTstatus changed
https://trac.sagemath.org/ticket/10132#comment:85
https://trac.sagemath.org/ticket/10132#comment:85
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
TicketjvkerschSat, 23 Nov 2013 15:55:00 GMTattachment set
https://trac.sagemath.org/ticket/10132
https://trac.sagemath.org/ticket/10132
<ul>
<li><strong>attachment</strong>
set to <em>trac_10132_revised_sage-5.13.beta2.patch</em>
</li>
</ul>
TicketjvkerschSat, 23 Nov 2013 15:55:41 GMTstatus, description changed
https://trac.sagemath.org/ticket/10132#comment:86
https://trac.sagemath.org/ticket/10132#comment:86
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/10132?action=diff&version=86">diff</a>)
</li>
</ul>
TicketjvkerschSat, 23 Nov 2013 15:55:59 GMT
https://trac.sagemath.org/ticket/10132#comment:87
https://trac.sagemath.org/ticket/10132#comment:87
<p>
Thanks, I forgot about that one.
</p>
TicketjdemeyerThu, 05 Dec 2013 08:02:19 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/10132#comment:88
https://trac.sagemath.org/ticket/10132#comment:88
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>merged</strong>
set to <em>sage-5.13.beta5</em>
</li>
</ul>
Ticket