Opened 15 months ago

Last modified 3 months ago

#30362 needs_work enhancement

Add symplectic structures

Reported by: gh-tobiasdiez Owned by:
Priority: major Milestone: sage-9.5
Component: manifolds Keywords:
Cc: tscrim, nthiery, gh-mjungmath, egourgoulhon, mkoeppe Merged in:
Authors: Tobias Diez Reviewers:
Report Upstream: N/A Work issues:
Branch: public/manifolds/symplectic (Commits, GitHub, GitLab) Commit: daf5af854c415c751f3b053b75195efef6632e27
Dependencies: #30551 Stopgaps:

Status badges

Description (last modified by gh-tobiasdiez)

This PR adds symplectic structures. The basic things like Poisson brackets and Hamiltonian vector fields are implemented.

TODO (as follow-up tickets):

Change History (65)

comment:1 follow-up: Changed 15 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 14 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 14 months ago by gh-mjungmath (previous) (diff)

comment:3 Changed 14 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:4 in reply to: ↑ 1 ; follow-up: Changed 14 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 14 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 14 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 11 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 11 months ago by gh-tobiasdiez (previous) (diff)

comment:8 Changed 11 months ago by gh-tobiasdiez

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

comment:9 in reply to: ↑ 7 Changed 11 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 11 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 11 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 11 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 11 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 11 months ago by gh-tobiasdiez (previous) (diff)

comment:14 in reply to: ↑ 13 ; follow-up: Changed 11 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 11 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 11 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 11 months ago by gh-tobiasdiez (previous) (diff)

comment:17 in reply to: ↑ 16 Changed 11 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 11 months ago by gh-tobiasdiez

  • Authors set to Tobias Diez

comment:19 Changed 11 months ago by gh-tobiasdiez

  • Description modified (diff)

comment:20 Changed 11 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 11 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 10 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 10 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 10 months ago by gh-tobiasdiez

  • Dependencies #30901, #30748 deleted

comment:25 Changed 10 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 10 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 10 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 10 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 10 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 10 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 10 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 10 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 10 months ago by gh-tobiasdiez

  • Description modified (diff)

comment:34 Changed 10 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 10 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 10 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 10 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 10 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 10 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 10 months ago by mkoeppe

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

Added #30551 dependency because of future.annotations

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

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

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

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

comment:42 follow-up: Changed 10 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 10 months ago by gh-mjungmath (previous) (diff)

comment:43 Changed 10 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 10 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 10 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 10 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 10 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 10 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 10 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 10 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 10 months ago by gh-tobiasdiez (previous) (diff)

comment:51 in reply to: ↑ 50 ; follow-up: Changed 10 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 10 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 10 months ago by gh-tobiasdiez (previous) (diff)

comment:53 Changed 6 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 6 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 6 months ago by mkoeppe (previous) (diff)

comment:55 Changed 6 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 6 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 6 months ago by mkoeppe

Rebased, no longer on top of #30551

comment:58 Changed 6 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 6 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 6 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 6 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 6 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 6 months ago by gh-mjungmath (previous) (diff)

comment:63 Changed 6 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 5 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 3 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.

Note: See TracTickets for help on using tickets.