Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enter the components of a metric while declaring #25457

Open
sagetrac-Dicolevrai mannequin opened this issue May 28, 2018 · 28 comments
Open

Enter the components of a metric while declaring #25457

sagetrac-Dicolevrai mannequin opened this issue May 28, 2018 · 28 comments

Comments

@sagetrac-Dicolevrai
Copy link
Mannequin

sagetrac-Dicolevrai mannequin commented May 28, 2018

The main purpose of this ticket is to give the possibility of entering the components of a metric while declaring this metric on a manifold.

In this ticket, there is a method which determine the signature of a metric. So it is no more necessary to give the signature of your metric while declaring, Sage is able to compute it! Therefore Sage would give the truth class of the declaring metric: Riemannian metric or Lorentzian metric or pseudo-Riemannian metric or degenerate metric.

Depends on #0

CC: @egourgoulhon

Component: geometry

Keywords: Pseudo-Riemannian Metric, Degenerate Metric

Author: Hans Fotsing Tetsing

Branch/Commit: public/manifolds/MetricComponents @ f494678

Reviewer: Eric Gourgoulhon

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

@sagetrac-Dicolevrai sagetrac-Dicolevrai mannequin added this to the sage-8.3 milestone May 28, 2018
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 28, 2018

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

7dfaad9Enter components of a metric while creating the metric

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 28, 2018

Changed commit from 52ea821 to 7dfaad9

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 31, 2018

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

188a1c3Enter the component of a metric while declaring the metric on a Parallelizable Manifold

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 31, 2018

Changed commit from 7dfaad9 to 188a1c3

@sagetrac-Dicolevrai sagetrac-Dicolevrai mannequin changed the title Enter the component of a metric while declaring the metric on a Parallelizable Manifold Enter the components of a metric while declaring the metric on a Parallelizable Manifold May 31, 2018
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 31, 2018

Changed commit from 188a1c3 to 33edd0e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 31, 2018

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

33edd0eEnter the components of a metric while declaring this metric on a Parallelizable Manifold

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 31, 2018

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

76e40c5Enter the components of a metric while declaring this metric on a Parallelizable Manifold

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 31, 2018

Changed commit from 33edd0e to 76e40c5

@egourgoulhon
Copy link
Member

comment:7

Thanks for this contribution!

As you can see on the patchbot report
(which you can get by clicking on the yellow question mark at the top right and then on the "shortlog" items), there are many doctest failures, mostly due to the change in handling the metric signature. Could you fix this please? Before submitting the change, you can make sure that all doctests are passed by running

sage -tp --long src/sage/manifolds/

In case you need more information about the patchbot, see here.

@sagetrac-Dicolevrai
Copy link
Mannequin Author

sagetrac-Dicolevrai mannequin commented Jun 6, 2018

New commits:

52ea821Enter the component of a meric while declaring the metric on a Parallelizable Manifold
7dfaad9Enter components of a metric while creating the metric
188a1c3Enter the component of a metric while declaring the metric on a Parallelizable Manifold
33edd0eEnter the components of a metric while declaring this metric on a Parallelizable Manifold
76e40c5Enter the components of a metric while declaring this metric on a Parallelizable Manifold

@sagetrac-Dicolevrai
Copy link
Mannequin Author

sagetrac-Dicolevrai mannequin commented Jun 6, 2018

Changed branch from public/manifolds/Metric to public/manifolds/MetricComponents

@sagetrac-Dicolevrai
Copy link
Mannequin Author

sagetrac-Dicolevrai mannequin commented Jun 6, 2018

Changed commit from 76e40c5 to none

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 6, 2018

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

52ea821Enter the component of a meric while declaring the metric on a Parallelizable Manifold
7dfaad9Enter components of a metric while creating the metric
188a1c3Enter the component of a metric while declaring the metric on a Parallelizable Manifold
25cff3cEnter the component of a metric while declaring the metric on a Parallelizable Manifold
a406fe6Solving some doctests errors

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 6, 2018

Commit: a406fe6

@egourgoulhon
Copy link
Member

comment:12

A doctest fails:

sage -t --long --warn-long 49.9 src/sage/manifolds/differentiable/metric.py
**********************************************************************
File "src/sage/manifolds/differentiable/metric.py", line 1006, in sage.manifolds.differentiable.metric.PseudoRiemannianMetric.riemann
Failed example:
    g.riemann()[:]
Expected:
    [[[[0, 0], [0, 0]], [[0, sin(th)^2], [-sin(th)^2, 0]]],
    [[[0, (cos(th)^2 - 1)/sin(th)^2], [1, 0]], [[0, 0], [0, 0]]]]
Got:
    [[[[0, 0], [0, 0]], [[0, sin(th)^2], [-sin(th)^2, 0]]],
     [[[0, -1], [1, 0]], [[0, 0], [0, 0]]]]
**********************************************************************
1 item had failures:
   1 of  15 in sage.manifolds.differentiable.metric.PseudoRiemannianMetric.riemann
    [534 tests, 1 failure, 86.67 s]

Besides, the following doctest in line 168-170 of src/sage/manifolds/differentiable/pseudo_riemannian.py:

sage: g = M.metric()
sage: g
Riemannian metric g on the 4-dimensional Lorentzian manifold M

is not correct: the output should be Lorentzian metric (as it was prior to this ticket), since M has been declared in line 158 as being a Lorentzian manifold.

