sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.45k stars 481 forks source link

Pullback silently fails in some cases with multiple charts #31904

Closed egourgoulhon closed 3 years ago

egourgoulhon commented 3 years ago

In Sage 9.3, we have

sage: E.<x,y> = EuclideanSpace()
sage: polar.<r,ph> = E.polar_coordinates()
sage: g = E.metric()
sage: M = Manifold(1, 'M')
sage: Ct.<t> = M.chart()
sage: F = M.diff_map(E, coord_functions={(Ct, polar): (1 + cos(t), t)})
sage: gM = F.pullback(g)
sage: gM
Field of symmetric bilinear forms on the 1-dimensional differentiable
manifold M

So far so good, but

sage: gM.display()
ValueError: no basis could be found for computing the components in 
 the Coordinate frame (M, (d/dt)

Actually, gM has been initialized as a tensor field object, but its components have not been evaluated in any frame:

sage: gM._components
{}

Forcing the coordinate expression of the map F in the Cartesian chart (for instance by a call to F.display()) fixes the issue:

sage: F.display()
M --> E^2
   t |--> (x, y) = (cos(t)^2 + cos(t), (cos(t) + 1)*sin(t))
   t |--> (r, ph) = (cos(t) + 1, t)
sage: gM = F.pullback(g)
sage: gM.display()
(2*cos(t) + 2) dt*dt

However, the expression of F in Cartesian coordinates should not be required to compute the pullback of g since the latter is known in polar coordinates, where F has been defined:

sage: g.display(polar)
g = dr*dr + r^2 dph*dph

This bug has been reported at https://ask.sagemath.org/question/57431/

CC: @tscrim @mjungmath @mkoeppe

Component: manifolds

Keywords: pullback

Author: Eric Gourgoulhon

Branch/Commit: aea4554

Reviewer: Ricardo Buring

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

egourgoulhon commented 3 years ago

Description changed:

--- 
+++ 
@@ -19,7 +19,7 @@
 ValueError: no basis could be found for computing the components in 
  the Coordinate frame (M, (d/dt)

-Actually, gM has not been initialized as a tensor field object, but its components have not been evaluated in any frame: +Actually, gM has been initialized as a tensor field object, but its components have not been evaluated in any frame:

 sage: gM._components
egourgoulhon commented 3 years ago

Description changed:

--- 
+++ 
@@ -36,7 +36,7 @@
 sage: gM.display()
 (2*cos(t) + 2) dt*dt

-However, the expression of F in Cartesian coordinates should not be required to compute the pullback of g since the latter is known in polar coordinates: +However, the expression of F in Cartesian coordinates should not be required to compute the pullback of g since the latter is known in polar coordinates, where F has been defined:

 sage: g.display(polar)
egourgoulhon commented 3 years ago

New commits:

8455aabFix bug #31904 in pullback
egourgoulhon commented 3 years ago

Author: Eric Gourgoulhon

egourgoulhon commented 3 years ago

Commit: 8455aab

egourgoulhon commented 3 years ago

Branch: public/manifolds/pullback_bug-31904

egourgoulhon commented 3 years ago
comment:4

The fix consisted in making the internal function _pullback_chart of the method pullback to operate for a single pair of charts (now added as arguments), which is determined in the main part of pullback, based on the knowledge of the map's coordinate expressions.

rburing commented 3 years ago
comment:5

The return partial statement in the parallel code has seemingly accidentally been indented too far.

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

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

aea4554#31904: Fix indentation in _pullback_chart
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 8455aab to aea4554

egourgoulhon commented 3 years ago
comment:7

Replying to @rburing:

The return partial statement in the parallel code has seemingly accidentally been indented too far.

Good catch, thanks! (it was not revealed by the parallel doctest because local_list_ind had a single element in that case). This is corrected in the above commit (as well as a pyflakes error reported by the patchbot).

egourgoulhon commented 3 years ago
comment:8

Ricardo, do you agree with the last version?

rburing commented 3 years ago

Reviewer: Ricardo Buring

rburing commented 3 years ago
comment:9

Yes, looks good now.

egourgoulhon commented 3 years ago
comment:10

Thank you!

vbraun commented 3 years ago

Changed branch from public/manifolds/pullback_bug-31904 to aea4554