Construction of a vector frame from a family of vector fields

egourgoulhon commented 5 years ago

This ticket modifies DifferentiableManifold.vector_frame() to allow for constructing a vector frame from a spanning family of linearly independent vector fields:

sage: M = Manifold(2, 'M')
sage: X.<x,y> = M.chart()
sage: e0 = M.vector_field(1+x^2, 1+y^2)
sage: e1 = M.vector_field(2, -x*y)
sage: e = M.vector_frame('e', (e0, e1)); e
Vector frame (M, (e_0,e_1))
sage: e[0].display()
e_0 = (x^2 + 1) d/dx + (y^2 + 1) d/dy
sage: e[1].display()
e_1 = 2 d/dx - x*y d/dy
sage: (e[0], e[1]) == (e0, e1)
True

Previously, the only way to introduce the vector frame e was to first introduce the automorphism relating the frame (d/dx, d/dy) to (e0, e1) and to pass this automorphism to VectorFrame.new_frame():

sage: aut = M.automorphism_field()
sage: aut[:] = [[e0[0], e1[0]], [e0[1], e1[1]]]
sage: e = X.frame().new_frame(aut, 'e')

Implementation details: such functionality already existed for bases of finite rank free modules; the relevant code is extracted from the method FiniteRankFreeModule.basis() and put into the new method FreeModuleBasis._init_from_family(), in order to be used in DifferentiableManifold.vector_frame() as well.

CC: @tscrim

Component: geometry

Keywords: vector_frame

Author: Eric Gourgoulhon

Branch/Commit: 81e2f60

Reviewer: Michael Jung

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

egourgoulhon commented 5 years ago

New commits:

013fb8b Add construction of a vector frame from a family of vector fields

egourgoulhon commented 5 years ago

Commit: 013fb8b

egourgoulhon commented 5 years ago

Branch: public/manifolds/vector_frame_from_family-28716

egourgoulhon commented 5 years ago

Description changed:

--- 
+++ 
@@ -14,7 +14,7 @@
 sage: (e[0], e[1]) == (e0, e1)
 True

-Previously, the only way to introduce the vector frame e was to first introduce an automorphism relating the frame (d/dx, d/dy) to (e0, e1) and to pass this automorphism to VectorFrame.new_frame(): +Previously, the only way to introduce the vector frame e was to first introduce the automorphism relating the frame (d/dx, d/dy) to (e0, e1) and to pass this automorphism to VectorFrame.new_frame():

 sage: aut = M.automorphism_field()

tscrim commented 5 years ago

comment:4

Do we need the optional parameter? Basically, can we just use the fact that a tuple/list is being given and then assume it is suppose to be a family of vector fields? If it has to be a keyword, I would change from_family to the more descriptive from_vector_fields.

b220a61f-8dcb-4d62-9a53-d56a2b1bfc8f commented 5 years ago

comment:5

This is a great idea and would be very useful for vector bundles, too. Sometimes I got really annoyed by this detour. Would you mind to adapt your code, if working, for vector bundles as-well?

By the way: We should combine vector bundles and the previous implementations really really soon (in this case inherit vector frames from local frames) otherwise things could get extremly messy.

Unfortunately, I am quite busy working at my master thesis right now. I can almost feel the deadline touching my skin. I promise to work on that as soon as I've gained some time back.

Even though I don't have the time now, I've opened the corresponding ticket #28718, just to keep this task in mind.

egourgoulhon commented 5 years ago

comment:6

Replying to @tscrim:

Thanks for your prompt feedback.

Do we need the optional parameter? Basically, can we just use the fact that a tuple/list is being given and then assume it is suppose to be a family of vector fields?

Good idea, this is much more user-friendly! I am on it...

egourgoulhon commented 5 years ago

comment:7

Replying to @DeRhamSource:

This is a great idea and would be very useful for vector bundles, too. Sometimes I got really annoyed by this detour.

Yes, this should have been done sooner...

Would you mind to adapt your code, if working, for vector bundles as-well?

OK, I'll try to do this (see below).

By the way: We should combine vector bundles and the previous implementations really really soon (in this case inherit vector frames from local frames) otherwise things could get extremly messy.

Yes, I agree. Note however that this ticket does not touch the class VectorFrame, only the user interface DifferentiableManifold.vector_frame(). I'll perform a similar change to the interfaces TopologicalVectorBundle.local_frame() and TensorBundle.local_frame().

Unfortunately, I am quite busy working at my master thesis right now. I can almost feel the deadline touching my skin.

Good luck with your master thesis!

I promise to work on that as soon as I've gained some time back.

Even though I don't have the time now, I've opened the corresponding ticket #28718, just to keep this task in mind.

Thanks.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from 013fb8b to d0ef4d7

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

`71b5f0f`	`Replace keyword argument 'from_family' by positional argument in vector_frame()`
`d0ef4d7`	`Add construction of a local frame from a set of vector fields in TensorBundle.local_frame()`

egourgoulhon commented 5 years ago