Similarly, in lines 566-567 of the same file, the output of the doctest

sage: h = M.metric('h', signature=1); h
Riemannian metric h on the 3-dimensional Riemannian manifold M

is not correct, since a signature of 1 on a 3-dimensional manifold implies a Lorentzian metric.

In the helper function _diag defined in line 1908 of src/sage/manifolds/differentiable/metric.py, it is quite unconventional to name the first argument self since _diag is not a class method.

You should not end docstring lines by a backslash character, as done in lines 1976-1982 of metric.py.

The documentation fails to build: the command

./sage -docbuild reference/manifolds html

returns errors like

docstring of sage.manifolds.differentiable.metric.PseudoRiemannianMetricParal.index:7: 
WARNING: Bullet list ends without a blank line; unexpected unindent.

@egourgoulhon
Copy link
Member

comment:13

Another point: you should fill the Description field of this ticket, to tell what it is about.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 15, 2018

Changed commit from a406fe6 to a6071cb

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 15, 2018

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

a6071cbEntering components of a metric while declaring the metric on a parallelizable manifold

@sagetrac-Dicolevrai

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 18, 2018

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

8ddc1b2Merge branch 'public/manifolds/MetricComponents' of git://trac.sagemath.org/sage into Sage 8.3.beta4
cae28a7Reviewer tweaks for #25457

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 18, 2018

Changed commit from a6071cb to cae28a7

@egourgoulhon
Copy link
Member

comment:17

I've pushed to the ticket branch a commit with small tweaks, which

  • correct errors in the documentation building
  • correct the doctest failure mentioned in comment:12 (trigonometric simplification)
  • apply some coding style along the recommended PEP 8 conventions, in particular: "The preferred way of wrapping long lines is by using Python's implied line continuation inside parentheses, brackets and braces. Long lines can be broken over multiple lines by wrapping expressions in parentheses. These should be used in preference to using a backslash for line continuation."

To see all my changes, click on cae28a7 in comment:16.

There are still some points that must be discussed or fixed:

  • At the moment, if no metric components are provided at some metric declaration (i.e. if comp is None), this defaults to initializing the metric components to diag(1,1,...,1). It would be preferable to left the components uninitialized instead, especially if the default chart is not of Cartesian type. I think we can make both behaviors (the previous one, with no components preinitialized, and the new one, with components set via the argument comp=...) coexist nicely
  • Since the argument comp is implemented only in PseudoRiemannianMetricParal.__init__, there is a difference of behavior between parallelizable manifolds and non-parallelizable ones; the argument comp has been added to VectorFieldModule.metric but is totally unused there.
  • Even on parallelizable manifolds, what if more than one chart is required to cover the manifold, like for instance S1 or S3 ?
  • The issue with the naming of the first argument of _diag mentioned in comment:12 is not addressed
  • The argument comp should be documented everywhere it has been added (e.g. in VectorFieldFreeModule.metric)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 27, 2018

Changed commit from cae28a7 to f494678

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 27, 2018

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

f494678Entering the components of a metric while declaring

@sagetrac-Dicolevrai
Copy link
Mannequin Author

sagetrac-Dicolevrai mannequin commented Jun 27, 2018

Changed keywords from Parallelizable Manifold, Pseudo-Riemannian Metric, Degenerate Metric to Pseudo-Riemannian Metric, Degenerate Metric

@sagetrac-Dicolevrai

This comment has been minimized.

@sagetrac-Dicolevrai
Copy link
Mannequin Author

sagetrac-Dicolevrai mannequin commented Jun 27, 2018

Changed author from Hans FOTSING TETSING to Hans Fotsing Tetsing

@sagetrac-Dicolevrai sagetrac-Dicolevrai mannequin changed the title Enter the components of a metric while declaring the metric on a Parallelizable Manifold Enter the components of a metric while declaring Jun 27, 2018
@egourgoulhon
Copy link
Member

comment:20

There are various issues:

  • The doctest time of metric.py is too long: on my computer, running
sage -t --long src/sage/manifolds/differentiable/metric.py

yields a total CPU time of 144.2 s, while with Sage 8.3.rc0 (i.e. outside the ticket branch) it is only of 62.3 s. Such an increase of doctest time is not acceptable, especially for a new functionality, which in principle, does not require much computing.
One can see the culprits by running

sage -t --long --warn-long 5 src/sage/manifolds/differentiable/metric.py

which reports the doctests running for more than 5 s. They are the doctests involving a metric on the sphere, with the components given in 6 charts. Why do you choose such a lengthy example? It is also very long, and thus not pleasant to read, in the html documentation.

  • In line 726 of levi_civita_connection.py, you have changed the output of the doctest by replacing Lorentzian metric by Riemannian metric (so that the doctest is passed), but this not correct since the metric has been declared as Lorentzian a few lines above. Actually this reveals that the handling of metric signatures must be entirely rethinked after the introduction of your function for computing the signature (previously, there was only the concept of "user declared signature"). This is out of the scope of this ticket and I will open a new ticket for this.

  • The method sign() should return a tuple, and not a list, since the signature is intended to be immutable.

  • The documentation does not build, due to a spurious u99 inserted in line 3216 of src/sage/manifolds/differentiable/manifold.py. On general grounds, please check that the documentation builds correctly, via

sage -docbuild reference/manifolds html

and that the html output is OK, before submitting a ticket for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants