Opened 22 months ago

Closed 4 months ago

Last modified 4 weeks ago

#30362 closed enhancement (fixed)

Add symplectic structures

Reported by: gh-tobiasdiez Owned by:
Priority: major Milestone: sage-9.6
Component: manifolds Keywords:
Cc: tscrim, nthiery, gh-mjungmath, egourgoulhon, mkoeppe Merged in:
Authors: Tobias Diez Reviewers: Eric Gourgoulhon, Michael Jung, Matthias Koeppe, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: b78d8a2 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by slelievre)

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

Change History (182)

comment:1 follow-up: Changed 22 months ago by gh-mjungmath

In my humble opinion, I think the very first step should be to establish a Poisson manifold. This is much more general, i.e. symplectic structures / manifolds are special cases of them. Since none of these are implemented yet, it is much easier to start with the more general setup.

I would like to get some initial feedback, before I cleanup the code and documentation.

I am still busy with other things, but as soon as I have some free time, I could go through your code. However, it would be good to at least remove unneccessary methods (like e.g. hodge_star or sqrt_abs_det) to make the code much more readable and seek your new features immediately.

I noticed that you already added typing. I think, this belongs to another ticket, namely #29775, and should be discussed there before we apply it to new things. There is still this issue with pyflakes and it seems there are still different opinions.

For me SymplecticFormParal is only a implementation detail, so I don't actually want to expose it to the user. Any ideas? (probably needs some modifications in the tensorfield.restrict method).

The use of ...Paral is of course necessary since structures on manifolds are "glued together" via parallelizable parts. I agree in so far that there is no explicit need to expose it to the user since this is all managed via methods in manifold.py, that's right. Howver, in my opinion, there's also nothing wrong about it. Since this is a general problem not only restricted to symplectic forms, I suggest you open another ticket if you want to discuss it.

comment:2 follow-up: Changed 22 months ago by gh-mjungmath

I swiftly overviewed your code. It's a nice addition! However, I have some comments:

  • I think, this is redundant:
             PseudoRiemannianMetric._del_derived(self)
    +        self._del_inverse()
     
         def _del_inverse(self):
    

