Add symplectic structures

tobiasdiez commented 4 years ago

This ticket implements the basics of symplectic structures, like Poisson brackets and Hamiltonian vector fields.

TODO (as follow-up tickets):

Extract general coordinate stuff from EuclideanSpace to new class VectorSpace, and let SymplecticVectorSpace derive from VectorSpace

CC: @tscrim @nthiery @mjungmath @egourgoulhon @mkoeppe

Component: manifolds

Author: Tobias Diez

Branch: b78d8a2

Reviewer: Eric Gourgoulhon, Michael Jung, Matthias Koeppe, Travis Scrimshaw

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

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

Changed commit from fbc1af8 to bf56226

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

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

bf56226 #30362: minimal fix of docstrings to have reference manual built

egourgoulhon commented 2 years ago

comment:75

While taking a look to the ticket, I've noticed that the relevant documentation was not generated because there was no entry about symplectic structures in Sage's reference manual. In the above commit, I have therefore added the new file

src/doc/en/reference/manifolds/poisson_manifold.rst

and inserted a reference to it in index.rst in the same directory. Then building the documentation failed due to various errors, mostly

EXAMPLES: to be replaced by EXAMPLES:: when immediately followed by sage: ...
.. MATH:: to be followed by a blank line
the bibliographic references [AM1990] and [RS2007] are missing in the biblio file I fixed those so that the documentation now builds. Note that at the moment (i.e. until #32946 is merged), one cannot use sage -docbuild reference/manifolds html to quickly generate the relevant part of the reference manual. One has to use make instead, which takes much longer time.

For the references [AM1990] and [RS2007], I simply turned them into literal text so that the documentation could build; could you please add them in the bibliographic file

src/doc/en/reference/references/index.rst

egourgoulhon commented 2 years ago

comment:76

A few comments:

The file src/sage/all_cmdline.py should not be touched by this ticket.
As stated in comment:2, the changes in TopologicalManifold.__init__ and DifferentiableManifold.__init__ should be reverted.
In the initialization
```
sage: M.<q, p> = EuclideanSpace(2, "R2", r"\mathbb{R}^2", symbols=r"q:q p:p")
```
the arguments 2 and symbols are not necessary. This can be shortened to
```
sage: M.<q,p> = EuclideanSpace(name="R2", latex_name=r"\mathbb{R}^2")
```
Even better, for the sake of clarity, you could use the default in these doctests, since neither the name nor the LaTeX name of the Euclidean space appears:
```
sage: M.<q,p> = EuclideanSpace()
```
See here for details.

In the examples, the following two lines

sage: from sage.manifolds.differentiable.poisson_tensor import PoissonTensorField
sage: poisson = PoissonTensorField(M, 'varpi')

should be replaced by

poisson = M.poisson_tensor(name='varpi')

since this is the way the end user should use initialize a Poisson tensor.

Similarly

sage: from sage.manifolds.differentiable.symplectic_form import SymplecticForm
sage: omega = SymplecticForm(M, 'omega', r'\omega')

should be replaced by

sage: omega = M.symplectic_form(name='omega', latex_name=r'\omega')

mjungmath commented 2 years ago

comment:77

Something a bit off-topic, but mentioned here a couple of months ago:

Replying to @tobiasdiez:

I think, it's strange if the constructor requires more data than actually is required. Although that might be a matter of taste, but I find M = TopologicalManifold(2, 'M') more readable than M = Manifold(2, 'M', structure='topological').

On a second thought (after this time) I tend to agree with Tobias, though I also see the point of cluttering Sage's namespace.

What if we use the namespace manifolds, i.e. manifolds.TopologicalManifold, manifolds.SymplecticManifold, ..., instead? We already have manifolds.Sphere, manifolds.RealLine, ..., and it seems quite natural to me adding the above to this namespace, too. This way, we do not clutter the global namespace but maintain the desired convenience. Eric, what do you think?

Of course, this shouldn't be treated in this ticket, rather in a follow-up.

egourgoulhon commented 2 years ago

comment:78

Replying to @mjungmath:

What if we use the namespace manifolds, i.e. manifolds.TopologicalManifold, manifolds.SymplecticManifold, ..., instead? We already have manifolds.Sphere, manifolds.RealLine, ..., and it seems quite natural to me adding the above to this namespace, too. This way, we do not clutter the global namespace but maintain the desired convenience. Eric, what do you think?

Yes, why not?

Of course, this shouldn't be treated in this ticket, rather in a follow-up.

Certainly.

tobiasdiez commented 2 years ago

comment:79

Thanks for the feedback. I've incorporated your suggestions and updated the code.

Still have to check for the docs, since building them takes adges...

Concerning

In the examples, the following two lines

sage: from sage.manifolds.differentiable.poisson_tensor import PoissonTensorField sage: poisson = PoissonTensorField(M, 'varpi')

should be replaced by

poisson = M.poisson_tensor(name='varpi')

since this is the way the end user should use initialize a Poisson tensor.

I've changed it, but think that it still would be useful to have the examples directly for the constructor. It's similar to what I've mentioned above, but why should M.poisson_tensor be superior to PoissonTensor(M)? Sure for a normal sage user the first is more convient, as you don't have to import anything. But if you use sage more as a python library, than the latter is maybe your preferred way.

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

Changed commit from bf56226 to 3f390f8

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

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

3f390f8 Incorporate feedback

tscrim commented 2 years ago

comment:81

Replying to @tobiasdiez:

Concerning
In the examples, the following two lines
sage: from sage.manifolds.differentiable.poisson_tensor import PoissonTensorField
sage: poisson = PoissonTensorField(M, 'varpi')
should be replaced by
poisson = M.poisson_tensor(name='varpi')
since this is the way the end user should use initialize a Poisson tensor.
I've changed it, but think that it still would be useful to have the examples directly for the constructor. It's similar to what I've mentioned above, but why should M.poisson_tensor be superior to PoissonTensor(M)? Sure for a normal sage user the first is more convient, as you don't have to import anything. But if you use sage more as a python library, than the latter is maybe your preferred way.

It is something that is associated to the manifold, therefore the API should favor the method. (Contrast this to the exterior algebra, which is not associated to its defining base ring.) Plus it means you do not have to care about it being parallelizeable or not. Not only for front-end users because of the lack of import, but we also want people using it as a library to use the method. It also helps avoid a bit circular import hell.

Remove the pytest test and replace them with standard tests. The disassociation with the doctests makes it very easy to miss full coverage (to the point where I have to do a lot of very detailed checking), there are no examples right there in the methods for a future developer to quickly look at, it is a heavier maintenance burden to keep things in sync, and it adds clutter. I also do not want to keep a uniform approach to doctests at this point until there is a large consensus to adopt such a change. This also removes the otherwise unnecessary dependency on #32975.

egourgoulhon commented 2 years ago

comment:82

Replying to @tscrim:

Remove the pytest test and replace them with standard tests. The disassociation with the doctests makes it very easy to miss full coverage (to the point where I have to do a lot of very detailed checking), there are no examples right there in the methods for a future developer to quickly look at, it is a heavier maintenance burden to keep things in sync, and it adds clutter. I also do not want to keep a uniform approach to doctests at this point until there is a large consensus to adopt such a change. This also removes the otherwise unnecessary dependency on #32975.

I fully agree: introducing the pytest framework in Sage does not belong to this ticket. It would be a pity to have to wait for #32975 for having symplectic structures in Sage. Similarly, I don't think #32953 should be a dependency.

mjungmath commented 2 years ago

comment:83

Replying to @tscrim:

Remove the pytest test and replace them with standard tests. The disassociation with the doctests makes it very easy to miss full coverage (to the point where I have to do a lot of very detailed checking), there are no examples right there in the methods for a future developer to quickly look at, it is a heavier maintenance burden to keep things in sync, and it adds clutter. I also do not want to keep a uniform approach to doctests at this point until there is a large consensus to adopt such a change. This also removes the otherwise unnecessary dependency on #32975.

+1

Replying to @egourgoulhon:

I fully agree: introducing the pytest framework in Sage does not belong to this ticket. It would be a pity to have to wait for #32975 for having symplectic structures in Sage. Similarly, I don't think #32953 should be a dependency.

+1

tobiasdiez commented 2 years ago

Changed dependencies from #32953, #32975 to none

tobiasdiez commented 2 years ago

comment:84

Pytest is already an optional package for a long time, and it has been integrated in the usual sage -t command in #31003. So this ticket here is only using existing infrastructure. Note also that pytest is not essentially used - it only provides consistency and more detailed tests of special cases, every method still has "normal" examples in their documentation. The longterm idea is to make pytest a standard package, see #31110. But for this one needs to collect experience with its usage in sage. For example, as you point out the coverage tools needs improvement to be still useful with pytest. I've opened #32994 to keep track of this. Thanks for the suggestion.

I thought ticket dependencies are a natural element of the sage workflow. Since you collectively don't liked them, I've now added a few doctests instead of using the proposed TEST: pytest syntax proposed in #32975. Once this ticket is merged one could revert to that syntax as a follow-up. The dependency on #32953 can indeed be removed thanks to the fix by Eric.

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

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

7d98bd4 Remove dependency on other ticket

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

Changed commit from 3f390f8 to 7d98bd4

tscrim commented 2 years ago

comment:86

Thank you for making the changes.

Dependencies are a natural thing, but they still should be relevant to the ticket at-hand. Imposing a dependency because you want to use a new feature you care about is not a good practice IMO. (I feel it is more akin to hostage diplomacy.) To gain experience in using something, one should do a ticket that changes some part of Sage that is more stable that can be compared (or perhaps one that needs some attention because it is older code/doc).

tobiasdiez commented 2 years ago

comment:87

Replying to @tscrim:

Imposing a dependency because you want to use a new feature you care about is not a good practice IMO. (I feel it is more akin to hostage diplomacy.)

Sorry that it made this impression. It was actually the other way around: while working on this ticket here, I created #32975 to make my life easier...

To gain experience in using something, one should do a ticket that changes some part of Sage that is more stable that can be compared.

That's also done, see for example #30738 which awaits review/feedback.

slel commented 2 years ago

Description changed:

--- 
+++ 
@@ -1,6 +1,9 @@
-This PR adds symplectic structures. The basic things like Poisson brackets and Hamiltonian vector fields are implemented. 
+This ticket implements the basics of symplectic structures,
+like Poisson brackets and Hamiltonian vector fields.

 TODO (as follow-up tickets):
-- Extract general coordinate stuff from EuclideanSpace to new class VectorSpace, and let SymplecticVectorSpace derive from VectorSpace
+- Extract general coordinate stuff from `EuclideanSpace`
+  to new class `VectorSpace`, and let `SymplecticVectorSpace`
+  derive from `VectorSpace`

egourgoulhon commented 2 years ago

Reviewer: Eric Gourgoulhon, Michael Jung, Travis Scrimshaw

egourgoulhon commented 2 years ago

comment:89

Thanks for the changes. It seems that we are converging ;-)

