Add a bit of typing to manifold code

[gh-tobiasdiez]

This PR adds a bit of typing information to some of the methods in the manifolds module that I added while browsing the code. If these types of changes are ok, I'll add further typing information as I proceed understanding the code.

Moreover, a few very minor clarifications to the documentation were added as well.

[mkoeppe]

In case people are interested in a general discussion of typing in the Sage library: #29756 Meta-ticket: Review of Python 3 features that sagelib should use systematically

[egourgoulhon]

The addition of identity_map to DifferentiableManifold is code duplication (the code is identical to identity_map provided by the mother class TopologicalManifold). I'm not sure adding typing is worth duplicating the code, opening the root to all evil...

[git]

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

​23b6012

Add more typing

[mkoeppe]

Replying to egourgoulhon:

The addition of identity_map to DifferentiableManifold is code duplication (the code is identical to identity_map provided by the mother class TopologicalManifold). I'm not sure adding typing is worth duplicating the code, opening the root to all evil...

I think this is a very important point, and one that needs discussion in a broader context.

1) Can the category system be used to add these tightened type annotations automatically?

2) In such situations, should the doctests be replaced or augmented by _test_... methods so that subclasses are tested properly.

[egourgoulhon]

Replying to egourgoulhon:

The addition of identity_map to DifferentiableManifold is code duplication (the code is identical to identity_map provided by the mother class TopologicalManifold). I'm not sure adding typing is worth duplicating the code, opening the root to all evil...

If we don't duplicate the code, then the output type of identity_map is indicated as ContinuousMap, which is correct for a smooth manifold as well. Of course, for such a manifold, it is a particular kind of continuous map, namely a smooth one and the returned object pertains to the subclass DiffMap of ContinuousMap. But I would say we can live with this; IMHO, it is preferable not to duplicate the code and have type hints not as precise as they could be.

[gh-tobiasdiez]

It's true that I noticed while working on typing support, but the addition is actually rather independent of this. As Eric says, identity_map on a differentiable manifold only returns a ContinuousMap and one can thus not take it's differential etc. So far this doesn't lead to any problems since the identity case is always handled separately in the code. I'm not familiar with the category code, but maybe it can be improved so that the function def hom_set: return Hom(self, self) defined in class A, returns the correct homomrophisms for derived classes B(A) as well (so b.hom_set returns the hom set of b as an element of B).

