Opened 10 years ago

Closed 7 years ago

#10132 closed enhancement (fixed)

Parametrization of (metric) surfaces in 3D euclidean space

Reported by: mikarm Owned by: mikarm
Priority: major Milestone: sage-5.13
Component: geometry Keywords: differential geometry, parametrized surface
Cc: jvkersch, mhampton, niles, jason, novoselt, maldun, mpatel, vdelecroix Merged in: sage-5.13.beta5
Authors: Mikhail Malakhaltsev, Joris Vankerschaver Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #11549, #12737 Stopgaps:

Status badges

Description (last modified by jvkersch)

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 trac_10132_revised_sage-5.13.beta2.patch

Attachments (20)

mikarm_ticket_files.zip (744.5 KB) - added by mikarm 10 years ago.
parametrized_curve3d class and example worksheets
trac_10132_differential_geometry_sage.patch (40.6 KB) - added by jvkersch 10 years ago.
Code of Mikhail as a Sage patch
Parametrized Curve_ Computing Geodesics.sws (9.1 KB) - added by jvkersch 10 years ago.
Worksheet to illustrate caching for the computation of geodesics
trac_10132_differential_geometry_sage_v2.patch (51.9 KB) - added by jvkersch 10 years ago.
New version of the patch
ParametrizedSurface3D_ Examples.sws (10.3 KB) - added by jvkersch 10 years ago.
parametrized_surface3d.py (50.3 KB) - added by mikarm 10 years ago.
version 2 of parametrized_surface3d.py with comments
trac_10132_differential_geometry_sage_v3.patch (43.5 KB) - added by jvkersch 10 years ago.
parametrized_surface3d.2.py (40.1 KB) - added by mikarm 10 years ago.
parametrized_surface3d.py with small changes in parallel_translation_numeric_new
parallel_translation.sws (1.6 KB) - added by mikarm 10 years ago.
an example of usage of the method parallel_translation_numerical_new
trac_10132_differential_geometry_sage_v3.2.patch (40.8 KB) - added by jvkersch 10 years ago.
parametrized_surface3d.3.py (40.4 KB) - added by mikarm 10 years ago.
Next version of ParametrizedSurface3D class
Minimal_surfaces_and_surface_of_revolution.sws (51.9 KB) - added by mikarm 10 years ago.
Examples of calculations for minimal surfaces and some calculations for the surfaces of revolution in general form
trac_10132_diff_geom_sage_doctests.patch (55.9 KB) - added by jvkersch 10 years ago.
Latest version of the patch, with 100% coverage and doctests in place.
trac_10132_final.patch (25.5 KB) - added by mikarm 10 years ago.
trac_10132_final_all_files_done.patch (61.5 KB) - added by mikarm 10 years ago.
trac_10123_final_allfiles_done.patch (58.7 KB) - added by jvkersch 10 years ago.
trac_10132_final_allfiles_done_no_matrix_simplify.patch (60.5 KB) - added by jvkersch 9 years ago.
trac_10132_sage-5.beta10.patch (59.2 KB) - added by jvkersch 9 years ago.
Current version of the patch
trac_10132_revised_sage-5.10.patch (82.4 KB) - added by jvkersch 7 years ago.
Revised version of the patch
trac_10132_revised_sage-5.13.beta2.patch (82.4 KB) - added by jvkersch 7 years ago.

Download all attachments as: .zip

Change History (108)

Changed 10 years ago by mikarm

parametrized_curve3d class and example worksheets

comment:1 Changed 10 years ago by jason

  • Cc jvkersch mhampton niles jason added

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! :)

comment:2 Changed 10 years ago by novoselt

  • Cc novoselt added

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.

comment:3 Changed 10 years ago by jvkersch

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.

comment:4 follow-up: Changed 10 years ago by mikarm

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?

comment:5 in reply to: ↑ 4 Changed 10 years ago by novoselt

Replying to 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.)

comment:6 Changed 10 years ago by jvkersch

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.
  1. 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.

Changed 10 years ago by jvkersch

Code of Mikhail as a Sage patch

comment:7 Changed 10 years ago by mikarm

  • Description modified (diff)

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.

comment:8 Changed 10 years ago by maldun

  • Cc maldun added

comment:9 Changed 10 years ago by mpatel

  • Cc mpatel added

comment:10 follow-up: Changed 10 years ago by jvkersch

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]
    
  4. 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.

Changed 10 years ago by jvkersch

Worksheet to illustrate caching for the computation of geodesics

comment:11 in reply to: ↑ 10 Changed 10 years ago by jvkersch

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.

comment:12 Changed 10 years ago by jvkersch

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.

comment:13 Changed 10 years ago by mikarm

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

comment:14 Changed 10 years ago by jvkersch

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.

comment:15 Changed 10 years ago by jvkersch

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.

Changed 10 years ago by jvkersch

New version of the patch

Changed 10 years ago by jvkersch

comment:16 Changed 10 years ago by mikarm

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.

Changed 10 years ago by mikarm

version 2 of parametrized_surface3d.py with comments

comment:17 Changed 10 years ago by jvkersch

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...

comment:18 Changed 10 years ago by mikarm

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

Changed 10 years ago by mikarm

parametrized_surface3d.py with small changes in parallel_translation_numeric_new

Changed 10 years ago by mikarm

an example of usage of the method parallel_translation_numerical_new

comment:19 follow-up: Changed 10 years ago by 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. 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.

comment:20 in reply to: ↑ 19 ; follow-up: Changed 10 years ago by jason

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.

comment:21 in reply to: ↑ 20 Changed 10 years ago by jvkersch

Replying to jason:

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...

comment:22 Changed 10 years ago by mikarm

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

Changed 10 years ago by mikarm

Next version of ParametrizedSurface3D class

Changed 10 years ago by mikarm

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

comment:23 Changed 10 years ago by jvkersch

  • Status changed from new to needs_work

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.

comment:24 Changed 10 years ago by jvkersch

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.

comment:25 follow-up: Changed 10 years ago by mhampton

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

comment:26 in reply to: ↑ 25 Changed 10 years ago by jvkersch

Replying to 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.

comment:27 Changed 10 years ago by jvkersch

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/.

comment:28 follow-up: Changed 10 years ago by mikarm

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.

comment:29 Changed 10 years ago by mikarm

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()?

comment:30 Changed 10 years ago by jvkersch

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!

comment:31 in reply to: ↑ 28 Changed 10 years ago by jvkersch

Replying to 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.

comment:32 Changed 10 years ago by jason

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

hg qimport http://trac.sagemath.org/sage_trac/raw-attachment/ticket/10132/trac_10132_diff_geom_sage_doctests.patch
hg qpush

Changed 10 years ago by jvkersch

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

comment:33 Changed 10 years ago by jvkersch

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.
  1. 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.

comment:34 Changed 10 years ago by mikarm

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 R2 and R3.

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.

comment:35 Changed 10 years ago by mikarm

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 :)

comment:36 Changed 10 years ago by jvkersch

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.

comment:37 Changed 10 years ago by mikarm

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

comment:38 Changed 10 years ago by mikarm

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.

Changed 10 years ago by mikarm

comment:39 Changed 10 years ago by mikarm

  • Description modified (diff)
  • Status changed from needs_work to needs_review

comment:40 Changed 10 years ago by mikarm

  • Owner changed from mhampton to mikarm

Changed 10 years ago by mikarm

comment:41 Changed 10 years ago by mikarm

  • Description modified (diff)

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!

comment:42 Changed 10 years ago by mikarm

  • Description modified (diff)

comment:43 Changed 10 years ago by novoselt

  • Description modified (diff)
  • Milestone set to sage-4.7

Made a patchbot-friendly description.

comment:44 Changed 10 years ago by vdelecroix

  • Description modified (diff)
  • Reviewers set to vdelecroix
  • Status changed from needs_review to needs_work
  • Summary changed from Differential Geometry via Sage to Parametrization of (metric) surfaces in 3D euclidean space

Hello,

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

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.

1) patch errors

  • 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
  • I get a minor error for patching index.rst in the doc with sage-4.6.2

2) name logic

  • There is a directory sage/geometry/ which is the directory intended for... geometry. It would be better to put all Riemannian geometry inside.
  • "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.
  • 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.

3) about a class for generic manifolds: I do not think the class GenericManifold? 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 EmbeddedRiemannianManifold?. And if so, you have to implement a class for morphisms between RiemannianManifolds?... it will be painful !

4) In your file you mention "PatchOfCoordinates?" 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.

comment:45 Changed 10 years ago by vdelecroix

  • Description modified (diff)

comment:46 Changed 10 years ago by jvkersch