comment:9

In the latest version (cf. comment:8 commits)

vector_frame() accepts a tuple/list of vector fields as a positional argument, the keyword argument from_family being suppressed, following the suggestion made in comment:4.
The ZeroDivisionError that occurs if the vector fields are not linearly independent (the exception is raised when computing the inverse of the automorphism relating the new frame to a previous one) is cached with a proper error message.
The documentation of the module sage.manifolds.differentiable.vectorframe has been updated to take into account the new functionality.
TensorBundle.local_frame() has been updated to offer the same functionality, following comment:5.
A TODO section has been added to TopologicalVectorBundle.local_frame() for implementing a similar functionality with local sections in the future.

I propose to stay here for this ticket, i.e. to let the modification of TopologicalVectorBundle.local_frame() to a future ticket (#28718 ?). This is mostly to avoid code duplication with DifferentiableManifold.vector_frame(), waiting for a clearer view of #28718. Besides, I will be extremely busy in the coming weeks and I would like very much the vector_frame() functionality introduced in the current ticket to make its way in Sage 9.0.

egourgoulhon commented 5 years ago

Description changed:

--- 
+++ 
@@ -1,11 +1,11 @@
-This ticket introduces the keyword argument `from_family` to `DifferentiableManifold.vector_frame()` to allow for constructing a vector frame from a spanning family of linearly independent vector fields:
+This ticket modifies `DifferentiableManifold.vector_frame()` to allow for constructing a vector frame from a spanning family of linearly independent vector fields:

sage: M = Manifold(2, 'M') sage: X.<x,y> = M.chart() sage: e0 = M.vector_field(1+x^2, 1+y^2) sage: e1 = M.vector_field(2, -x*y) -sage: e = M.vector_frame('e', from_family=(e0, e1)); e +sage: e = M.vector_frame('e', (e0, e1)); e Vector frame (M, (e_0,e_1)) sage: e[0].display() e_0 = (x^2 + 1) d/dx + (y^2 + 1) d/dy @@ -21,6 +21,6 @@ sage: aut[:] = [[e0[0], e1[0]], [e0[1], e1[1]]] sage: e = X.frame().new_frame(aut, 'e')

-The introduction of `from_family` in `vector_frame()` simplifies this process.
+

 **Implementation details:** such functionality already existed for bases of finite rank free modules; the relevant code is extracted from the method `FiniteRankFreeModule.basis()` and put into the new method `FreeModuleBasis._init_from_family()`, in order to be used in `DifferentiableManifold.vector_frame()` as well.

egourgoulhon commented 5 years ago

comment:10

Do you agree with the above changes (comment:9)?

b220a61f-8dcb-4d62-9a53-d56a2b1bfc8f commented 5 years ago

comment:11

Partially. Of course, we can postpone this to another ticket, preferrably to #28718. However, I think this fits in here perfectly well and enables the feature for vector bundles in Sage 9.0 without too much effort [1] as you already did it for the tensor bundle. But I have no strong opinion on that so just do as you prefer.

One personal reformulation:

-        any connection with previously defined vector frames or vector fields
-        (the connection can be performed later via the method
+        connecting it to previously defined vector frames or vector fields
+        (this can still be performed later via the method

[1] at least so far as I can see

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from d0ef4d7 to 6635fad

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

6635fad Construction of a local frame on a vector bundle from a family of sections

egourgoulhon commented 5 years ago

comment:13

Replying to @DeRhamSource:

Partially. Of course, we can postpone this to another ticket, preferrably to #28718. However, I think this fits in here perfectly well and enables the feature for vector bundles in Sage 9.0 without too much effort [1] as you already did it for the tensor bundle.

OK I've done it in the above commit. I've also improved the catching of the error in case of linearly dependent elements.

One personal reformulation:

-        any connection with previously defined vector frames or vector fields
-        (the connection can be performed later via the method
+        connecting it to previously defined vector frames or vector fields
+        (this can still be performed later via the method

Done as well.

b220a61f-8dcb-4d62-9a53-d56a2b1bfc8f commented 5 years ago

comment:14

Thank you so much! I'll give it a further look this or tomorrow afternoon.

It seems, we are at beta6 now.

b220a61f-8dcb-4d62-9a53-d56a2b1bfc8f commented 5 years ago

comment:15

I gave it some short tests. This is a huge improvement for using frames! Thanks! :)

Just a minor thing:

-          ``self`` (`n` being the dimension of ``self``) defining the local
+          ``self`` (`n` being the rank of ``self``) defining the local

Apart from that it looks fine to me. As soon as you merged the recent develop branch into this one and patchbot says "yes", I could give it a positive review. Travis?

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from 6635fad to 81e2f60

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

`5c9c516`	`Merge branch 'public/manifolds/vector_frame_from_family-28716' of git://trac.sagemath.org/sage into Sage 9.0.beta6`
`81e2f60`	`Minor fix in the documentation of TopologicalVectorBundle.local_frame()`

egourgoulhon commented 5 years ago

comment:17

Replying to @DeRhamSource:

Just a minor thing:

-          ``self`` (`n` being the dimension of ``self``) defining the local
+          ``self`` (`n` being the rank of ``self``) defining the local

Thanks for pointing this; it is corrected in the above commit.

b220a61f-8dcb-4d62-9a53-d56a2b1bfc8f commented 5 years ago

comment:18

There is one thing I'm not sure about. Namely the line:

            mat = [[c[[i]] for c in comps] for i in fmodule.irange()]
            aut.add_comp(basis)[:] = mat
   this --> aut.add_comp(self)[:] = mat
            fmodule.set_change_of_basis(basis, self, aut)

in free_module_basis.py.

Shouldn't it be the identity matrix with respect to that basis? Or did I get something wrong?

egourgoulhon commented 5 years ago

comment:19

Replying to @DeRhamSource:

There is one thing I'm not sure about. Namely the line:
            mat = [[c[[i]] for c in comps] for i in fmodule.irange()]
            aut.add_comp(basis)[:] = mat
   this --> aut.add_comp(self)[:] = mat
            fmodule.set_change_of_basis(basis, self, aut)
in free_module_basis.py.

Shouldn't it be the identity matrix with respect to that basis? Or did I get something wrong?

The formula is correct: it should not be the identity matrix but the matrix of the change-of-basis automorphism, which has the same expression in both bases.

b220a61f-8dcb-4d62-9a53-d56a2b1bfc8f commented 5 years ago

comment:20

Replying to @egourgoulhon:

The formula is correct: it should not be the identity matrix but the matrix of the change-of-basis automorphism, which has the same expression in both bases.

Yeah, you're absolutely right. I thought it through once again and come to the same conclusion now. Furthermore, some tests on this did work properly. Sorry!

So from my perspective, everything is fine. I'll give it a positive review.

b220a61f-8dcb-4d62-9a53-d56a2b1bfc8f commented 5 years ago

Reviewer: Michael Jung

egourgoulhon commented 5 years ago

comment:21

Thank you for the review!

vbraun commented 4 years ago

comment:22

Merge conflict

egourgoulhon commented 4 years ago

comment:23

Replying to @vbraun:

Merge conflict

There is no conflict with the just released 9.0.beta7. Is it a conflict with #27784 (which is not merged yet)?

b220a61f-8dcb-4d62-9a53-d56a2b1bfc8f commented 4 years ago

comment:24

Replying to @egourgoulhon:

Replying to @vbraun:

Merge conflict

There is no conflict with the just released 9.0.beta7. Is it a conflict with #27784 (which is not merged yet)?

Not sure. But to run some tests for my thesis, I merged these two tickets and a conflict occured. Just a very minor thing about lines if I remember correctly.

However, either ticket needs to be merged to be certain.

egourgoulhon commented 4 years ago

comment:25

Replying to @DeRhamSource:

Replying to @egourgoulhon:

Replying to @vbraun:

Merge conflict

There is no conflict with the just released 9.0.beta7. Is it a conflict with #27784 (which is not merged yet)?

Not sure. But to run some tests for my thesis, I merged these two tickets and a conflict occured. Just a very minor thing about lines if I remember correctly.

Yes most of the time these conflicts due to various developments performed in parallel are very minor and easy to solve.

However, either ticket needs to be merged to be certain.

Yes. I am afraid we have to wait for the next beta to solve this...

tscrim commented 4 years ago

comment:26

Since you basically know what ticket, I would just merge that in and set this back to positive review with that as a dependency.

b220a61f-8dcb-4d62-9a53-d56a2b1bfc8f commented 4 years ago

comment:27

Replying to @tscrim:

Since you basically know what ticket, I would just merge that in and set this back to positive review with that as a dependency.

Good idea. I'll do that for #27784. I need to add a minor thing into the documentation of characteristic classes anyway.

b220a61f-8dcb-4d62-9a53-d56a2b1bfc8f commented 4 years ago

comment:28

Replying to @DeRhamSource:

Replying to @tscrim:

Since you basically know what ticket, I would just merge that in and set this back to positive review with that as a dependency.

Good idea. I'll do that for #27784. I need to add a minor thing into the documentation of characteristic classes anyway.

Done.

egourgoulhon commented 4 years ago

comment:29

Replying to @DeRhamSource:

Replying to @DeRhamSource:

Replying to @tscrim:

Since you basically know what ticket, I would just merge that in and set this back to positive review with that as a dependency.

Good idea. I'll do that for #27784. I need to add a minor thing into the documentation of characteristic classes anyway.

Done.

Thanks!

I am then setting this ticket back to positive review and will have a look at #27784.

egourgoulhon commented 4 years ago

comment:30

According to comment:29.

vbraun commented 4 years ago

Changed branch from public/manifolds/vector_frame_from_family-28716 to 81e2f60

sagemath / sage

Construction of a vector frame from a family of vector fields #28716