If it were just for typing, one could also annotate the original identity_map on TopologicalManifold (see ​https://mypy.readthedocs.io/en/stable/more_types.html#advanced-uses-of-self-types).

[egourgoulhon]

Replying to gh-tobiasdiez:

It's true that I noticed while working on typing support, but the addition is actually rather independent of this. As Eric says, identity_map on a differentiable manifold only returns a ContinuousMap and one can thus not take it's differential etc.

No, one can take the differential because the returned object in this case is actually a DiffMap, which is also a ContinuousMap since DiffMap is a subclass of ContinuousMap (see the example below).

I'm not familiar with the category code, but maybe it can be improved so that the function def hom_set: return Hom(self, self) defined in class A, returns the correct homomrophisms for derived classes B(A) as well (so b.hom_set returns the hom set of b as an element of B).

No need for improvement, this is already the case:

sage: M = Manifold(2, 'M'); M
2-dimensional differentiable manifold M
sage: Id = M.identity_map()
sage: type(Id)
<class 'sage.manifolds.differentiable.manifold_homset.DifferentiableManifoldHomset_with_category.element_class'>
sage: isinstance(Id, sage.manifolds.differentiable.diff_map.DiffMap)
True
sage: isinstance(Id, sage.manifolds.continuous_map.ContinuousMap)
True
sage: Id.parent()
Set of Morphisms from 2-dimensional differentiable manifold M 
 to 2-dimensional differentiable manifold M in Category of 
 smooth manifolds over Real Field with 53 bits of precision

[git]

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

​ca06162	Remove identity_map duplication
​3f93d31	Add mininmal configuration for type checking

[gh-tobiasdiez]

Thanks for the clarification. I actually tested it before, but apparently made some mistakes as identity_map indeed returns a DiffMap on a differentiable manifold.

I've thus removed the identity_map method again from DifferentiableManifold and instead added the typing information to the existing map.

Anything else that needs improvement? From my side this is good to go. I would add more typing in follow-up issues to keep the size of the changes small and reviewable.

[egourgoulhon]

There are some doctest errors, as well as coverage, pyflakes and pycodeystyle errors; check the patchbot reports (click on the "9.2.beta0 button" in the top right of the ticket description).

[egourgoulhon]

Another thing: the typing information does not show up in the html documentation produced by Sphinx.

[egourgoulhon]

The file pyrightconfig.json at the root of the Sage directory has probably been added to the commit by mistake.

[gh-tobiasdiez]

I had a quick look at the errors and most of them are a result of that the tools (pyflakes and pycodestyle) have problems with the typing syntax. After a bit of googling, it appears that these issues are fixed in the most recent versions (e.g. ​https://github.com/PyCQA/pyflakes/issues/247 and ​https://github.com/vim-syntastic/syntastic/issues/1667). Where do I find the version used? Moreover, one common problems seems to be that these tools need to be installed under python 3, since otherwise they check the files using python 2 syntax (e.g. ​https://github.com/vim-syntastic/syntastic/issues/1667#issuecomment-172538109). How can I make sure that it's the case indeed? I'll have a look at the remaining issues after these false positives are fixed.

Finally, the pyrightconfig.json is the configuration file for pyright, which is static type checker. At the moment there are still to many type problems but in the long term this should be integrated in the build progress.

[mkoeppe]

Replying to gh-tobiasdiez:

Finally, the pyrightconfig.json is the configuration file for pyright, which is static type checker. At the moment there are still to many type problems but in the long term this should be integrated in the build progress.

Could you create a separate ticket for this please and link to it from #28936

[mkoeppe]

Replying to gh-tobiasdiez:

I had a quick look at the errors and most of them are a result of that the tools (pyflakes and pycodestyle) have problems with the typing syntax. ... Where do I find the version used?

Best to open an issue on ​https://github.com/sagemath/sage-patchbot

[chapoton]

branch is red and needs to be rebased on the latest beta

[git]

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

​f348b54

Merge branch 'develop' of git://github.com/sagemath/sage into public/manifolds/typing

[git]

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

​3a4e0eb

Remove pyright config

[gh-tobiasdiez]

@mkoeppe done! @chapoton rebased!

The pyflakes tests still fail because pyflakes does not (fully) understand the typings code. Not sure if the reason is an old pyflakes or if this is even unresolved in the most recent version. Refs ​https://github.com/sagemath/sage-patchbot/issues/143. Can I disable pyflakes for the lines that are wrongly reported?

[gh-kliem]

sage -t --long --warn-long 63.8 --random-seed=0 src/sage/manifolds/manifold.py
**********************************************************************
File "src/sage/manifolds/manifold.py", line 2203, in sage.manifolds.manifold.TopologicalManifold.identity_map
Failed example:
    isinstance(id, DiffMap)
Exception raised:
    Traceback (most recent call last):
      File "/srv/public/patchbot-sage/local/lib/python3.7/site-packages/sage/doctest/forker.py", line 715, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/srv/public/patchbot-sage/local/lib/python3.7/site-packages/sage/doctest/forker.py", line 1139, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.manifolds.manifold.TopologicalManifold.identity_map[10]>", line 1, in <module>
        isinstance(id, DiffMap)
    NameError: name 'DiffMap' is not defined
**********************************************************************
File "src/sage/manifolds/manifold.py", line 2209, in sage.manifolds.manifold.TopologicalManifold.identity_map
Failed example:
    isinstance(M.identity_map(), DiffMap)
Exception raised:
    Traceback (most recent call last):
      File "/srv/public/patchbot-sage/local/lib/python3.7/site-packages/sage/doctest/forker.py", line 715, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/srv/public/patchbot-sage/local/lib/python3.7/site-packages/sage/doctest/forker.py", line 1139, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.manifolds.manifold.TopologicalManifold.identity_map[12]>", line 1, in <module>
        isinstance(M.identity_map(), DiffMap)
    NameError: name 'DiffMap' is not defined

[gh-tobiasdiez]

Thanks for your comment. How are imports for the documentation handled? Should I simply add the import for the whole file, or is there a special way to do this just for the doc tests?

[gh-kliem]

I think you can just do

from sage.manifolds.differentiable.diff_map import DiffMap

at the beginning of the doctest or

isinstance(id, sage.manifolds.differentiable.diff_map.DiffMap)

After building your modified sage, the doctest should pass with

sage -t --long src/sage/manifolds/manifold.py

The doctest works almost like a mini sage session.

[git]

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

​40aab0b	Merge branch 'develop' of git://github.com/sagemath/sage into public/manifolds/typing
​45c55d6	Fix patchbot warnings

[git]

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

​4827cd2

Fix import problem

[gh-tobiasdiez]

I've finally fixed these missing imports (or so I hope, I cannot run sage -t because pip couldn't be installed via make -- I will report this elsewhere). Thanks @gh-kliem for the helpful instructions.

The patchbot warnings should be fixed now as well. Note that the negative coverage changes are due to the fact that @overload seems not to be supported.

[chapoton]

red branch => needs work

[git]

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

​c9bf862

Merge branch 'develop' of git://github.com/sagemath/sage into public/manifolds/typing

[gh-tobiasdiez]

Thanks. I've now merged upstream into this branch.

[chapoton]

red branch => needs work

[git]

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

​5db7ddf

Merge develop

[git]

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

​cd9ef9e

Merge branch 'develop' of git://github.com/sagemath/sage into public/manifolds/typing

[gh-tobiasdiez]

Merged upstream.

[mkoeppe]

In changes like this one:

@@ -596,14 +597,13 @@ class DegenerateSubmanifold(DegenerateManifold, DifferentiableSubmanifold):
         self._default_screen = self._screens[name]
         return self._screens[name]
 
-    def induced_metric(self):
+    def induced_metric(self) -> DegenerateMetric:
         r"""
         Return the pullback of the ambient metric.
 
         OUTPUT:
 
-        - induced metric, as an instance of
-          :class:`~sage.manifolds.differentiable.metric.DegenerateMetric`
+        - induced mettric
 
         EXAMPLES:

Did you check that our built documentation contains the output type?

(Also note this introduces a typo)

[egourgoulhon]

Replying to mkoeppe:

In changes like this one:

-    def induced_metric(self):
+    def induced_metric(self) -> DegenerateMetric:
 
-        - induced metric, as an instance of
-          :class:`~sage.manifolds.differentiable.metric.DegenerateMetric`
+        - induced mettric

Did you check that our built documentation contains the output type?

No it does not: the output type does no longer appear in the generated html documentation.

(Also note this introduces a typo)

Moreover, what is the meaning of this change in src/sage/manifolds/differentiable/manifold.py:

-    def metric(self, name, signature=None, latex_name=None, dest_map=None):
+    def metric(self, name=None, signature=None, latex_name=None, dest_map=None) -> PseudoRiemannianMetric:

i.e. why name is replaced by name=None ?

[egourgoulhon]

If we do

sage: M = Manifold(2, 'M')                                                                          
sage: M.vector_field_module?                                                                        

we get

Signature:     
M.vector_field_module(
    dest_map: sage.manifolds.differentiable.diff_map.DiffMap = None,
    force_free=False,
) -> sage.manifolds.differentiable.vectorfield_module.VectorFieldModule

which is barely readable, I think.

[gh-tobiasdiez]

I'm currently working on a solution to show the typing information in the documentation using ​https://pypi.org/project/sphinx-autodoc-typehints/ (in a new ticket of course).

Not sure why I changed the name to None. Probably I was "inspired" by the constructor of a generic tensor field:

def __init__(self, vector_field_module, tensor_type, name=None,
                 latex_name=None, sym=None, antisym=None, parent=None):

Does the name has to be set for a metric?

[egourgoulhon]

Replying to gh-tobiasdiez:

Not sure why I changed the name to None. Probably I was "inspired" by the constructor of a generic tensor field:

def __init__(self, vector_field_module, tensor_type, name=None,
                 latex_name=None, sym=None, antisym=None, parent=None):

Does the name has to be set for a metric?

Yes for it is used to set the name of derived quantities (Levi-Civita connection, curvature tensor, etc.). Of course, this can be discussed and changed, but in another ticket, since this definitely does not pertain to the current ticket.

[git]

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

​db5a3fc	Readd documentation
​0c937af	Remove none for metric

[gh-tobiasdiez]

I tried to change the documentation to incorporate typing information, but this turns out to be a bit more complicated than expected. So for now I've readded the class information in the documentation.

Moreover, I've removed the name = None for the metric again.

So it should be ready for review now.

[chapoton]

red branch

[git]

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

​06770cd

Merge branch 'develop' of git://github.com/sagemath/sage into public/manifolds/typing

[gh-tobiasdiez]

Merged develop branch.

[mkoeppe]

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

[gh-tobiasdiez]

It would be nice if this ticket could be reviewed. Thanks!

[egourgoulhon]

The patchbot reports many doctest errors.