Add a bit of typing to manifold code

[tobiasdiez]

This PR adds a bit of typing information to some of the methods in the manifolds module.

CC: @tscrim @nthiery @mjungmath @fchapoton

Component: manifolds

Keywords: typing

Author: Tobias Diez

Branch/Commit: 7c18c7f

Reviewer: Matthias Koeppe, Tobias Diez, Eric Gourgoulhon

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

[mkoeppe]

comment:1

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]

comment:3

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

[sagetrac-git]

Changed commit from e620d05 to 23b6012

[sagetrac-git]

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

23b6012

Add more typing

[mkoeppe]

comment:5

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]

comment:6

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.

[tobiasdiez]

comment:8

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]

comment:9

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

[sagetrac-git]

Changed commit from 23b6012 to 3f93d31

[sagetrac-git]

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

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

[tobiasdiez]

comment:11

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]

comment:12

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]

comment:13

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

[egourgoulhon]

comment:14

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

[tobiasdiez]

comment:15

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. PyCQA/pyflakes#247 and vim-syntastic/syntastic#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. vim-syntastic/syntastic#1667 (comment)). 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]

comment:17

Replying to @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]

comment:18

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

[fchapoton]

comment:19

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

[sagetrac-git]

Changed commit from 3f93d31 to f348b54

[sagetrac-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

[sagetrac-git]

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

3a4e0eb

Remove pyright config

[mkoeppe]

comment:124

There's also one file that still manually string-quotes types instead of relying on from future import annotations.

[sagetrac-git]

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

f813f3c

src/sage/manifolds/differentiable/tensorfield.py: Don't string-quote types

[sagetrac-git]

Changed commit from 056a69d to f813f3c

[mkoeppe]

comment:126

Ready now, I think.

The patch is still bigger than really necessary as a result of Tobias's tool alphabetically sorting the imports.

Also the formatting of the typed argument lists uses a mix of styles across the file.

[mkoeppe]

comment:127

Tests still OK with Python 3.7

[tobiasdiez]

comment:128

As all remarks seem to be addressed, Matthias is fine with my changes and I'm with his, I'll set it back to positively reviewed.

[tobiasdiez]

Changed reviewer from Matthias Koeppe, ... to Matthias Koeppe, Tobias Diez

[egourgoulhon]

comment:129

There is a merge failure with Sage 9.6.beta6

[sagetrac-git]

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

51ee92b	Add a bit of typing to manifold code
974ec0a	src/sage/tensor/modules/free_module_tensor.py: Conditionalize an import with 'if TYPE_CHECKING:', add 'from `__future__` import annotations'
9905ade	src/sage/manifolds/differentiable/manifold.py: Conditionalize imports with 'if TYPE_CHECKING:'
ba751f1	src/sage/manifolds/differentiable/degenerate.py: Conditionalize an import with 'if TYPE_CHECKING:'
3beb020	src/sage/manifolds/differentiable/degenerate_submanifold.py: Conditionalize an import with 'if TYPE_CHECKING:'
7c18c7f	src/sage/manifolds/differentiable/tensorfield.py: Don't string-quote types

[sagetrac-git]

Changed commit from f813f3c to 7c18c7f

[mkoeppe]

comment:131

rebased

[egourgoulhon]

comment:132

The patchbot reveals some coverage issue, but this seems to be triggered by the use of @overload, hence this is more an issue about the patchbot's coverage plugin than about the present code.

[tobiasdiez]

comment:133

Thanks!

[mkoeppe]

Changed reviewer from Matthias Koeppe, Tobias Diez to Matthias Koeppe, Tobias Diez, Eric Gourgoulhon

[slel]

comment:135

Replying to @egourgoulhon:

The patchbot reveals some coverage issue, but this seems
to be triggered by the use of @overload, hence this is
more an issue about the patchbot's coverage plugin
than about the present code.

Might be worth opening a ticket at

• https://github.com/sagemath/sage-patchbot/issues

[egourgoulhon]

comment:136

Replying to @slel:

Might be worth opening a ticket at

• https://github.com/sagemath/sage-patchbot/issues

Thanks for the suggestion; this is now
sagemath/sage-patchbot#161

[vbraun]

Changed branch from public/manifolds/typing to 7c18c7f