Add a bit of typing to manifold code

[Tobias Diez]

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

[Matthias Köppe]

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

[Eric Gourgoulhon]

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

[Matthias Köppe]

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.

[Eric Gourgoulhon]

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.

[Tobias Diez]

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

[Eric Gourgoulhon]

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

[Tobias Diez]

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.

[Eric Gourgoulhon]

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

[Eric Gourgoulhon]

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

[Eric Gourgoulhon]

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

[Tobias Diez]

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.

[Matthias Köppe]

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

[Matthias Köppe]

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

[Frédéric 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

[Tobias Diez]

@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

[Tobias Diez]

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

[Tobias Diez]

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.

[Frédéric 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

[Tobias Diez]

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

[Frédéric 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

[Tobias Diez]

Merged upstream.

[Matthias Köppe]

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)

[Eric Gourgoulhon]

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 ?

[Eric Gourgoulhon]

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.

[Tobias Diez]

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?

[Eric Gourgoulhon]

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

[Tobias Diez]

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.

[Frédéric 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

[Tobias Diez]

Merged develop branch.

[Matthias Köppe]

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

[Tobias Diez]

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

[Eric Gourgoulhon]

The patchbot reports many doctest errors.

[Matthias Köppe]

Also, now that we can use from __future__ import annotations, I think some of the type annotations can be simplified

[Matthias Köppe]

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

[Tobias Diez]

Sorry for the delay, but the doctests should be fixed now. I also use the future import for annotations, which indeed simplifies a few things. Again ready for review.

[git]

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

​b45edce	Merge branch 'develop' of https://github.com/sagemath/sage into public/manifolds/typing
​fc15d33	Improve typing

[git]

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

​3f16aee

Fix tests

[Matthias Köppe]

Stalled in needs_review or needs_info; likely won't make it into Sage 9.5.

[Frédéric Chapoton]

red branch => needs work

[Tobias Diez]

Setting it back to needs review as the merge conflict should be easy to resolve but I'm not near my dev machine for a couple of days.

It would be really nice if someone could review this ticket, as it's in this limbo state now for quite some time.

[git]

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

​69c0943

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

[Matthias Köppe]

red branch

[Tobias Diez]

Merging the develop branch should be trivial, but I'll not be able to do this is in the near future; so setting it back to "needs review".

[git]

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

​229fde1

Merge remote-tracking branch 'origin/develop' into public/manifolds/typing

[Matthias Köppe]

--- a/src/sage/manifolds/differentiable/metric.py
+++ b/src/sage/manifolds/differentiable/metric.py
@@ -41,12 +41,16 @@ REFERENCES:
 #  the License, or (at your option) any later version.
 #                  https://www.gnu.org/licenses/
 # *****************************************************************************
+from __future__ import annotations
+from typing import overload, TYPE_CHECKING
 
 from __future__ import annotations
 from typing import TYPE_CHECKING
 from sage.rings.integer import Integer
 from sage.manifolds.differentiable.tensorfield import TensorField
 from sage.manifolds.differentiable.tensorfield_paral import TensorFieldParal
+if TYPE_CHECKING:
+    from sage.manifolds.differentiable.diff_form import DiffForm
 
 if TYPE_CHECKING:
     from sage.manifolds.differentiable.diff_form import DiffForm

This doesn't look right

[Matthias Köppe]

And I would urge to use more restraint in changing the coding style. For example, these changes are entirely gratuitous:

     if structure is None:
-        diff_degree = extra_kwds.get('diff_degree', infinity)
+        diff_degree = extra_kwds.get("diff_degree", infinity)
         if diff_degree == infinity:
-            structure = 'smooth'
+            structure = "smooth"
         elif diff_degree > 0:
-            structure = 'differentiable'
+            structure = "differentiable"
         else:
-            structure = 'topological'
+            structure = "topological"

[git]

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

​4290c24

Fix style

[Tobias Diez]

Replying to mkoeppe:

--- a/src/sage/manifolds/differentiable/metric.py
+++ b/src/sage/manifolds/differentiable/metric.py
This doesn't look right

Thanks, I was still in the process of fixing some of the merge conflicts. Done now.

[Matthias Köppe]

You didn't address comment:69

[Matthias Köppe]

And this change is an API change (= introduces a bug).

The argument tensor_type does NOT have Tuple[int, int] as its input type. It also accepts lists. It marshals the input to a tuple.

Your change removes the marshaling.

 TensorType = Tuple[int, int]

@@ -402,8 +405,17 @@ class TensorField(ModuleElementWithMutability):
         ValueError: the name of an immutable element cannot be changed
 
     """
-    def __init__(self, vector_field_module, tensor_type, name=None,
-                 latex_name=None, sym=None, antisym=None, parent=None):
+
+    def __init__(
+        self,
+        vector_field_module: VectorFieldModule,
+        tensor_type: TensorType,
+        name: Optional[str] = None,
+        latex_name: Optional[str] = None,
+        sym=None,
+        antisym=None,
+        parent=None,
+    ):
         r"""
         Construct a tensor field.
 
@@ -451,7 +463,7 @@ class TensorField(ModuleElementWithMutability):
             parent = vector_field_module.tensor_module(*tensor_type)
         ModuleElementWithMutability.__init__(self, parent)
         self._vmodule = vector_field_module
-        self._tensor_type = tuple(tensor_type)
+        self._tensor_type = tensor_type
         self._tensor_rank = self._tensor_type[0] + self._tensor_type[1]
         self._is_zero = False # a priori, may be changed below or via
                               # method __bool__()

[Tobias Diez]

Replying to mkoeppe:

And this change is an API change (= introduces a bug).

The argument tensor_type does NOT have Tuple[int, int] as its input type. It also accepts lists. It marshals the input to a tuple.

According to the documentation

• tensor_type -- pair (k,l) with k being the contravariant rank

and l the covariant rank

which is quite clear about that it expects a 2-tuple. Thus, this change is only a formalization of the current public interface; I can of course revert it and adapt the documentation to point out that it also accepts arbitrary iterables if that's really desired. It feels however more consistent to represent the tensor type everywhere by a tuple.

[Tobias Diez]

Replying to mkoeppe:

You didn't address comment:69

I accidentally run a code formatter over these files and committed the changes; sorry for this. Will try not to do this again in the future.

[Matthias Köppe]

Replying to gh-tobiasdiez:

Replying to mkoeppe:

And this change is an API change (= introduces a bug).

The argument tensor_type does NOT have Tuple[int, int] as its input type. It also accepts lists. It marshals the input to a tuple.

According to the documentation

• tensor_type -- pair (k,l) with k being the contravariant rank

and l the covariant rank

which is quite clear about that it expects a 2-tuple.

Well, the code obviously was concerned about making it a tuple to provide a robust interface.

Thus, this change is only a formalization of the current public interface

No, this is not how Python typing works. You cannot rely on the type annotations for input checking/marshaling; this still has to be done by code. Type annotations are merely decoration for documentation purposes and static checkers.

[git]

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

​8e911a4	Readd tuple
​b7d121c	Merge remote-tracking branch 'origin/develop' into public/manifolds/typing

[Matthias Köppe]

I think these changes are not good. They are obscuring the internal data representation

@@ -847,11 +847,11 @@ class TopologicalSubmanifold(TopologicalManifold):
              3-dimensional topological manifold M
 
         """
-        if not self._immersed:
+        if not self._immersed or not self._immersion:
             raise ValueError("the submanifold is not immersed")
         return self._immersion

[...] 
@@ -873,7 +873,7 @@ class TopologicalSubmanifold(TopologicalManifold):
              3-dimensional topological manifold M
 
         """
-        if not self._embedded:
+        if not self._embedded or not self._immersion:
             raise ValueError("the submanifold is not embedded")
         return self._immersion
 

[Matthias Köppe]

I'm personally in favor of these changes (the 80 character limit is a bit inflexible when module names or identifiers are already 40-ish characters long), but probably @egourgoulhon should indicate whether these formatting changes are acceptable / welcome.

@@ -647,8 +654,8 @@ class VectorFieldModule(UniqueRepresentation, Parent):
             for more examples and documentation.
 
         """
-        from sage.manifolds.differentiable.diff_form_module import \
-                                                                 DiffFormModule
+        from sage.manifolds.differentiable.diff_form_module import DiffFormModule
+
         if p == 0:
             return self._ring
         if p not in self._dual_exterior_powers:

[Tobias Diez]

I don't understand what you mean. How can a submanifold be considered to be immersed/embedded if there is map that provides such an immersion?

[Matthias Köppe]

It is an invariant of the internal representation (data structure) that _immersed or _embedded implies that _immersion is set.

[Tobias Diez]

Replying to mkoeppe:

It is an invariant of the internal representation (data structure) that _immersed or _embedded implies that _immersion is set.

But that's not something a static checker could infer and thus they warn that return self._immersion might return null although the method is decorated to always return a continuous map.

[Matthias Köppe]

It may be a use case for an assert _immersion here. If the static checker still can't infer it after that, get a better one

[Tobias Diez]

Replying to mkoeppe:

It may be a use case for an assert _immersion here. If the static checker still can't infer it after that, get a better one

But that's what the above code is doing (except for displaying a nicer error message in the case its null). What a type checker cannot infer is the "invariant of the internal representation" that _immersed is true implies that _immersion is not null. There are workarounds for such implications for type checkers but they are overkill in this situation.

[Matthias Köppe]

No, your changes weaken the invariant, indicating to developers that it's OK to set _immersed to True and _immersion to None.

[Tobias Diez]

Would it be okay if I completely replace the _immersed variable with checks if _immersion is None? As you indirectly noticed, keeping the internal state consistent just creates an unnecessary overhead.

[Matthias Köppe]

Try assert if you haven't yet.

[git]

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

​3dd340d

Use assert

[Matthias Köppe]

To separate the changes that the ticket promises from the reformatting, I'd recommend git merge 9.6.beta5 && git reset 9.6.beta5 && git add -p.

[Tobias Diez]

Just tried it on gitpod: merge: 9.6.beta5 - not something we can merge

[Matthias Köppe]

Fetch it first: git fetch trac 9.6.beta5

[git]

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

​0a52165

Add a bit of typing to manifold code

[Tobias Diez]

That also don't work, but I've managed to revert (most of) the formatting changes now. Still don't understand why its worth that we both spend more time discussing this than it would to have been to review.

[Matthias Köppe]

I think time would be well spent on some git practice...

[Matthias Köppe]

Please learn to use git reset and git add -p and prepare a ticket that only changes what needs to be changed. comment:69 still unaddressed.

[Matthias Köppe]

Replying to gh-tobiasdiez:

Still don't understand why its worth that we both spend more time discussing this than it would to have been to review.

Then please prepare a focused ticket instead of forcing reviewers to look at all that stuff.

[Tobias Diez]

Replying to mkoeppe:

I think time would be well spent on some git practice...

Thanks for this very constructive and helpful comment. Definitely motivated me to not spend any more time in reverting formatting changes.

If you really think its worth the time to extract the homogenization of quotes around strings in 3 lines to a new ticket, please be my guest.

[Matthias Köppe]

There's no need for a new ticket because the change is not an improvement.

[git]

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

​c3239f7

Add a bit of typing to manifold code

[Matthias Köppe]

Replying to gh-tobiasdiez:

please be my guest.

here we go

[Matthias Köppe]

This is now focused on the change described on the ticket, so I can review it without needing to consult with the original authors regarding style changes (comment:80).

[Matthias Köppe]

Replying to gh-tobiasdiez:

why its worth that we both spend more time discussing this

There's no need to worry about my time - I'm investing it for the long term benefits.

[Matthias Köppe]

OK, ./sage -tox -e pyright src/sage/manifolds/topological_submanifold.py (#30411) does not report additional failures introduced here, so it looks like the solution with assert worked.

[Matthias Köppe]

I've also successfully tested ./sage -tp --long src/sage/manifolds/ with python 3.7.

[Matthias Köppe]

Thanks for this contribution.

[Tobias Diez]

Thanks

[Michael Jung]

Why do we have doc changes in here such as:

-    the *vector field module* `\mathfrak{X}(U,\Phi)` is the set of
+    the *vector field module* `\mathfrak{X}(U,\Phi) = \Gamma(\Phi^* TM)` is the set of

This should not be part of this ticket, should it?

I agree that the doc could use some upgrades, but I suggest we do that in another ticket, and then thoroughly.

[Michael Jung]

Here's the corresponding follow-up: #33542

[git]

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

​10bdad9

Add a bit of typing to manifold code

[Matthias Köppe]

I've reduced the scope of the ticket as requested

[Matthias Köppe]

The documentation changes are now on #33542.

[Eric Gourgoulhon]

I really don't like the extra imports like

+ from sage.tensor.modules.free_module_basis import FreeModuleBasis

in src/sage/tensor/modules/free_module_tensor.py. They are used only for type checking. On a pure Python side, they make the code more complicated (and potentially slower) for nothing. Do we have a policy at the level of all Sage for this?

[Matthias Köppe]

We can move them into if TYPE_CHECKING: blocks.

[Matthias Köppe]

I believe this the currently preferred solution, at least until ​https://peps.python.org/pep-0649/ hits. I don't think we need a Sage-specific policy on this.

[git]

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

​a3905b5

src/sage/tensor/modules/free_module_tensor.py: Conditionalize an import with 'if TYPE_CHECKING:', add 'from __future__ import annotations'

[Eric Gourgoulhon]

Replying to mkoeppe:

We can move them into if TYPE_CHECKING: blocks.

Yes, at least this would be consistent with other parts of the code introduced in this ticket where if TYPE_CHECKING: is used.

[git]

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

​b2a68dc

src/sage/manifolds/differentiable/manifold.py: Conditionalize imports with 'if TYPE_CHECKING:'

[git]

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

​2061a50

src/sage/manifolds/differentiable/degenerate.py: Conditionalize an import with 'if TYPE_CHECKING:'

[git]

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

​056a69d

src/sage/manifolds/differentiable/degenerate_submanifold.py: Conditionalize an import with 'if TYPE_CHECKING:'

[Matthias Köppe]

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

[git]

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

​f813f3c

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

[Matthias Köppe]

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.

[Matthias Köppe]

Tests still OK with Python 3.7

[Tobias Diez]

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.

[Eric Gourgoulhon]

There is a merge failure with Sage 9.6.beta6

[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

[Matthias Köppe]

rebased

[Eric Gourgoulhon]

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.

[Tobias Diez]

Thanks!

[Samuel Lelièvre]

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

[Eric Gourgoulhon]

Replying to slelievre:

Might be worth opening a ticket at

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

Thanks for the suggestion; this is now ​https://github.com/sagemath/sage-patchbot/issues/161