Bonjour Vincent,

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.

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.

I need a few more days to clean up the code, and I will post an updated patch shortly.

With best wishes, Joris

comment:47 Changed 10 years ago by jvkersch

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 #10552 and #11335, respectively.

comment:48 Changed 10 years ago by jvkersch

  • Dependencies set to #10552

I removed the simplification code for vectors and matrices, moved the files to a subfolder of sage.geometry and smoothed out some parts of the code. Doctests seem to pass and the documentation builds cleanly.

Changed 10 years ago by jvkersch

comment:49 Changed 10 years ago by jvkersch

  • Dependencies changed from #10552 to #10552, #11549
  • Status changed from needs_work to needs_review

comment:50 Changed 10 years ago by jvkersch

I forgot to add that I based the patch on sage-4.7.1.alpha3. If you use plain 4.7, also apply #11335 and #11381.

comment:51 Changed 9 years ago by jvkersch

  • Authors changed from Mikhail Malakhaltsev to Mikhail Malakhaltsev, Joris Vankerschaver
  • Dependencies changed from #10552, #11549 to #11549
  • Description modified (diff)

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.

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!

comment:52 Changed 9 years ago by vdelecroix

  • Cc vdelecroix added

I just found that sympy (which is a Sage spkg) has a module for tensor calculus in a geometric context : http://docs.sympy.org/0.7.1/modules/galgebra/GA/GAsympy.html

I do not claim that it should be used in some places, I just put a reference for future reviews.

comment:53 Changed 9 years ago by davidloeffler

Apply trac_10132_final_allfiles_done_no_matrix_simplify.patch

(for patchbot)

comment:54 Changed 9 years ago by davidloeffler

  • Status changed from needs_review to needs_work
  • Work issues set to doctest failure

There's a failing doctest on the current beta (see patchbot logs) -- seems to be a numerical precision issue:

**********************************************************************
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]

comment:55 Changed 9 years ago by jvkersch

I've updated the patch for sage-5.0.beta10.

comment:56 Changed 9 years ago by jvkersch

  • Description modified (diff)
  • Status changed from needs_work to needs_review

comment:57 Changed 9 years ago by davidloeffler

Apply trac_10132_sage-5.beta10.patch

(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.

Changed 9 years ago by jvkersch

Current version of the patch

comment:58 Changed 9 years ago by jvkersch

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.

Vincent, are you still able to review the patch? Thanks!

comment:59 Changed 9 years ago by jvkersch

  • Work issues doctest failure deleted

comment:60 Changed 9 years ago by davidloeffler

Apply trac_10132_sage-5.beta10.patch

(for the patchbot)

comment:61 Changed 9 years ago by vdelecroix

  • Status changed from needs_review to needs_work

Hello,

Sorry for the (very) long delay between the first and the second review.

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 ?

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

sage: surfaces.Sphere(center=(0,0,0), radius=2))
...

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

sage: graphs.CycleGraph(4) 
Cycle graph: Graph on 4 vertices

Depending on your feeling, it's possible to put this in another ticket (that I can do).

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.

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, ...)

Anyway, the patch is very nice. More to come, Vincent

comment:62 Changed 9 years ago by jvkersch

Hi Vincent,

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.

comment:63 Changed 8 years ago by jason

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.

comment:64 Changed 8 years ago by jvkersch

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.

comment:65 Changed 8 years ago by novoselt

Would be awesome to have something working in a month or so before the next school year ;-)

comment:66 Changed 8 years ago by jvkersch

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).

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!

comment:67 Changed 8 years ago by jvkersch

  • Description modified (diff)
  • Status changed from needs_work to needs_review

comment:68 follow-up: Changed 8 years ago by vdelecroix

  • Status changed from needs_review to needs_work

Hello,

I hope we will have this patch soonly integrated into Sage.

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.

1) Many tests fail on my computer because of the random (?) order of terms in a symbolic expression. Here is an example

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))

2) in all.py instead of using import you should use lazy_import as follows

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')

The advantage is that it does not affect Sage startup time.

3) You use the old framework for errors (which will not be compatible with Python 3), just replace

raise ValueError, "my error message"

by

raise ValueError("my error message")

4) In the surface generator, because self is never invoked in the core of the method it is better to use the decorator @staticmethod (see Python documentation). You only need to replace

    def my_generator(self, arg1, arg2, ...):
        # bla