A few more points:

The macro \contr in the docstring of hamiltonian_vector_field is not recognized by LaTeX: this produces a red literal \contr in the html documentation and breaks the pdf documentation, as you can easily check by running sage -docbuild reference/manifolds pdf (this works, no need to wait for #32946, which regards only the html documentation).
The docstring of SymplecticFormParal shall not start by TODO
The unnecessary import from sage.manifolds.structure import RealDifferentialStructure should be removed from differentiable/manifold.py.
The line containing _dim: int should be removed from manifolds/manifold.py (in other words, the file manifolds/manifold.py should not be touched by this ticket)
It seems to me that the files symplectic_form_test.py and symplectic_vector_space_test.py should be removed from this ticket, until #32975 is settled. They are triggering coverage and puflakes errors and are no used in the current test framework.

tobiasdiez commented 2 years ago

comment:90

Replying to @egourgoulhon:

The macro \contr in the docstring of hamiltonian_vector_field is not recognized by LaTeX: this produces a red literal \contr in the html documentation and breaks the pdf documentation, as you can easily check by running sage -docbuild reference/manifolds pdf (this works, no need to wait for #32946, which regards only the html documentation).

Should be fixed now. I cannot test this, as building the PDF also doesn't work for me: RuntimeError: failed to run $MAKE all-pdf in /home/tobias/sage/local/share/doc/sage/latex/en/reference/manifolds

The docstring of SymplecticFormParal shall not start by TODO

Nicely spotted!

The unnecessary import from sage.manifolds.structure import RealDifferentialStructure should be removed from differentiable/manifold.py.

Done!

The line containing _dim: int should be removed from manifolds/manifold.py (in other words, the file manifolds/manifold.py should not be touched by this ticket)

It's needed in order to not get typing issues in the new symplectic forms file. I know that there is no check in place (yet?) to verify that all (new) code adheres to typing standards, but I think its good practice to let the typing evolve as one works on the code. Also typing errors are shown as errors for me in VS and as I will be probably the person working on the symplectic code the most in the near future, I would strongly prefer to have such errors as little as possible.

It seems to me that the files symplectic_form_test.py and symplectic_vector_space_test.py should be removed from this ticket, until #32975 is settled. They are triggering coverage and puflakes errors and are no used in the current test framework.

They are invoked by ./sage -t (if pytest is installed in the sage venv) and run without issues (at least for me). The sage.all imports that pyflakes is complaining about are needed due to various cyclic imports. #30741 started the work to remove these cyclic imports, but more work is needed.

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

Changed commit from 7d98bd4 to 902bf29

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

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

902bf29 Further changes based on feedback

egourgoulhon commented 2 years ago

comment:92

Yet two more:

the INPUT blocks of __init__ methods shall be moved to the main docstring of the class; otherwise, they don't appear in the documentation. Similarly for the EXAMPLES section. See e.g. what is done indiff_form.py. Actually, only TESTS should appear in the docstring of __init__ since, as for any private method, its docstring does not appear to the generated documentation.
the docstring of hodge_star is still in the stage of a copy/paste from the metric one; it should be adapted to the symplectic form.

New commits:

902bf29 Further changes based on feedback

egourgoulhon commented 2 years ago

comment:93

Replying to @tobiasdiez:

Replying to @egourgoulhon:

The macro \contr in the docstring of hamiltonian_vector_field is not recognized by LaTeX: this produces a red literal \contr in the html documentation and breaks the pdf documentation, as you can easily check by running sage -docbuild reference/manifolds pdf (this works, no need to wait for #32946, which regards only the html documentation).

Should be fixed now.

Yes it is, thanks.

I cannot test this, as building the PDF also doesn't work for me: RuntimeError: failed to run $MAKE all-pdf in /home/tobias/sage/local/share/doc/sage/latex/en/reference/manifolds

Strange... It works for me. Don't you have a more precise error message?

The line containing _dim: int should be removed from manifolds/manifold.py (in other words, the file manifolds/manifold.py should not be touched by this ticket)

It's needed in order to not get typing issues in the new symplectic forms file. I know that there is no check in place (yet?) to verify that all (new) code adheres to typing standards, but I think its good practice to let the typing evolve as one works on the code. Also typing errors are shown as errors for me in VS and as I will be probably the person working on the symplectic code the most in the near future, I would strongly prefer to have such errors as little as possible.

OK, very well.

It seems to me that the files symplectic_form_test.py and symplectic_vector_space_test.py should be removed from this ticket, until #32975 is settled. They are triggering coverage and puflakes errors and are no used in the current test framework.

They are invoked by ./sage -t (if pytest is installed in the sage venv) and run without issues (at least for me). The sage.all imports that pyflakes is complaining about are needed due to various cyclic imports. #30741 started the work to remove these cyclic imports, but more work is needed.

OK.

mkoeppe commented 2 years ago

comment:94

Spotted some typos: "Riemaninan", "Poissen", "indicies", "symplecic", "where where", "calculate it's" -> "calculate its"

egourgoulhon commented 2 years ago

Changed reviewer from Eric Gourgoulhon, Michael Jung, Travis Scrimshaw to Eric Gourgoulhon, Michael Jung, Matthias Koeppe, Travis Scrimshaw

egourgoulhon commented 2 years ago

comment:96

The documentation of DifferentiableManifold.poisson_tensor and DifferentiableManifold.symplectic_form is pretty terse. It should contain an OUTPUT block with some hyperlink to the class PoissonTensorField, thereby providing an easy access to the latter, e.g.

OUTPUT:

- instance of
  :class:`~sage.manifolds.differentiable.poisson_tensor.PoissonTensorField`

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

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

da70277 Further improvements

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

Changed commit from 902bf29 to da70277

mkoeppe commented 2 years ago

comment:98

Replying to @mjungmath:

I am not sure that sharp and flat are proper names for lowering/raising the index w.r.t. a symplectic structure, I presume they are reserved for metrics only. raise and lower sounds more appropriate to me.

The terminology sharp and flat (as well as musical isomorphisms) is standard in symplectic geometry, too. See for example Abraham Marsden Foundations of Mechanics.

You have a page? I only find the terminology 'lowering' and 'raising'.

I looked at that reference, and page 128 calls the maps "raising" and "lowering". But other sources do use the terminology "the sharp of ..." and "the flat of ..." for the results of these operations. Other things equal, I think for method names one should prefer verbs. So I would be in favor of raise() and lower() as well. Just my 2 cents

mkoeppe commented 2 years ago

comment:99

(However, raise is a reserved word.)

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

Changed commit from da70277 to 92ee51c

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

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

92ee51c #30362: fix documentation + reviewer tweak

egourgoulhon commented 2 years ago

comment:101

The documentation was broken due to some indentation issue in the bullet list of some INPUT blocks. This is fixed in the above commit. In addition, I've

corrrected some cut-and-paste typos
added docstests to __init__ methods, which had no longer any
changed the doctests of PoissonTensorField, SymplecticForm and VectorFieldModule.poisson_tensor and VectorFieldModule.symplectic_form from the Euclidean space to the 2-sphere, since we need here non-parallelizable manifolds (otherwise, the doctest is actually not testing the said method)
added some hyperlink to the Schouten-Nijenhuis bracket

A question: in the definition of a symplectic form given in the docstring of SymplecticForm, shouldn't one add that \omega must be closed?

egourgoulhon commented 2 years ago

comment:102

It would be nice to have this in Sage 9.5. Does everybody agree with the positive review? The doctest error reported by the patchbot does not belong to this ticket and the remaining coverage and pyflakes issues are kind of spurious since they are due to the pytest files symplectic_form_test.py and symplectic_vector_space_test.py (cf. #32975).

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

Changed commit from 92ee51c to 4aef246

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

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

4aef246 Add that symplectic forms are closed

tobiasdiez commented 2 years ago

comment:104

Thanks for your additions and the review!

You are right, symplectic forms need to be closed. I've added it to documentation.

egourgoulhon commented 2 years ago

comment:105

Replying to @tobiasdiez:

You are right, symplectic forms need to be closed. I've added it to documentation.

Thanks!

mkoeppe commented 2 years ago

comment:106

LGTM too.

mjungmath commented 2 years ago

comment:107

Why are manifold.py, scalarfield.py and subset.py touched in this ticket?

mjungmath commented 2 years ago

comment:108

Here are some further comments:

I'd raise an error if the dimension of a symplectic vector space is not even, similarly as you did for symplectic forms.
Rather a comment than a necessity to change:

             if latex_name is None:
-                latex_name = "\\omega"
+                latex_name = r"\omega"

Some very minor comments on the documentation:

         INPUT:

-        - ``f`` -- first function
-        - ``g`` -- second function
+        - ``f`` -- function inserted in the first slot
+        - ``g`` -- function inserted in the second slot

     def volume_form(self, contra: int = 0) -> TensorField:
         r"""
         Liouville volume form `\omega^n` associated with the symplectic form `\omega`,
-        where `2n` is the dimension of the manifold).
+        where `2n` is the dimension of the manifold.
-        TODO: Decide about normalization

I am not sure whether a TODO is supposed to be in the documentation part. I probably would prefer it somewhere in the code. Nevertheless, it'd be nice if you could elaborate a bit more what that is supposed to mean. E.g. if it's a decision, present possible choices. I personally see those snippets as reminder for me and/or as a teaser for other developers who might want to attack this. (As for the former, I tend to forget what I meant by those things if I keep it too vague. :P)

These are just very minor things. If you want to change it, nice. If not, don't worry.

I may take a closer look soon. But so far it already looks really really nice! :)

tobiasdiez commented 2 years ago

comment:109

Replying to @mjungmath:

Why are manifold.py, scalarfield.py and subset.py touched in this ticket?

This is needed for the correct typing in the newly added classes, see #30362 comment:93 for discussion on this point.

Thanks for the other suggestions. I've now implemented them.

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

Changed commit from 4aef246 to ee5b552

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

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

ee5b552 Implement feedback

mkoeppe commented 2 years ago

comment:111

The new class is called SymplecticVectorSpace but as far as I can see there is no vector space structure implemented: Elements are points and cannot be added or scaled.

Perhaps, if it's not too late for this change, it should be called just SymplecticSpace (similar to EuclideanSpace) and we reserve the name SymplecticVectorSpace for later.

mkoeppe commented 2 years ago

comment:112

... or perhaps AffineSymplecticSpace.

mkoeppe commented 2 years ago

comment:113

In particular, sage.modules already has vector spaces that can be equipped with an arbitrary bilinear form - https://doc.sagemath.org/html/en/reference/modules/sage/modules/free_module.html#sage.modules.free_module.FreeModule; symplectic vector spaces are merely a special case.

sagemath / sage

Add symplectic structures #30362