The method _del_inverse is already invoked in PseudoRiemannianMetric._del_derived.

  • 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.
  • In my opinion this change is not necessary either:
         """
    -    def __init__(self, n, name, field, structure, base_manifold=None,
    +    def __init__(self, n, name, field='real', structure=RealDifferentialStructure(), base_manifold=None,
                      diff_degree=infinity, latex_name=None, start_index=0,
                      category=None, unique_tag=None):
    

The default input is already managed by the factory method Manifolds. And in my opinion, it should stay there for maintenance reasons.

  • It would be good to add a new category for symplectic manifolds and also a new structure in manifolds/structure.py. Besides, I am not sure, is it possible to combine structures? Symplectic and pseudo-Riemannian structures certainly do not exclude each other. If not, I think this is something we should attack at some point. The more structures we allow, the more flexible the framework has to be.
  • I still think that starting with Poisson manifolds is a more reasonable approach. Anyway, someone always has to have the time to do it.
Last edited 22 months ago by gh-mjungmath (previous) (diff)

comment:3 Changed 21 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:4 in reply to: ↑ 1 ; follow-up: Changed 21 months ago by egourgoulhon

Symplectic structures are certainly a nice addition to Sage! Thank you for contributing to this.

I gave a look to the code, but I have to say that in the current state, it is hardly readable for me, because the docstrings are simply copied from other files, without any cleaning nor adaptation of the doctests to the new features introduced here. Can I suggest that you move forward and provide a first version with some minimal documentation and doctests, cleaning out what does not pertain to this ticket.

Replying to gh-mjungmath:

For me SymplecticFormParal is only a implementation detail, so I don't actually want to expose it to the user. Any ideas? (probably needs some modifications in the tensorfield.restrict method).

The use of ...Paral is of course necessary since structures on manifolds are "glued together" via parallelizable parts. I agree in so far that there is no explicit need to expose it to the user since this is all managed via methods in manifold.py, that's right. Howver, in my opinion, there's also nothing wrong about it. Since this is a general problem not only restricted to symplectic forms, I suggest you open another ticket if you want to discuss it.

SymplecticFormParal is not exposed to the user, since the interface should not be via the class names but rather via a method M.symplectic_form() of the manifold M, which returns a SymplecticFormParal if M is parallelizable and a SymplecticForm otherwise.

Besides, I agree with Michael's remarks in comment:2.

comment:5 in reply to: ↑ 4 Changed 21 months ago by gh-mjungmath

Replying to egourgoulhon:

Can I suggest that you move forward and provide a first version with some minimal documentation and doctests, cleaning out what does not pertain to this ticket.

+1

comment:6 Changed 21 months ago by gh-tobiasdiez

Thanks for the feedback. I'll cleanup and improve the code accordingly. I might need a couple of weeks for this however as I'm currently quite busy.

comment:7 in reply to: ↑ 2 ; follow-ups: Changed 18 months ago by gh-tobiasdiez

Thanks for the initial reviews!

Replying to gh-mjungmath:

I swiftly overviewed your code. It's a nice addition! However, I have some comments:

  • I think, this is redundant:
             PseudoRiemannianMetric._del_derived(self)
    +        self._del_inverse()
     
         def _del_inverse(self):
    

The method _del_inverse is already invoked in PseudoRiemannianMetric._del_derived.

Agreed! Not sure why I added it in the first place, but I've now reverted this change.

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

  • In my opinion this change is not necessary either:
         """
    -    def __init__(self, n, name, field, structure, base_manifold=None,
    +    def __init__(self, n, name, field='real', structure=RealDifferentialStructure(), base_manifold=None,
                      diff_degree=infinity, latex_name=None, start_index=0,
                      category=None, unique_tag=None):
    

The default input is already managed by the factory method Manifolds. And in my opinion, it should stay there for maintenance reasons.

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

  • It would be good to add a new category for symplectic manifolds and also a new structure in manifolds/structure.py. Besides, I am not sure, is it possible to combine structures? Symplectic and pseudo-Riemannian structures certainly do not exclude each other. If not, I think this is something we should attack at some point. The more structures we allow, the more flexible the framework has to be.

Agreed! But I would like to leave the categorical questions to a follow-up ticket!

  • I still think that starting with Poisson manifolds is a more reasonable approach. Anyway, someone always has to have the time to do it.

I'm working on it.

Last edited 18 months ago by gh-tobiasdiez (previous) (diff)

comment:8 Changed 18 months ago by gh-tobiasdiez

  • Dependencies set to #30901, #30748
  • Description modified (diff)

comment:9 in reply to: ↑ 7 ; follow-up: Changed 18 months ago by gh-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'.

comment:10 Changed 18 months ago by gh-tobiasdiez

In the second edition, its definition 3.2.1 (p. 174, chapter about symplectic geometry). They don't write "sharp" or "flat" as words, but use the musical symbols.

comment:11 in reply to: ↑ 7 Changed 18 months ago by egourgoulhon

Replying to gh-tobiasdiez:

Although that might be a matter of taste, but I find M = TopologicalManifold(2, 'M') more readable than M = Manifold(2, 'M', structure='topological').

I agree, but the reason for using the function Manifold instead of a direct call to a specific class, like TopologicalManifold, is to not clutter Sage's global namespace, which is already very large. Likewise, at the user level, one has to use Manifold(..., structure='symplectic') instead of SymplecticManifold(...).

comment:12 Changed 18 months ago by git

  • Commit changed from 1dfcf8a41a3cadb6ba3c84a13a8a89bb604bf295 to edc41c5f8ec1346881d28719454934ba40a6aff5

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

13216c1Fix multiarch for shared libraries
d9f36dcDon't use Python 3.8 syntax
94e20c7Revert some of the changes
6dd6e5cFix compilation
eceefb3Remove string wrap
d345bffFix test
c47c4bfCorrect indent
360b312Merge branch 'public/build/multiarchsimple' of git://trac.sagemath.org/sage into public/manifolds/symplectic
2f2997cRevert del_inverse change
edc41c5Rework symplectic form, and introduce Poisson structures

comment:13 follow-up: Changed 18 months ago by gh-tobiasdiez

Ok, makes sense!

I've continued working on it, introduced Poisson structures, wrote most of the documentation and introduced a few tests. It's not yet finished, but if you have a spare minute I would like to get feedback.

A few questions:

  • I get the following error message. What's the origin, and what do I have to do to make it work?
src/sage/manifolds/differentiable/symplectic_form.py:973: in poisson
    if frame not in self._poisson._components:
sage/structure/element.pyx:493: in sage.structure.element.Element.__getattr__
    ???
sage/structure/element.pyx:506: in sage.structure.element.Element.getattr_from_category
    ???
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

>   ???
E   AttributeError: 'SymplecticVectorSpace_with_category' object has no attribute '__custom_name'
  • I'm still somewhat confused by the subclasses that handle the structure on a parallelizable manifold. For example, in the symplectic form class I create the Poisson tensor, give it a name etc.
    poisson_name = f'poisson_{self._name}'
    poisson_latex_name = f'{self._latex_name}^{{-1}}'
    self._poisson = PoissonTensorField(self._vmodule, poisson_name, poisson_latex_name)
    

How do I make sure that it derives from TensorFieldParal if the manifold is parallelizable? I can of course put the same code in SymplecticFormParal and replace PoissonTensorField by PoissonTensorFieldParal but this appears to be somewhat subopmimal.

Moreover, do I have to copy-paste the documentation from the parent class, or what is the convention?

  • How do I obtain a general function on a manifold? On a vector space M, I can do something like
    q, p = M.cartesian_coordinates()[:]
    f = M.scalar_field(function('f')(q,p), name='f')
    

but what would be the version on a sphere (in such a way that the code works for M being a vector space and a sphere)?

  • Since the hodge star operator makes sense also with respect to a symplectic form, I would propose to move the current method from Metric to DifferentialForm: DifferentialForm.hodge_star(Metric|SymplecticForm) (with an alias in Metric and SymplecticForm). Opinions?
  • What is the best way to test if two differential forms are equal (including / except of their name)?

Thanks!

Last edited 18 months ago by gh-tobiasdiez (previous) (diff)

comment:14 in reply to: ↑ 13 ; follow-up: Changed 18 months ago by egourgoulhon

Replying to gh-tobiasdiez:

  • I'm still somewhat confused by the subclasses that handle the structure on a parallelizable manifold. For example, in the symplectic form class I create the Poisson tensor, give it a name etc.
    poisson_name = f'poisson_{self._name}'
    poisson_latex_name = f'{self._latex_name}^{{-1}}'
    self._poisson = PoissonTensorField(self._vmodule, poisson_name, poisson_latex_name)
    

How do I make sure that it derives from TensorFieldParal if the manifold is parallelizable?

You should construct the Poisson tensor from the module of vector fields on the manifold, not by a direct call to PoissonTensorField, see the example of DifferentiableManifold.metric() in src/sage/manifods/differentiable/manifold.py.

Moreover, do I have to copy-paste the documentation from the parent class, or what is the convention?

I don't understand the question, sorry. Could you rephrase it?

  • How do I obtain a general function on a manifold? On a vector space M, I can do something like
    q, p = M.cartesian_coordinates()[:]
    f = M.scalar_field(function('f')(q,p), name='f')
    

but what would be the version on a sphere (in such a way that the code works for M being a vector space and a sphere)?

The symbolic functions created by function(...) are functions of coordinates on a given chart; so to construct a generic function on a manifold, you have to pass a dictionary with as many charts as necessary to cover the manifold:

   f = M.scalar_field({X1: function('f_1')(q1, p1), X2: function('f_2')(q2, p2),...})

where X1 is the chart with coordinates (q1, p1), etc.

  • Since the hodge star operator makes sense also with respect to a symplectic form, I would propose to move the current method from Metric to DifferentialForm: DifferentialForm.hodge_star(Metric|SymplecticForm) (with an alias in Metric and SymplecticForm). Opinions?

The Hodge star is already in DifferentialForm: it is called hodge_dual() there and its code is simply return metric.hodge_star(self).

  • What is the best way to test if two differential forms are equal (including / except of their name)?

I would say simply a == b. This is actually computing a - b (in an efficient way, using the full antisymmetry of a and b's components) and tests whether the result is zero.

A question from my side: why is this ticket depending on #30901 and #30748 ? In the ticket description you write Please ignore the dependencies, these are just there so that I can work locally on my PC. This looks somewhat odd...

Maybe this is related, but the ticket branch contains the following modified files:

-rw-r--r--	.gitignore	9	
-rw-r--r--	src/sage/all.py	6	
-rw-r--r--	src/sage/all_cmdline.py	1	
-rw-r--r--	src/sage/env.py	76	
-rw-r--r--	src/sage/libs/gap/util.pyx	1		
-rw-r--r--	src/sage/libs/singular/singular.pyx	11	
-rw-r--r--	src/sage/misc/lazy_import.pyx	69	
-rw-r--r--	src/sage/misc/startup_guard.py	25			
-rw-r--r--	src/test.py	64	

Do they really pertain to this ticket?

comment:15 Changed 18 months ago by git

  • Commit changed from edc41c5f8ec1346881d28719454934ba40a6aff5 to c6fb9c6e92188c93adf5aaf1474fb041e74460a8

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

c6fb9c6Fix tests

comment:16 in reply to: ↑ 14 ; follow-up: Changed 18 months ago by gh-tobiasdiez

  • Cc mkoeppe added

Replying to egourgoulhon:

Replying to gh-tobiasdiez:

  • I'm still somewhat confused by the subclasses that handle the structure on a parallelizable manifold. For example, in the symplectic form class I create the Poisson tensor, give it a name etc.
    poisson_name = f'poisson_{self._name}'
    poisson_latex_name = f'{self._latex_name}^{{-1}}'
    self._poisson = PoissonTensorField(self._vmodule, poisson_name, poisson_latex_name)
    

How do I make sure that it derives from TensorFieldParal if the manifold is parallelizable?

You should construct the Poisson tensor from the module of vector fields on the manifold, not by a direct call to PoissonTensorField, see the example of DifferentiableManifold.metric() in src/sage/manifods/differentiable/manifold.py.

Ok, thanks! That makes sense indeed and worked.

Moreover, do I have to copy-paste the documentation from the parent class, or what is the convention?

I don't understand the question, sorry. Could you rephrase it?

If I implement a method in say PoissonTensorFieldParal that overrides a method in PoissonTensorField, do I have to add documentation/tests/examples to the method in PoissonTensorFieldParal?` (given that the signature of the method is exactly the same)? I think sphinx normally copies the documentation given in the parent class.

  • How do I obtain a general function on a manifold? On a vector space M, I can do something like
    q, p = M.cartesian_coordinates()[:]
    f = M.scalar_field(function('f')(q,p), name='f')
    

but what would be the version on a sphere (in such a way that the code works for M being a vector space and a sphere)?

The symbolic functions created by function(...) are functions of coordinates on a given chart; so to construct a generic function on a manifold, you have to pass a dictionary with as many charts as necessary to cover the manifold:

   f = M.scalar_field({X1: function('f_1')(q1, p1), X2: function('f_2')(q2, p2),...})

where X1 is the chart with coordinates (q1, p1), etc.

That worked! Merci!

  • Since the hodge star operator makes sense also with respect to a symplectic form, I would propose to move the current method from Metric to DifferentialForm: DifferentialForm.hodge_star(Metric|SymplecticForm) (with an alias in Metric and SymplecticForm). Opinions?

The Hodge star is already in DifferentialForm: it is called hodge_dual() there and its code is simply return metric.hodge_star(self).

  • What is the best way to test if two differential forms are equal (including / except of their name)?

I would say simply a == b. This is actually computing a - b (in an efficient way, using the full antisymmetry of a and b's components) and tests whether the result is zero.

That worked as well!

A question from my side: why is this ticket depending on #30901 and #30748 ? In the ticket description you write Please ignore the dependencies, these are just there so that I can work locally on my PC. This looks somewhat odd...

Maybe this is related, but the ticket branch contains the following modified files:

-rw-r--r--	.gitignore	9	
-rw-r--r--	src/sage/all.py	6	
-rw-r--r--	src/sage/all_cmdline.py	1	
-rw-r--r--	src/sage/env.py	76	
-rw-r--r--	src/sage/libs/gap/util.pyx	1		
-rw-r--r--	src/sage/libs/singular/singular.pyx	11	
-rw-r--r--	src/sage/misc/lazy_import.pyx	69	
-rw-r--r--	src/sage/misc/startup_guard.py	25			
-rw-r--r--	src/test.py	64	

Do they really pertain to this ticket?

Sorry for these (unnessary) changes. The only way I currently have to work on sage is with the virtual environment created in #30371. The compiled cython however only works correctly if the changes of #30901 and #30748 are included. Thus, until these packages are merged, I have to sadly include them as dependencies on all packages I develop...maybe you or Matthias have a better idea for a workaround.

Now all tests are passing (not sure about the doctests), and the only thing left is to change the documentation a bit.


New commits:

c6fb9c6Fix tests
Last edited 18 months ago by gh-tobiasdiez (previous) (diff)

comment:17 in reply to: ↑ 16 Changed 18 months ago by mkoeppe

Replying to gh-tobiasdiez:

A question from my side: why is this ticket depending on #30901 and #30748 ? In the ticket description you write Please ignore the dependencies, these are just there so that I can work locally on my PC. This looks somewhat odd...

Indeed, this is not the way to do this.

Sorry for these (unnessary) changes. The only way I currently have to work on sage is with the virtual environment created in #30371. [...]maybe you or Matthias have a better idea for a workaround.

I would suggest to create a clean branch starting from the latest beta and to push that to the ticket only. Use git cherry-pick to get the commits from your branch (with the dependencies that you say you need for running Sage) onto the clean branch. Push the clean branch to the ticket.

comment:18 Changed 18 months ago by gh-tobiasdiez

  • Authors set to Tobias Diez

comment:19 Changed 18 months ago by gh-tobiasdiez

  • Description modified (diff)

comment:20 Changed 18 months ago by git

  • Commit changed from c6fb9c6e92188c93adf5aaf1474fb041e74460a8 to c507da7bf9de1cf9903d5f3169b2953981cfa0e4

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

c507da7Add documentation to symplectic vector space

comment:21 Changed 18 months ago by git

  • Commit changed from c507da7bf9de1cf9903d5f3169b2953981cfa0e4 to da7e6c3d96b804050be04a201c30d0bb19a0c59d

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

da7e6c3Revise docs for Poissen tensors

comment:22 Changed 17 months ago by git

  • Commit changed from da7e6c3d96b804050be04a201c30d0bb19a0c59d to 137314a8c11e2d291f524da7ab84d5dfc0a6f760

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

a3e1fc0Remove test script
c81cec9Remove additions to gitignore
a131e26Revise code
d31e066Cleanup
b827f46Merge branch 'develop' of git://github.com/sagemath/sage into public/manifolds/symplectic
137314aMerge branch 'develop' of git://github.com/sagemath/sage into public/manifolds/symplectic

comment:23 Changed 17 months ago by git

  • Commit changed from 137314a8c11e2d291f524da7ab84d5dfc0a6f760 to 17fadb64d0b4d36686c5594d8da1ead358824188

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

17fadb6Revert "Wrap each sage.all import in startup guard"

comment:24 Changed 17 months ago by gh-tobiasdiez

  • Dependencies #30901, #30748 deleted

comment:25 Changed 17 months ago by git

  • Commit changed from 17fadb64d0b4d36686c5594d8da1ead358824188 to b8f7881d962051123dfcc15a85154671ea220942

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

b8f7881Remove unrelated changes

comment:26 Changed 17 months ago by git

  • Commit changed from b8f7881d962051123dfcc15a85154671ea220942 to 8e8923b38a3559a21ce1a89a3c725b97a46a0f5b

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

070c7f9Remove unrelated changes
8e8923bImplement hodge star

comment:27 Changed 17 months ago by gh-tobiasdiez

  • Status changed from new to needs_review

So, I think, I'm mostly finished with the implementation. Some of the doctests may still fail and this will be a bit of a tedious procedure since sage -t doesn't work for me locally (need to investigate). But it's definitely in a state where I would appreciate feedback!

comment:28 Changed 17 months ago by tscrim

Thank you for your work on this. I will let someone who is more invested in the manifold code give more feedback about the overall implementation and structure. I will focus here on the more technical aspects.

From the patchbot, you have at least one infinite recursion to take care of:

**********************************************************************
File "src/sage/manifolds/differentiable/vectorfield.py", line 1024, in sage.manifolds.differentiable.vectorfield.VectorField.curl
Failed example:
    curl(v) == s
Exception raised:
    Traceback (most recent call last):
      File "/home/sagemath/sage-9.1/local/lib/python3.7/site-packages/sage/doctest/forker.py", line 714, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/sagemath/sage-9.1/local/lib/python3.7/site-packages/sage/doctest/forker.py", line 1133, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.manifolds.differentiable.vectorfield.VectorField.curl[6]>", line 1, in <module>
        curl(v) == s
      File "/home/sagemath/sage-9.1/local/lib/python3.7/site-packages/sage/manifolds/operators.py", line 239, in curl
        return vector.curl()
      File "/home/sagemath/sage-9.1/local/lib/python3.7/site-packages/sage/manifolds/differentiable/vectorfield.py", line 1052, in curl
        resu = der.hodge_dual(metric).up(metric)
      File "/home/sagemath/sage-9.1/local/lib/python3.7/site-packages/sage/manifolds/differentiable/diff_form.py", line 1568, in hodge_dual
        return metric.hodge_star(self)
      File "/home/sagemath/sage-9.1/local/lib/python3.7/site-packages/sage/manifolds/differentiable/metric.py", line 1970, in hodge_star
        return pform.hodge_dual(self)
      File "/home/sagemath/sage-9.1/local/lib/python3.7/site-packages/sage/manifolds/differentiable/diff_form.py", line 1568, in hodge_dual

You should write all of the docstrings and doctests for all of your methods.

There are a number of formatting issues with your docstrings:

-TESTS:
+TESTS::
+
     sage: import pytest
     sage: pytest.main("symplectic_vector_space_test.py")
     TODO: add output
     def hamiltonian_vector_field(self, function: DiffScalarField) -> VectorField:
         r"""
         Return the Hamiltonian vector field `X_f` generated by the given function `f: M \to \RR`.
+
-        It is defined by
+        The Hamiltonian vector field is defined by
+
         .. MATH::
 
             X_f = - \varpi^\sharp (d f),
 
         where `\varpi^\sharp: T^* M \to TM` is given by
         `\beta(\varpi^\sharp(\alpha)) = \varpi(\alpha, \beta)`.
 
         INPUT:
+
         - ```function`` -- the function generating the Hamiltonian vector field
        - ``coordinates`` -- (default: ``'Cartesian'``) the
          type of coordinates to be initialized at the Euclidean space
          creation; allowed values are

          * ``'Cartesian'`` (canonical coordinates on `\RR^{2n}`)
          * ``'polar'`` for ``dimension=2`` only (see
            :meth:`~sage.manifolds.differentiable.examples.euclidean.EuclideanPlane.polar_coordinates`)
        - ``symbols`` -- the coordinate
          text symbols and LaTeX symbols, with the same conventions as the
          argument ``coordinates`` in
          ...

among others.

I am not sure I agree with using pytest and a test file as opposed to standard docstrings. We are still experimenting with these features.

poisson_tensor.py is missing the initial file header info and docstring.

I don't see the reason for the TYPE_CHECKING. There are also other ways to add the type info without importing.

I think this docstring is wrong:

    def poisson(self, expansion_symbol: Optional[Expression] = None, order: int = 1) -> TensorFieldParal:
        r"""
        Return the inverse metric.

comment:29 Changed 17 months ago by gh-tobiasdiez

  • Dependencies set to #31003

Thanks for the feedback! I've now implemented your suggestions.

I don't see the reason for the TYPE_CHECKING. There are also other ways to add the type info without importing.

What "other ways" do you mean? These imports are encapsulated by a check for TYPE_CHECKING, which is false during runtime and is only set to true by static code analyzers. This is needed to prevent circular inputs or imports from half-initialized modules.

comment:30 Changed 17 months ago by git

  • Commit changed from 8e8923b38a3559a21ce1a89a3c725b97a46a0f5b to aa6a0e3918299a5ed79bcb691d659c60dc64e8aa

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

9f814f1Fix tests
aa6a0e3Implement feedback

comment:31 Changed 17 months ago by git

  • Commit changed from aa6a0e3918299a5ed79bcb691d659c60dc64e8aa to 8dedc5e449a63bf30937ded298a39ca71e9412e4

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

8dedc5eCleanup code

comment:32 Changed 17 months ago by git

  • Commit changed from 8dedc5e449a63bf30937ded298a39ca71e9412e4 to 6e1b515ddd13c826789a5dd89843da3d669f71fc

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

6e1b515Fix tests

comment:33 Changed 17 months ago by gh-tobiasdiez

  • Description modified (diff)

comment:34 Changed 17 months ago by gh-mjungmath

It might be worth mentioning that there was already an attempt in implementing Poisson structures. A first step had been made with #23429. It'd be beneficial to reuse parts of that code instead of writing new one. In particular, the Poisson tensor field, as you call it, is a bi-vectorfield and hence a multivectorfield in the above sense. It would thus make sense if your Poisson tensor inherits from a multivectorfield.

Moreover, I think it's a good idea to split this ticket into two: one for the Poisson structure and another (dependent on the Poisson structure ticket) for the symplectic structure. That makes the debugging and review much easier.

At the moment, I am unfortunately busy with other things. If I have some time, I'll take a closer look.

comment:35 Changed 17 months ago by gh-mjungmath

And pyflakes is complaining:

src/sage/manifolds/differentiable/examples/symplectic_vector_space_test.py:1:1 'sage.all' imported but unused

src/sage/manifolds/differentiable/symplectic_form_test.py:5:1 'sage.all' imported but unused

comment:36 Changed 17 months ago by git

  • Commit changed from 6e1b515ddd13c826789a5dd89843da3d669f71fc to 63d3ae6feadec0b127b6896e856d980f6c712748

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

63d3ae6Implement poisson tensor using multivector fields

comment:37 follow-up: Changed 17 months ago by gh-tobiasdiez

Thanks for the pointer to the multivector fields! I've not been aware of their existence. The poisson tensor now inherits from MultivectorField? instead of TensorField?.

Since the PoissonTensor? class is relatively short, and most changes intertwine Poisson and symplectic things (e.g. the up/down methods) I would like to keep them together.

The sage.all imports are needed due to half-finished module imports otherwise. I plan to address this in forthcoming tickets.

comment:38 in reply to: ↑ 37 ; follow-up: Changed 17 months ago by gh-mjungmath

Replying to gh-tobiasdiez:

Since the PoissonTensor? class is relatively short...

Just underlines my argument. ;)

Short code is much easier to review. Besides, since all the symplectic part relies on the Poisson structure, it's good to review that one first.

The sage.all imports are needed due to half-finished module imports otherwise. I plan to address this in forthcoming tickets.

What do you mean?

comment:39 Changed 17 months ago by git

  • Commit changed from 63d3ae6feadec0b127b6896e856d980f6c712748 to d968468319d7111fe1b12fa6acb4a3a69fc3c960

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

d968468A bit more code cleanup

comment:40 Changed 17 months ago by mkoeppe

  • Dependencies changed from #31003 to #31003, #30551

Added #30551 dependency because of future.annotations

Last edited 17 months ago by mkoeppe (previous) (diff)

comment:41 follow-up: Changed 17 months ago by gh-mjungmath

What's the story behind the ..._test.py files? Is it related to #30738?

Last edited 17 months ago by gh-mjungmath (previous) (diff)

comment:42 follow-up: Changed 17 months ago by gh-mjungmath

I'd say we should drop these features such as typing, pytest etc. in this ticket for now, otherwise this ticket will stay on hold as long as #30738, #30551, #31003, #29775 etc. and all their follow ups have not been settled.

Taking a look at the discussions, that could take a while, whereas this ticket seems in principle to be ready for review.

Last edited 17 months ago by gh-mjungmath (previous) (diff)

comment:43 Changed 17 months ago by git

  • Commit changed from d968468319d7111fe1b12fa6acb4a3a69fc3c960 to 1c2cd6d063c335acf3b4736b74d78485970b74b5

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

1c2cd6dAdd more examples

comment:44 in reply to: ↑ 38 ; follow-up: Changed 17 months ago by gh-tobiasdiez

Replying to gh-mjungmath:

The sage.all imports are needed due to half-finished module imports otherwise. I plan to address this in forthcoming tickets.

What do you mean?

Without these imports I get the following error:

ImportError while importing test module '/mnt/d/Programming/sage/src/sage/manifolds/differentiable/examples/symplectic_vector_space_test.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/usr/lib/python3.8/importlib/__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
sage/manifolds/differentiable/examples/symplectic_vector_space_test.py:2: in <module>
    from sage.manifolds.differentiable.symplectic_form import SymplecticForm
sage/manifolds/differentiable/symplectic_form.py:34: in <module>
    from sage.symbolic.expression import Expression
sage/symbolic/expression.pyx:310: in init sage.symbolic.expression
    from .ring import SR
sage/symbolic/ring.pyx:1: in init sage.symbolic.ring
    # -*- coding: utf-8 -*-
sage/rings/integer.pyx:1: in init sage.rings.integer
    r"""
sage/rings/rational.pyx:95: in init sage.rings.rational
    import sage.rings.real_mpfr
sage/rings/real_mpfr.pyx:1: in init sage.rings.real_mpfr
    r"""
sage/libs/mpmath/utils.pyx:1: in init sage.libs.mpmath.utils
    """
sage/rings/complex_mpfr.pyx:1: in init sage.rings.complex_mpfr
    """
sage/rings/complex_double.pyx:98: in init sage.rings.complex_double
    from . import complex_mpfr
E   ImportError: cannot import name complex_mpfr

I tried to make progress towards solving this in #30741, but I cannot really work on this until #30371 is merged.

comment:45 in reply to: ↑ 41 ; follow-up: Changed 17 months ago by gh-tobiasdiez

Replying to gh-mjungmath:

What's the story behind the ..._test.py files? Is it related to #30738?

It doesn't really rely on #30738, but it uses pytest as well, yes. I'm not a big fan of doctests (as unit tests) and was told that I can always extract them to test files if I prefer that.

comment:46 in reply to: ↑ 42 Changed 17 months ago by gh-tobiasdiez

Replying to gh-mjungmath:

I'd say we should drop these features such as typing, pytest etc. in this ticket for now, otherwise this ticket will stay on hold as long as #30738, #30551, #31003, #29775 etc. and all their follow ups have not been settled.

The added typing doesn't have any dependencies (beyond #30551, which will be merged in 9.4). The pytests rely only on #31003, which is hopefully close to be merged. If you prefer, I can extract the pytests to a new ticket although it feels strange to separate the implementation from its tests.

comment:47 in reply to: ↑ 45 ; follow-up: Changed 17 months ago by gh-mjungmath

Technically, yes. However, I have the feeling that there are still too many open discussions about using pytest and typing and how to pull it on the current framework. I doubt this ticket will get a positive review until everything is clarified in a broader context like #29775 and #30738. See also comment:28 from Travis.

Anyway, I think that symplectic forms are a great and extremely useful addition to Sage! I am looking forward to have that feature acutally in there. Thanks! :)

comment:48 in reply to: ↑ 44 ; follow-up: Changed 17 months ago by mkoeppe

Replying to gh-tobiasdiez:

I tried to make progress towards solving this in #30741, but I cannot really work on this until #30371 is merged.

It's best to keep workarounds for your experimental setup on your branches and not to push them to tickets. We cannot merge such code and it complicates the discussions in the review. I recommend to use git add -p and git cherry-pick to make clean branches.

comment:49 in reply to: ↑ 47 Changed 17 months ago by gh-tobiasdiez

  • Milestone changed from sage-9.3 to sage-9.4

Replying to gh-mjungmath:

I have the feeling that there are still too many open discussions about using pytest and typing and how to pull it on the current framework.

I agree! Since it's also a lot of work to remove the typing information again, I would say we wait until the development cycle of 9.4 is started, which will resolve #30551 and thereby the other typing tickets. When the time comes, we can reevaluate to split of the pytests to a new ticket. I think this is a resonable time plan anyway, since this is my first bigger contribution to sage and I'm expecting quite some comments concerning the actual implementation, beyond pytest and typing.

comment:50 in reply to: ↑ 48 ; follow-up: Changed 17 months ago by gh-tobiasdiez

Replying to mkoeppe:

It's best to keep workarounds for your experimental setup on your branches and not to push them to tickets. We cannot merge such code and it complicates the discussions in the review. I recommend to use git add -p and git cherry-pick to make clean branches.

Thanks, but the problem does not come from my editable setup. Pytest simply doesn't import sage.all before importing the test file, and without such an import other imports such as from sage.symbolic.expression import Expression do not work (because they implicitly rely on a certain import order that is provided by sage.all). The reference to #30371 was only because I cannot efficiently work on #30741 without recompiling cython files for which I need #30371.

Last edited 17 months ago by gh-tobiasdiez (previous) (diff)

comment:51 in reply to: ↑ 50 ; follow-up: Changed 17 months ago by mkoeppe

Replying to gh-tobiasdiez:

the problem does not come from my editable setup. Pytest simply doesn't import sage.all before importing the test file, and without such an import other imports such as from sage.symbolic.expression import Expression do not work (because they implicitly rely on a certain import order that is provided by sage.all).

Thanks for the clarification. Would it be enough to do the import in the src/conftest.py file added by #31003?

comment:52 in reply to: ↑ 51 Changed 17 months ago by gh-tobiasdiez

Replying to mkoeppe:

Thanks for the clarification. Would it be enough to do the import in the src/conftest.py file added by #31003?

Sadly not. The fixture in conftest.py does load sage.all, but this is only used when the test are run and not on test discovery. Hence, this only prevents these problems on runtime, but not if the import of the test module already fails. Anyway, for the modularization effort is still desired that one can import and use any sage module without using sage.all before.

Last edited 17 months ago by gh-tobiasdiez (previous) (diff)

comment:53 Changed 13 months ago by git

  • Commit changed from 1c2cd6d063c335acf3b4736b74d78485970b74b5 to 20875716e4e04bcb894594da61ae7e02be2acd1c

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

66c2987build/pkgs/{sage_sws2rst,sage_docbuild}/install-requires.txt: New
7f8ec6cbuild/pkgs/sagelib/src/tox.ini: Add factor sitepackages
0283da5build/make/Makefile.in: Add wheel, setuptools_wheel to PYTHON_TOOLCHAIN to make sure that PEP 517 packages have a complete build system
f720722build/pkgs/sagelib/src/tox.ini: Add factor nobuildisolation
c451b31src/setup.cfg.m4 (install_requires): Add sage_conf
6700223Merge tag '9.3.rc0' into t/30913/sagelib__add_setup_cfg__install_requires_
04da2c6build/pkgs/ipywidgets: Patch out declaring install-requires of nbformat and jupyterlab-widgets
815c944Merge #30913
b06731cRemove Python 3.6 support from metadata and documentation
2087571Add symplectic structures (Trac 30362 squashed)

comment:54 Changed 13 months ago by mkoeppe

  • Dependencies changed from #31003, #30551 to #30551
  • Status changed from needs_review to needs_work

Rebased (squashed) on top of updated #30551

Last edited 13 months ago by mkoeppe (previous) (diff)

comment:55 Changed 13 months ago by git

  • Commit changed from 20875716e4e04bcb894594da61ae7e02be2acd1c to 14619d194e8cdb85da06390d5b8c4da929c4a664

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

14619d1Add symplectic structures (Trac 30362 squashed)

comment:56 Changed 13 months ago by git

  • Commit changed from 14619d194e8cdb85da06390d5b8c4da929c4a664 to daf5af854c415c751f3b053b75195efef6632e27

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

daf5af8Add symplectic structures (Trac 30362 squashed)

comment:57 Changed 13 months ago by mkoeppe

Rebased, no longer on top of #30551

comment:58 Changed 13 months ago by gh-tobiasdiez

  • Dependencies #30551 deleted
  • Status changed from needs_work to needs_review

Thanks! From my side this is also ready for review!

comment:59 Changed 13 months ago by mkoeppe

  • Dependencies set to #30551

The ticket still has to depend on #30551, just testing it you don't need the branch to be on top of it.

comment:60 Changed 13 months ago by gh-mjungmath

  • Status changed from needs_review to needs_work

There are a lot of doctests missing. For example symplectic_form and poisson_tensor in differentiable/manifold.py.

comment:61 Changed 13 months ago by gh-tobiasdiez

Thanks for the input. That was one point I was struggling with. The tests are there, just in a different file (e.g. differentiable/poisson_tensor.py) and the methods in manifold.py is just a convenient helper method. So I was wondering where to add tests, without just copying them all over the place.

comment:62 Changed 13 months ago by gh-mjungmath

Well, it is not only about testing but also about documenting. Keep in mind that EXAMPLE blocks occur in the official reference manual and therefore provide a guideline for the user how to use that method adequately aside from testing.

Having the same examples in different places in the documentation isn't necessarily a bad thing. What I usually do is to change some values, dimensions or expressions instead of blindly copy.

Last edited 13 months ago by gh-mjungmath (previous) (diff)

comment:63 Changed 13 months ago by gh-mjungmath

It is also a good idea to add a SEEALSO block in the poisson_tensor method linking to differentiable/poisson_tensor.py.

comment:64 Changed 12 months ago by egourgoulhon

Now that Sage 9.4 beta cycle has started, it would be nice to have this ticket make its way into Sage 9.4.

comment:65 Changed 10 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

Setting a new milestone for this ticket based on a cursory review.

comment:66 Changed 6 months ago by git

  • Commit changed from daf5af854c415c751f3b053b75195efef6632e27 to c471d06b6cef6051bb6759b8d458842167cfade5

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

193bb88Merge branch 'develop' of https://github.com/sagemath/sage into public/manifolds/symplectic
c471d06Cleanup and add missing doctests

comment:67 Changed 6 months ago by git

  • Commit changed from c471d06b6cef6051bb6759b8d458842167cfade5 to 0903929abbfc0c4901bcc92d9046afc66d96ebcf

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

0903929Final cleanup

comment:68 follow-up: Changed 6 months ago by gh-tobiasdiez

  • Dependencies changed from #30551 to #32953
  • Status changed from needs_work to needs_review

Please excuse that it took me so long, but I've finally found the time to cleanup the code. The tests should be complete and pass now, except for the pytests for S2 which fail because of #32953. Other than this, this is ready for review.

comment:69 in reply to: ↑ 68 Changed 6 months ago by egourgoulhon

Replying to gh-tobiasdiez:

Please excuse that it took me so long, but I've finally found the time to cleanup the code. The tests should be complete and pass now, except for the pytests for S2 which fail because of #32953. Other than this, this is ready for review.

Happy to see that symplectic structures make their way to Sage! I'll have a look in the coming days. Meanwhile, the patchbot reveals that the doctest coverage is not complete. You can check it by yourself by running sage -coverage src/sage/manifolds.

comment:70 Changed 6 months ago by git

  • Commit changed from 0903929abbfc0c4901bcc92d9046afc66d96ebcf to 16a08af77e3aafc65ea709ea3d3adeba188362ba

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

16a08afAdd and fix doctests

comment:71 Changed 6 months ago by gh-tobiasdiez

Thanks, I've overlooked that some doctests were missing. Now everything should be in order, except for a few 'false-positves' related to the usage of pytest. For this, I've opened #32975.

comment:72 Changed 6 months ago by gh-tobiasdiez

  • Dependencies changed from #32953 to #32953, #32975

comment:73 Changed 6 months ago by git

  • Commit changed from 16a08af77e3aafc65ea709ea3d3adeba188362ba to fbc1af8138cb0777a6fe7cfefd2f6ea8809a92b8

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

fbc1af8Fix S2 tests

comment:74 Changed 6 months ago by git

  • Commit changed from fbc1af8138cb0777a6fe7cfefd2f6ea8809a92b8 to bf562263d6c35df665557c1a1e027424aabfe878

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

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

comment:75 Changed 6 months ago by egourgoulhon

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

comment:76 Changed 6 months ago by egourgoulhon

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

comment:77 in reply to: ↑ 7 ; follow-up: Changed 6 months ago by gh-mjungmath

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

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

comment:78 in reply to: ↑ 77 Changed 6 months ago by egourgoulhon

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

comment:79 follow-up: Changed 6 months ago by gh-tobiasdiez

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.

comment:80 Changed 6 months ago by git

  • Commit changed from bf562263d6c35df665557c1a1e027424aabfe878 to 3f390f86a8f1958265325a32999f3c4e9c4b860f

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

3f390f8Incorporate feedback

comment:81 in reply to: ↑ 79 ; follow-ups: Changed 6 months ago by tscrim

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

comment:82 in reply to: ↑ 81 Changed 6 months ago by egourgoulhon

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.

comment:83 in reply to: ↑ 81 Changed 6 months ago by gh-mjungmath

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

comment:84 Changed 6 months ago by gh-tobiasdiez

  • Dependencies #32953, #32975 deleted

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.

comment:85 Changed 6 months ago by git

  • Commit changed from 3f390f86a8f1958265325a32999f3c4e9c4b860f to 7d98bd4afa83148a9fae4890751913043b73b695

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

7d98bd4Remove dependency on other ticket

comment:86 follow-up: Changed 6 months ago by tscrim

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

comment:87 in reply to: ↑ 86 Changed 6 months ago by gh-tobiasdiez

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.

comment:88 Changed 6 months ago by slelievre

  • Description modified (diff)

comment:89 follow-up: Changed 6 months ago by egourgoulhon

  • Reviewers set to Eric Gourgoulhon, Michael Jung, Travis Scrimshaw

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.

comment:90 in reply to: ↑ 89 ; follow-up: Changed 6 months ago by gh-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. 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.

comment:91 Changed 6 months ago by git

  • Commit changed from 7d98bd4afa83148a9fae4890751913043b73b695 to 902bf29675c3919a69630bda1e8260463dc2e664

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

902bf29Further changes based on feedback

comment:92 Changed 6 months ago by egourgoulhon

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:

902bf29Further changes based on feedback

comment:93 in reply to: ↑ 90 Changed 6 months ago by egourgoulhon

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

comment:94 Changed 6 months ago by mkoeppe

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

comment:95 Changed 6 months ago by egourgoulhon

  • Reviewers changed from Eric Gourgoulhon, Michael Jung, Travis Scrimshaw to Eric Gourgoulhon, Michael Jung, Matthias Koeppe, Travis Scrimshaw

comment:96 Changed 6 months ago by egourgoulhon

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`

comment:97 Changed 6 months ago by git

  • Commit changed from 902bf29675c3919a69630bda1e8260463dc2e664 to da70277b017d78cee9234f06692725ebb6b51eed

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

da70277Further improvements

comment:98 in reply to: ↑ 9 Changed 6 months ago by mkoeppe

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

comment:99 Changed 6 months ago by mkoeppe

(However, raise is a reserved word.)

comment:100 Changed 6 months ago by git

  • Commit changed from da70277b017d78cee9234f06692725ebb6b51eed to 92ee51cf10b6c460ee5dc98db2c4ec8be612ebd4

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

92ee51c#30362: fix documentation + reviewer tweak

comment:101 Changed 6 months ago by egourgoulhon

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?

Last edited 6 months ago by egourgoulhon (previous) (diff)

comment:102 Changed 6 months ago by egourgoulhon

  • Status changed from needs_review to positive_review

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

comment:103 Changed 6 months ago by git

  • Commit changed from 92ee51cf10b6c460ee5dc98db2c4ec8be612ebd4 to 4aef2467eaa1c41815bde8804a54722704f869b0
  • Status changed from positive_review to needs_review

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

4aef246Add that symplectic forms are closed

comment:104 follow-up: Changed 6 months ago by gh-tobiasdiez

Thanks for your additions and the review!

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

comment:105 in reply to: ↑ 104 Changed 6 months ago by egourgoulhon

  • Status changed from needs_review to positive_review

Replying to gh-tobiasdiez:

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

Thanks!

comment:106 Changed 6 months ago by mkoeppe

LGTM too.

comment:107 follow-up: Changed 6 months ago by gh-mjungmath

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

comment:108 Changed 6 months ago by gh-mjungmath

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

comment:109 in reply to: ↑ 107 Changed 5 months ago by gh-tobiasdiez

Replying to gh-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 https://trac.sagemath.org/ticket/30362#comment:93 for discussion on this point.

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

comment:110 Changed 5 months ago by git

  • Commit changed from 4aef2467eaa1c41815bde8804a54722704f869b0 to ee5b5528032a3994c1e9d711eba6d622e7319ea2
  • Status changed from positive_review to needs_review

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

ee5b552Implement feedback

comment:111 follow-up: Changed 5 months ago by mkoeppe

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.

comment:112 Changed 5 months ago by mkoeppe

... or perhaps AffineSymplecticSpace.

comment:113 Changed 5 months ago by mkoeppe

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.

comment:114 follow-up: Changed 5 months ago by gh-mjungmath

Another thing. I think it is better to use Sage's factorial function in symplectic_form.py, and import this function in the only method where it is needed, namely volume_form:

 from __future__ import annotations
 from six.moves import range
 from typing import Dict, Union, Optional
-from math import factorial
             sage: vol = omega.volume_form() ; vol
             4-form mu_omega on the 4-dimensional symplectic vector space R4
             sage: vol.display()
             mu_omega = dq1∧dp1∧dq2∧dp2
         """
+        from sage.functions.other import factorial
+
         if self._vol_form is None:

comment:115 in reply to: ↑ 111 ; follow-up: Changed 5 months ago by gh-tobiasdiez

Replying to mkoeppe:

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.

That's true. As written in the ticket description, the plan is to provide a "vector space as a manifold" example that should be used as the base class for SymplecticVectorSpace.

.. or perhaps AffineSymplecticSpace?.

Not sure if one needs this generalization, and even then one probably wants to say that its an affine space modeled on a symplectic vector space.

comment:116 in reply to: ↑ 114 ; follow-up: Changed 5 months ago by gh-tobiasdiez

Replying to gh-mjungmath:

Another thing. I think it is better to use Sage's factorial function in symplectic_form.py

I was wondering about this too, but in the end decided for the built-in factorial since the numbers for which one calculates this are very small (and are dominated by the n-th wedge product anyway). So I used the factorial from math to not pull-in some dependencies. But I can change this of course.

comment:117 in reply to: ↑ 116 ; follow-up: Changed 5 months ago by mkoeppe

Replying to gh-tobiasdiez:

to not pull-in some dependencies.

This does not matter here because sage.manifolds already depends on all of sage.symbolic and sage.functions

comment:118 in reply to: ↑ 115 Changed 5 months ago by mkoeppe

Replying to gh-tobiasdiez:

.. or perhaps AffineSymplecticSpace?.

Not sure if one needs this generalization, and even then one probably wants to say that its an affine space modeled on a symplectic vector space.

I don't know, one could just say it's an affine space with a translation-invariant symplectic form.

comment:119 in reply to: ↑ description ; follow-up: Changed 5 months ago by mkoeppe

Replying to gh-tobiasdiez:

TODO (as follow-up tickets):

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

We will have to be very careful with the design of this so that we do not add a 4th incompatible implementation of finite-dimensional vector spaces in Sage in this way. (We already have 3, implemented in sage.modules, sage.combinat.free_module, and sage.tensor.modules, respectively, see the constructor in https://doc.sagemath.org/html/en/reference/modules/sage/modules/free_module.html#sage.modules.free_module.FreeModule)

That's why I would urge to set the vector space side of things aside and only focus on the (affine) symplectic space here on this ticket.

The relation of

  • free modules with a symplectic form to SymplecticSpace
  • free modules with a positive definite form to EuclideanSpace

should be done in one follow up ticket.

Last edited 5 months ago by mkoeppe (previous) (diff)

comment:120 in reply to: ↑ 117 Changed 5 months ago by gh-mjungmath

Replying to mkoeppe:

Replying to gh-tobiasdiez:

to not pull-in some dependencies.

This does not matter here because sage.manifolds already depends on all of sage.symbolic and sage.functions

Unfortunately, I am not an expert in this import business. Why does sage.manifolds matter here? And doesn't factorial belong to sage.functions.other? Wouldn't that make a difference, too? Notice that you did the same in diff_form.py:

@@ -732,9 +746,22 @@ class DiffForm(TensorField):
         :meth:`~sage.manifolds.differentiable.metric.PseudoRiemannianMetric.hodge_star`
         for more examples.
         """
-        if metric is None:
-            metric = self._vmodule._ambient_domain.metric()
-        return metric.hodge_star(self)
+        from sage.functions.other import factorial

comment:121 in reply to: ↑ 119 Changed 5 months ago by egourgoulhon

Replying to mkoeppe:

Replying to gh-tobiasdiez:

TODO (as follow-up tickets):

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

We will have to be very careful with the design of this so that we do not add a 4th incompatible implementation of finite-dimensional vector spaces in Sage in this way. (We already have 3, implemented in sage.modules, sage.combinat.free_module, and sage.tensor.modules, respectively, see the constructor in https://doc.sagemath.org/html/en/reference/modules/sage/modules/free_module.html#sage.modules.free_module.FreeModule)

+1

That's why I would urge to set the vector space side of things aside and only focus on the (affine) symplectic space here on this ticket.

The relation of

  • free modules with a symplectic form to SymplecticSpace
  • free modules with a positive definite form to EuclideanSpace

should be done in one follow up ticket.

+1

In the current state, SymplecticVectorSpace is simply a subclass of EuclideanSpace, which is fine at this stage. Maybe, following Matthias' recommendation, just rename it to AffineSymplecticSpace (despite there is no affine structure explicitly implemented). IMHO, SymplecticSpace would be too general, sounding like a synonym for SymplecticManifold.

Besides, I agree with Michael: import factorial as in diff_form.py.

comment:122 follow-up: Changed 5 months ago by gh-mjungmath

To keep the old behavior of hodge_dual, I'd suggest to fall back on the metric if no input has been given, otherwise an AttributeError will be thrown (which didn't happen before):

         from sage.functions.other import factorial
         from sage.tensor.modules.format_utilities import format_unop_txt, \
                                                          format_unop_latex

+        if nondegenerate_tensor is None:
+            nondegenerate_tensor = self._vmodule._ambient_domain.metric()

         p = self.tensor_type()[1]
         eps = nondegenerate_tensor.volume_form(p)

Accordingly, this behavior should be documented.

Last edited 5 months ago by gh-mjungmath (previous) (diff)

comment:123 in reply to: ↑ 122 Changed 5 months ago by gh-mjungmath

Replying to gh-mjungmath:

To keep the old behavior of hodge_dual, I'd suggest to fall back on the metric if no input has been given, otherwise an AttributeError will be thrown (which didn't happen before):

         from sage.functions.other import factorial
         from sage.tensor.modules.format_utilities import format_unop_txt, \
                                                          format_unop_latex

+        if nondegenerate_tensor is None:
+            nondegenerate_tensor = self._vmodule._ambient_domain.metric()

         p = self.tensor_type()[1]
         eps = nondegenerate_tensor.volume_form(p)

Accordingly, this behavior should be documented.

In any case, if we really want to change this behavior, I'd opt for a more meaningful error message than NoneType object has no attribute....

comment:124 Changed 5 months ago by git

  • Commit changed from ee5b5528032a3994c1e9d711eba6d622e7319ea2 to afefe4aa449b492a4d756e5981f10f9dd59dcf3b

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

afefe4aImprove hodge_dual and use right factorial

comment:125 follow-up: Changed 5 months ago by gh-tobiasdiez

I've now implemented the your suggestions except for the name change to "AffineSymplecticSpace?". I agree that from a mathematical perspective "affine" has some justification, but in the end a affine symplectic space is diffeomorphic (even symplectomorphic) to a symplectic vector space; thus from a manifolds perspective there is no difference. More importantly, this class is meant as an example for symplectic manifolds (mostly for the doctests) and all textbooks talk about symplectic vector space but almost no one is interested in affine ones (google has like 800 hits for "affine symplectic space").

comment:126 Changed 5 months ago by mkoeppe

How is the vector space structure used? It seems to me that there is some conflation of the space and its (constant) tangent space happening.

comment:127 Changed 5 months ago by mkoeppe

Note that also most linear algebra textbooks also talk about vector spaces all the time even when they manipulate points in an affine space, not vectors in a vector space.

comment:128 Changed 5 months ago by gh-tobiasdiez

The manifold package currently doesn't provide a way to define a manifold abstractly (i.e. by specifying its points), instead a manifold is given in terms of its chart. But an affine and a vector space both have a single chart covering the whole space, the only difference being that there is no "canonical" choice in the affine case while there is one in the vector space setting.

The "correct" name of this class would be "a symplectic manifold symplectomorphic to R2n with its standard symplectic form". For me "symplectic vector space" is a good-enough approximation to this truth.

comment:129 follow-up: Changed 5 months ago by mkoeppe

This would be fine in a specialized system focused only on manifolds; but Sage covers many mathematical areas, and I am concerned that it is misleading to introduce a class that is called ...VectorSpace but is not in the category VectorSpaces.

comment:130 in reply to: ↑ 129 Changed 5 months ago by gh-mjungmath

Replying to gh-tobiasdiez:

The manifold package currently doesn't provide a way to define a manifold abstractly (i.e. by specifying its points), instead a manifold is given in terms of its chart.

All points on a manifold are already uniquely determined in terms of charts. In fact, due to the manifold structure, specifying a manifold by its points or by its charts is equivalent.

Replying to mkoeppe:

This would be fine in a specialized system focused only on manifolds; but Sage covers many mathematical areas, and I am concerned that it is misleading to introduce a class that is called ...VectorSpace but is not in the category VectorSpaces.

I agree. I also think that the best compromise for now is AffineSymplecticSpace.

comment:131 follow-ups: Changed 5 months ago by gh-tobiasdiez

But its currently neither an affine nor a vector space... so the question would be: should sage (at some point) has a proper implementation of an affine symplectic space or of a symplectic vector space? In my opinion, for practical purposes, the vector version should be sufficient - if you really only have an affine space, then just don't use the fact that you could add vectors.

Even if we should go with affine symplectic, how would one express the fact that the chart depends on a choice of a base point?

comment:132 Changed 5 months ago by mkoeppe

A key difference is that we do not currently have a category for affine spaces. There's merely one implementation, in sage.schemes, that implements AffineSpace from a scheme-theoretic viewpoint. Its category is just the category of schemes, nothing more detailed than that.

comment:133 in reply to: ↑ 131 Changed 5 months ago by gh-mjungmath

Replying to gh-tobiasdiez:

But its currently neither an affine nor a vector space... so the question would be: should sage (at some point) has a proper implementation of an affine symplectic space or of a symplectic vector space? In my opinion, for practical purposes, the vector version should be sufficient - if you really only have an affine space, then just don't use the fact that you could add vectors.

The important difference here is that, in a strict sense, the affine version has no vector space structure. If I understand Matthias's objection correctly, this is exactly the punchline.

comment:134 Changed 5 months ago by gh-mjungmath

In addition, notice that EuclideanSpace is meant to represent an affine space, too:

An *Euclidean space of dimension* `n` is an affine space `E`, whose associated
vector space is a `n`-dimensional vector space over `\RR` and is equipped with
a positive definite symmetric bilinear form, called the *scalar product* or
*dot product* [Ber1987]_. An Euclidean space of dimension `n` can also be
viewed as a Riemannian manifold that is diffeomorphic to `\RR^n` and that
has a flat metric `g`. The Euclidean scalar product is then that defined
by the Riemannian metric `g`.

From that viewpoint, since you derive from EuclideanSpace, it is only natural to view your implementation as an affine symplectic space.

Last edited 5 months ago by gh-mjungmath (previous) (diff)

comment:135 in reply to: ↑ 131 Changed 5 months ago by mkoeppe

Replying to gh-tobiasdiez:

the question would be: should sage (at some point) has a proper implementation of an affine symplectic space or of a symplectic vector space?

To make it a "proper" affine space, one only needs to implement lines, easy to do.

On the other hand, to make it a vector space, one would need to identify the space with its tangent space -- so that the bilinear form can be applied to elements of the space.

comment:136 follow-up: Changed 5 months ago by gh-mjungmath

@Eric: Speaking of which, that brings to my mind: do we want to implement a method vector_space for EuclideanSpace that returns the corresponding underlying vector space (w.r.t. a given frame)?

comment:137 Changed 5 months ago by gh-mjungmath

I think, affinness is sufficiently represented by:

sage: EuclideanSpace(3) is EuclideanSpace(3)
False

Thus, you get two distinct affine spaces, you may (or may not) relate to each other. Of course, lines would make this construction mathematically more rigorous.

comment:138 in reply to: ↑ 125 ; follow-ups: Changed 5 months ago by egourgoulhon

Replying to gh-tobiasdiez:

More importantly, this class is meant as an example for symplectic manifolds (mostly for the doctests) and all textbooks talk about symplectic vector space but almost no one is interested in affine ones (google has like 800 hits for "affine symplectic space").

This is a good point. Let us forget then about AffineSymplecticSpace. What about StandardSymplecticSpace? This would be in line with https://en.wikipedia.org/wiki/Symplectic_vector_space#Standard_symplectic_space.

comment:139 in reply to: ↑ 136 Changed 5 months ago by egourgoulhon

Replying to gh-mjungmath:

@Eric: Speaking of which, that brings to my mind: do we want to implement a method vector_space for EuclideanSpace that returns the corresponding underlying vector space (w.r.t. a given frame)?

This should probably be discussed in the follow up ticket mentioned in comment:119, in particular to decide which (Sage) type of vector space we want to return.

comment:140 in reply to: ↑ 138 Changed 5 months ago by gh-mjungmath

Replying to egourgoulhon:

Replying to gh-tobiasdiez:

More importantly, this class is meant as an example for symplectic manifolds (mostly for the doctests) and all textbooks talk about symplectic vector space but almost no one is interested in affine ones (google has like 800 hits for "affine symplectic space").

This is a good point. Let us forget then about AffineSymplecticSpace. What about StandardSymplecticSpace? This would be in line with https://en.wikipedia.org/wiki/Symplectic_vector_space#Standard_symplectic_space.

I am open to other suggestions, but I'd like to point out that the above implementation in fact represents an affine symplectic space since it derives from EuclideanSpace. Thus, there is no other more appropriate name than AffineSymplecticSpace.

And indeed, textbooks usually use symplectic vector spaces as primary example. But most textbooks also talk about the Euclidean vector space RR^n as primary example for Riemannian manifolds. Nevertheless, we decided for the affine version in Sage for the sake of convenience and (imho) accuracy. I say we should not break with it now.

Last edited 5 months ago by gh-mjungmath (previous) (diff)

comment:141 follow-ups: Changed 5 months ago by gh-tobiasdiez

Replying to mkoeppe:

Replying to gh-tobiasdiez:

the question would be: should sage (at some point) has a proper implementation of an affine symplectic space or of a symplectic vector space?

To make it a "proper" affine space, one only needs to implement lines, easy to do.

On the other hand, to make it a vector space, one would need to identify the space with its tangent space -- so that the bilinear form can be applied to elements of the space.

Well, you just declare the chart to be a linear isomorphism and your done carrying all the (vector space) structure over from Rn. Also easy ;-) To be precise, the new "SymplecticVectorSpace?" class currently represents nothing else then R2n with a fixed basis and its canonical symplectic form, with addition and scalar multiplation forgotten/not yet implemented. You could view this as an abstract affine symplectic space with a chart; but in order to define a chart on an affine space one needs to choose a base point and a basis.

For these reasons I really like the idea

What about StandardSymplecticSpace?

Should I go ahead and rename everything?

In addition, notice that EuclideanSpace? is meant to represent an affine space, too:

The documentation is at least claiming this, but EuclideanSpace? is also not a proper affine space either. You cannot subtract elements (at least I couldn't figure out how) and there is only one chart - but on an affine space you don't have a canonical chart; and once you fix a base point you have a vector space. Moreover, what are "polar coordinates" on an affine space? I think "EuclideanSpace?" is also just Rn with its Euclidean metric, so maybe it should be renamed to StandardEuclideanSpace? in a follow-up ticket?

Sorry for being so stubborn on this, but I would bet that an average graduate student has never seen the definition of an affine symplectic space (I certainly didn't) and so far I cannot see any good examples / uses cases why one would actually need them, especially for a CAS. I agree that the current implementation is not satisfying, but that's why I'd added it as a follow-up todo to implement the "vector space" part. Finally, and most importantly for me, I think the docstrings should try to be as understandable as possible, hiding as much mathematical complexity and focus on how the method/class are used - thus, I would like to use a symplectic vector space as a primary example in the documentation of a symplectic form and not an abstract concept one first has to lookup and understand.

comment:142 in reply to: ↑ 141 ; follow-up: Changed 5 months ago by egourgoulhon

Replying to gh-tobiasdiez:

For these reasons I really like the idea

What about StandardSymplecticSpace?

Should I go ahead and rename everything?

Yes please go on. If you agree with this name, this makes at least two of us and it does not collide with the name VectorSpace. We could still revert to SymplecticVectorSpace later on. IMHO, this nomenclature debate should not prevent this ticket to make its way into Sage 9.5, which is about to be released.

In addition, notice that EuclideanSpace? is meant to represent an affine space, too:

The documentation is at least claiming this, but EuclideanSpace? is also not a proper affine space either.

Actually, the documentation is not claiming that EuclideanSpace is an affine space, because the sentence just after the quoted one in comment:134 is

The current implementation of Euclidean spaces is based on the second point 
of view.

This second point of view is that of a Riemannian manifold, not of an affine space.

Sorry for being so stubborn on this, but I would bet that an average graduate student has never seen the definition of an affine symplectic space (I certainly didn't) and so far I cannot see any good examples / uses cases why one would actually need them, especially for a CAS.

+1

I agree that the current implementation is not satisfying, but that's why I'd added it as a follow-up todo to implement the "vector space" part. Finally, and most importantly for me, I think the docstrings should try to be as understandable as possible, hiding as much mathematical complexity and focus on how the method/class are used - thus, I would like to use a symplectic vector space as a primary example in the documentation of a symplectic form and not an abstract concept one first has to lookup and understand.

+1

comment:143 in reply to: ↑ 138 Changed 5 months ago by mkoeppe

Replying to egourgoulhon:

What about StandardSymplecticSpace?

+1, this is a great suggestion

comment:144 in reply to: ↑ 142 Changed 5 months ago by gh-mjungmath

Replying to egourgoulhon:

Actually, the documentation is not claiming that EuclideanSpace is an affine space, because the sentence just after the quoted one in comment:134 is

The current implementation of Euclidean spaces is based on the second point 
of view.

This second point of view is that of a Riemannian manifold, not of an affine space.

Fair enough.

All fine by me. :)

comment:145 Changed 5 months ago by git

  • Commit changed from afefe4aa449b492a4d756e5981f10f9dd59dcf3b to cdffbb2a587116f58e37ef8e409325b911aff05b

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

91fb04dMerge branch 'develop' of https://github.com/sagemath/sage into public/manifolds/symplectic
cdffbb2Rename to standard symplectic space

comment:146 follow-up: Changed 5 months ago by gh-mjungmath

-        return f"{self._dim}-dimensional symplectic vector space {self._name}"
+        return f"Standard symplectic vector space {self._name}"

This looks like cheating to me. Please correct me if I'm wrong, but wasn't the whole point that this implementation does not constitute a vector space? This would still leave the impression to the user that they can add/substract or scale points.

For the sake of consistency, I opt for:

-        return f"Standard symplectic vector space {self._name}"
+        return f"Standard symplectic space {self._name}"

Otherwise, we have to rename Euclidean space to Euclidean vector space, too. Which wouldn't make sense to me due to the aforementioned reasons.

comment:147 in reply to: ↑ 141 Changed 5 months ago by gh-mjungmath

Replying to gh-tobiasdiez:

Sorry for being so stubborn on this, but I would bet that an average graduate student has never seen the definition of an affine symplectic space (I certainly didn't) and so far I cannot see any good examples / uses cases why one would actually need them, especially for a CAS. I agree that the current implementation is not satisfying, but that's why I'd added it as a follow-up todo to implement the "vector space" part. Finally, and most importantly for me, I think the docstrings should try to be as understandable as possible, hiding as much mathematical complexity and focus on how the method/class are used - thus, I would like to use a symplectic vector space as a primary example in the documentation of a symplectic form and not an abstract concept one first has to lookup and understand.

Matthias already pointed out in comment:113 that symplectic vector spaces are at present part of Sage. More reasonable, in my opinion, would be a link between both implementations instead of equipping StandardSymplecticSpace with an additional vector space structure. See for example #30832 for a vaguely similar attempt.

Last edited 5 months ago by gh-mjungmath (previous) (diff)

comment:148 follow-up: Changed 5 months ago by gh-mjungmath

@Tobias: I'd like to point out that I perfectly understand your concerns. Actually, I dealt with similar issues when I started in the Sage project. Mathematical accuracy and implementation details cannot always be aligned, unfortunately. That being said, we cannot create Sage objects that unify all their mathematical properties. That's something Travis taught me with hours of patience. Isn't that right Travis? :D

comment:149 in reply to: ↑ 146 ; follow-ups: Changed 5 months ago by gh-tobiasdiez

Replying to gh-mjungmath:

-        return f"{self._dim}-dimensional symplectic vector space {self._name}"
+        return f"Standard symplectic vector space {self._name}"

This looks like cheating to me. Please correct me if I'm wrong, but wasn't the whole point that this implementation does not constitute a vector space? This would still leave the impression to the user that they can add/substract or scale points.

For the sake of consistency, I opt for:

-        return f"Standard symplectic vector space {self._name}"
+        return f"Standard symplectic space {self._name}"

Otherwise, we have to rename Euclidean space to Euclidean vector space, too. Which wouldn't make sense to me due to the aforementioned reasons.

I wasn't sure about this one. The "vector space" part was meant to convey that "R2" is really the vector space R^2 one knows and that the symplectic form is constant. Can be changed of course.

comment:150 in reply to: ↑ 148 ; follow-ups: Changed 5 months ago by gh-tobiasdiez

Replying to gh-mjungmath:

@Tobias: I'd like to point out that I perfectly understand your concerns. Actually, I dealt with similar issues when I started in the Sage project. Mathematical accuracy and implementation details cannot always be aligned, unfortunately. That being said, we cannot create Sage objects that unify all their mathematical properties. That's something Travis taught me with hours of patience. Isn't that right Travis? :D

I guess I still have much to learn ;-). So thanks for everyone's patience. Also, as a physicist by education, it is perhaps easier for me to ignore fine mathematical details (affine vs vector space) or mathematical structure (category theory) if that makes the implementation easier.


That being said, I still think it would be handy in applications if points of EuclideanSpace? could be added and scaled (without the need to resort to explicit coordinates).

comment:151 in reply to: ↑ 149 Changed 5 months ago by egourgoulhon

Replying to gh-tobiasdiez:

Replying to gh-mjungmath:

-        return f"{self._dim}-dimensional symplectic vector space {self._name}"
+        return f"Standard symplectic vector space {self._name}"

This looks like cheating to me. Please correct me if I'm wrong, but wasn't the whole point that this implementation does not constitute a vector space? This would still leave the impression to the user that they can add/substract or scale points.

For the sake of consistency, I opt for:

-        return f"Standard symplectic vector space {self._name}"
+        return f"Standard symplectic space {self._name}"

Otherwise, we have to rename Euclidean space to Euclidean vector space, too. Which wouldn't make sense to me due to the aforementioned reasons.

I wasn't sure about this one. The "vector space" part was meant to convey that "R2" is really the vector space R^2 one knows and that the symplectic form is constant. Can be changed of course.

Yes please change it for consistency, albeit I agree that "vector space" in the name would make perfectly clear that the object is what the user expect.

comment:152 in reply to: ↑ 150 Changed 5 months ago by egourgoulhon

Replying to gh-tobiasdiez:

That being said, I still think it would be handy in applications if points of EuclideanSpace? could be added and scaled (without the need to resort to explicit coordinates).

This would be handy indeed and there is no reason why this cannot be implemented in a future ticket.

comment:153 in reply to: ↑ 149 ; follow-up: Changed 5 months ago by gh-mjungmath

Replying to gh-tobiasdiez:

I wasn't sure about this one. The "vector space" part was meant to convey that "R2" is really the vector space R^2 one knows and that the symplectic form is constant. Can be changed of course.

Well, the Euclidean space is abbreviated with E^n and not R^n. Perhaps we should do the same here, too?

At least until we have the vector space structure on Euclidean space (though, I'd personally prefer an affine structure).

This all is indeed a bit of a hassle, and a lot of subtleties lie on the path. Maybe it is a good idea to nudge a discussion with the Sage (manifolds) community?

comment:154 Changed 5 months ago by gh-mjungmath

In my head, these things make perfect sense for me:

  • Euclidean space in Sage is an affine vector space.
  • The symplectic space above is an affine space, too, endowed with a symplectic structure.
  • As for the vector space structure, I think we should link the Euclidean space to preexisting vector spaces endowed with bilinear forms.

I am open to suggestions, different opinions and arguments. :)

But that's not for this ticket.

comment:155 follow-up: Changed 5 months ago by egourgoulhon

By the way, why don't you add StandardSymplecticSpace to the manifold catalog? This would avoid having to import it:

- sage: from sage.manifolds.differentiable.examples.symplectic_space import StandardSymplecticSpace
- sage: M.<q, p> = StandardSymplecticSpace(2, symplectic_name='omega')
+ sage: M.<q, p> = manifolds.StandardSymplecticSpace(2, symplectic_name='omega')

To do this, simply add

_lazy_import('sage.manifolds.differentiable.examples.symplectic_space',
             'StandardSymplecticSpace')

to the file src/sage/manifolds/catalog.py (see e.g. the example of Sphere in that file).

comment:156 in reply to: ↑ 153 ; follow-up: Changed 5 months ago by egourgoulhon

Replying to gh-mjungmath:

Well, the Euclidean space is abbreviated with E^n and not R^n. Perhaps we should do the same here, too?

-1. R^n is certainly much more standard here.

comment:157 in reply to: ↑ 156 Changed 5 months ago by gh-mjungmath

Replying to egourgoulhon:

Replying to gh-mjungmath:

Well, the Euclidean space is abbreviated with E^n and not R^n. Perhaps we should do the same here, too?

-1. R^n is certainly much more standard here.

Mhm. Well, since StandardSymplecticSpace inherits from EuclideanSpace, such an object still carries the structure of Euclidean space. I see your point though. Difficult...

comment:158 in reply to: ↑ 155 Changed 5 months ago by gh-mjungmath

Replying to egourgoulhon:

By the way, why don't you add StandardSymplecticSpace to the manifold catalog? This would avoid having to import it:

- sage: from sage.manifolds.differentiable.examples.symplectic_space import StandardSymplecticSpace
- sage: M.<q, p> = StandardSymplecticSpace(2, symplectic_name='omega')
+ sage: M.<q, p> = manifolds.StandardSymplecticSpace(2, symplectic_name='omega')

To do this, simply add

_lazy_import('sage.manifolds.differentiable.examples.symplectic_space',
             'StandardSymplecticSpace')

to the file src/sage/manifolds/catalog.py (see e.g. the example of Sphere in that file).

Big +1.

comment:159 Changed 5 months ago by git

  • Commit changed from cdffbb2a587116f58e37ef8e409325b911aff05b to cfe6a240128acbf6100b69f273178ae58bb9e10f

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

cfe6a24Remove vector

comment:160 Changed 5 months ago by git

  • Commit changed from cfe6a240128acbf6100b69f273178ae58bb9e10f to 2208dfce0918890a04bde3e90d23cfbd5e5f26ec

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

2208dfcUse catalog

comment:161 Changed 5 months ago by gh-tobiasdiez

Okay done

comment:162 in reply to: ↑ 150 Changed 5 months ago by tscrim

Replying to gh-tobiasdiez:

Replying to gh-mjungmath:

@Tobias: I'd like to point out that I perfectly understand your concerns. Actually, I dealt with similar issues when I started in the Sage project. Mathematical accuracy and implementation details cannot always be aligned, unfortunately. That being said, we cannot create Sage objects that unify all their mathematical properties. That's something Travis taught me with hours of patience. Isn't that right Travis? :D

I guess I still have much to learn ;-). So thanks for everyone's patience. Also, as a physicist by education, it is perhaps easier for me to ignore fine mathematical details (affine vs vector space) or mathematical structure (category theory) if that makes the implementation easier.

The "canonical" example of this is the real numbers. In implementations, we run up against things that are not quite mathematical, but we have to deal with the realities of finite and explicit computations.

comment:163 Changed 5 months ago by egourgoulhon

  • Status changed from needs_review to positive_review

Thanks for the changes!

comment:164 Changed 5 months ago by git

  • Commit changed from 2208dfce0918890a04bde3e90d23cfbd5e5f26ec to 8ca17243ec62a6152649cf9f0b2b8ae897b34146
  • Status changed from positive_review to needs_review

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

8ca1724#30362: fix documentation

comment:165 Changed 5 months ago by egourgoulhon

  • Status changed from needs_review to positive_review

The documentation was broken after the change of filename symplectic_vector_space.py --> symplectic_space.py. This is fixed in the latest commit.

comment:166 Changed 5 months ago by gh-tobiasdiez

Thanks for the review and the fix!

comment:167 Changed 5 months ago by mkoeppe

Looks good to me too.

comment:168 follow-up: Changed 5 months ago by gh-mjungmath

Last question: do we really need to import everything in the test files?

Otherwise, LGTM.

comment:169 in reply to: ↑ 168 Changed 5 months ago by gh-tobiasdiez

Replying to gh-mjungmath:

Last question: do we really need to import everything in the test files?

Otherwise, LGTM.

Sadly yes, at the moment, since there are a few cyclic imports that are only resolved using the sage.all import.

comment:170 Changed 5 months ago by gh-mjungmath

Then we should probably add a TODO comment so that we don't forget to rectify this in due course, no?

comment:171 Changed 5 months ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:172 Changed 5 months ago by git

  • Commit changed from 8ca17243ec62a6152649cf9f0b2b8ae897b34146 to b78d8a273a333cf59e66e09b9cb0380a28e88133

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

b78d8a2#30362: fix merge conflict

comment:173 Changed 5 months ago by egourgoulhon

  • Status changed from needs_work to positive_review

Merge conflict fixed.

comment:174 Changed 4 months ago by mkoeppe

  • Milestone changed from sage-9.5 to sage-9.6

comment:175 Changed 4 months ago by vbraun

  • Branch changed from public/manifolds/symplectic to b78d8a273a333cf59e66e09b9cb0380a28e88133
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:176 follow-up: Changed 4 months ago by egourgoulhon

  • Commit b78d8a273a333cf59e66e09b9cb0380a28e88133 deleted

Now that the branch has been merged in Sage 9.6.beta0 (unfortunately it did not make its way to Sage 9.5...), there remains a last thing to do: preparing some examples of use for Sage 9.6 release tour, on the same footing as what has been done for Sage 9.5: https://wiki.sagemath.org/ReleaseTours/sage-9.5#Manifolds. I've just added an entry for this in Sage 9.6 release tour. Tobias, could you provide some nice examples?

comment:177 follow-up: Changed 4 months ago by gh-tobiasdiez

Is a copy&paste from the doctests good enough?

Should I paste it here then, or how does one get access to the wiki?

comment:178 in reply to: ↑ 177 Changed 4 months ago by tscrim

Replying to gh-tobiasdiez:

Is a copy&paste from the doctests good enough?

It would be a bit better to give a very quick summary and some simple example usage (which can by c/p from the doctests).

Should I paste it here then, or how does one get access to the wiki?

You can edit the wiki by logging in with your trac account info. I don't know if it works for gh authentication.

comment:179 in reply to: ↑ 176 Changed 8 weeks ago by egourgoulhon

Replying to egourgoulhon:

Now that the branch has been merged in Sage 9.6.beta0 (unfortunately it did not make its way to Sage 9.5...), there remains a last thing to do: preparing some examples of use for Sage 9.6 release tour, on the same footing as what has been done for Sage 9.5: https://wiki.sagemath.org/ReleaseTours/sage-9.5#Manifolds. I've just added an entry for this in Sage 9.6 release tour. Tobias, could you provide some nice examples?

Ping (since the release of Sage 9.6 seems to be close).

comment:180 Changed 5 weeks ago by egourgoulhon

Another ping, pointing to the new release tour page, which is editable via a github login: https://trac.sagemath.org/wiki/ReleaseTours/sage-9.6?version=13#Symplecticmanifolds Tobias, would you have time to provide a few examples there?

comment:181 follow-up: Changed 5 weeks ago by gh-tobiasdiez

Sorry, I've been somewhat busy and forgot about this. Thanks for the reminder! I've now updated the release notes with a quick introduction.

comment:182 in reply to: ↑ 181 Changed 4 weeks ago by egourgoulhon

Replying to gh-tobiasdiez:

I've now updated the release notes with a quick introduction.

Thanks!

Note: See TracTickets for help on using tickets.