by

    @staticmethod
    def my_generator(arg1, arg2, ...)
        # bla

5) Both attributes index_list and index_list_3 are *never* modified by your program nor used very often. I think it would be better to remove them. For iteration, you can use

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

and for containance test

sage: t = (1,3,2)
sage: len(t) == 3 and all(i == 1 or i == 2 or i == 3 for i in t)
True

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 R^3) 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 R^2 how do I get the corresponding tangent vector on the surface ?

7) Do you agree to add this example in the doc ?

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()

Actually it would be nice to also add some tangent vectors to that picture.

comment:69 Changed 8 years ago by jvkersch

Hi Vincent,

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 #12737 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.

comment:70 in reply to: ↑ 68 ; follow-up: Changed 8 years ago by jvkersch

  • Dependencies changed from #11549 to #11549, #12737
  • Status changed from needs_work to needs_review

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.

Replying to vdelecroix:

0) ... your file is still not linked in the reference manual, modify the file 'doc/en/reference/geometry/index.rst' accordingly.

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'.

1) Many tests fail on my computer because of the random (?) order of terms in a symbolic expression.

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.

6) I was not able to build a point in the parametrization.

I've added methods point and tangent_vector, which do what you suggested. Adding these methods also had the advantage of making the implementation somewhat clearer: I've now removed eq_callable (which was not very clear to begin with) and replaced its invocation with calls to the method point().

7) Do you agree to add this example in the doc ?

This is a very nice and illustrative example; I've added it to the class documentation.

Some other changes that I've made include anticipating #12737, which removes simplify_radical from simplify_full. I found that we actually rely on simplify_radical a lot, among other things to get rid of expressions like sqrt(x^2)/x' for 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.

Let me know if you need more information.

comment:71 in reply to: ↑ 70 Changed 8 years ago by vdelecroix

Replying to jvkersch:

Sounds cool! I will have a look as soon as possible.

Vincent

comment:72 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:73 Changed 8 years ago by chapoton

for the bot, instructions should be given here as follows:

apply only trac_10132_revised_sage-5.10.patch

comment:74 Changed 7 years ago by vdelecroix

  • Status changed from needs_review to positive_review

Hi,

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.

I just leave further remarks for next improvement

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.

2) the code here is in its most part compatible with sage-manifolds (#14865). There should be a merge at some point. Sage manifolds allows to have multiple charts and hence to have more embedded surfaces!

comment:75 Changed 7 years ago by jdemeyer

  • Description modified (diff)

comment:76 Changed 7 years ago by jvkersch

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.

comment:77 Changed 7 years ago by jdemeyer

  • Reviewers changed from vdelecroix to Vincent Delecroix

comment:78 Changed 7 years ago by jdemeyer

  • Status changed from positive_review to needs_work

There is a problem with plotting on powerpc64 (skynet system silius):

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
**********************************************************************

I also ask you to rebase the patch to a more recent version of Sage, as there is some fuzz when applying the patch.

comment:79 Changed 7 years ago by vdelecroix

Hi Jeroen,

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 ?

Thanks Vincent

comment:80 Changed 7 years ago by jdemeyer

On Solaris SPARC also:

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
**********************************************************************

This could very well be a bug in Tachyon, but it's strange that no other 3D plots fail doctests like this.

comment:81 Changed 7 years ago by jdemeyer

sage: print surfaces.Dini().plot(aspect_ratio='automatic').tachyon()

shows a lot of nan values, perhaps this is the problem.

comment:82 Changed 7 years ago by jdemeyer

Feel free to add # known bug to those plots...

Changed 7 years ago by jvkersch

Revised version of the patch

comment:83 Changed 7 years ago by jvkersch

  • Status changed from needs_work to needs_review

Hey guys, thanks for the input. It looks like Dini is the only surface that has any nans 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).

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?

I'll leave the patch as needs-review in the meantime.

comment:84 Changed 7 years ago by jdemeyer

There is still

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
**********************************************************************

comment:85 Changed 7 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Changed 7 years ago by jvkersch

comment:86 Changed 7 years ago by jvkersch

  • Description modified (diff)
  • Status changed from needs_work to needs_review

comment:87 Changed 7 years ago by jvkersch

Thanks, I forgot about that one.

comment:88 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.13.beta5
  • Resolution set to fixed
  • Status changed from needs_review to closed
Note: See TracTickets for help on using tickets.