Opened 17 months ago

Last modified 3 months ago

#29775 needs_work enhancement

Add a bit of typing to manifold code

Reported by: gh-tobiasdiez Owned by:
Priority: minor Milestone: sage-9.5
Component: geometry Keywords: typing
Cc: tscrim, nthiery, gh-mjungmath, chapoton Merged in:
Authors: Tobias Diez Reviewers:
Report Upstream: N/A Work issues:
Branch: public/manifolds/typing (Commits, GitHub, GitLab) Commit: 06770cdb3dafa5f0833b96580675e2bf847d5ca3
Dependencies: Stopgaps:

Status badges

Description (last modified by 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.

Change History (55)

comment:1 Changed 17 months ago by 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

comment:2 Changed 17 months ago by gh-tobiasdiez

  • Status changed from new to needs_review

comment:3 follow-ups: Changed 17 months ago by 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...

comment:4 Changed 17 months ago by git

  • Commit changed from e620d059f0f89f840fa2b0091477e7a7ddaa6630 to 23b6012d4b5a2f9dbd1b8a07f454ec29191bf0a3

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

23b6012Add more typing

comment:5 in reply to: ↑ 3 Changed 17 months ago by mkoeppe

  • Cc tscrim nthiery added

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.

comment:6 in reply to: ↑ 3 Changed 17 months ago by 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.

comment:7 Changed 17 months ago by egourgoulhon

  • Description modified (diff)
  • Summary changed from Add a bit of typing to manifolds package to Add a bit of typing to manifold code

comment:8 follow-up: Changed 17 months ago by 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).

comment:9 in reply to: ↑ 8 Changed 17 months ago by 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

comment:10 Changed 17 months ago by git

  • Commit changed from 23b6012d4b5a2f9dbd1b8a07f454ec29191bf0a3 to 3f93d314779566116609cf208ba3651fbdadca03

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

ca06162Remove identity_map duplication
3f93d31Add mininmal configuration for type checking

comment:11 Changed 17 months ago by 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.

comment:12 Changed 17 months ago by 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).

comment:13 Changed 17 months ago by egourgoulhon

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

comment:14 Changed 17 months ago by egourgoulhon

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

comment:15 follow-ups: Changed 17 months ago by 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.

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

comment:16 Changed 15 months ago by gh-mjungmath

  • Cc gh-mjungmath added

comment:17 in reply to: ↑ 15 Changed 15 months ago by 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

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

  • Cc chapoton added

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

comment:19 Changed 14 months ago by chapoton

  • Status changed from needs_review to needs_work

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

comment:20 Changed 14 months ago by git

  • Commit changed from 3f93d314779566116609cf208ba3651fbdadca03 to f348b54a4ddb2d0bf45836af3b12ab2b460558d0

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

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

comment:21 Changed 14 months ago by git

  • Commit changed from f348b54a4ddb2d0bf45836af3b12ab2b460558d0 to 3a4e0ebeb1ae217cae58fbd8c8e645eed6e1e7b0

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

3a4e0ebRemove pyright config

comment:22 Changed 14 months ago by 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?

comment:23 Changed 14 months ago by gh-tobiasdiez

  • Status changed from needs_work to needs_review

comment:24 Changed 14 months ago by gh-kliem

  • Status changed from needs_review to needs_work
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

comment:25 Changed 14 months ago by 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?

comment:26 Changed 14 months ago by 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.

comment:27 Changed 14 months ago by git

  • Commit changed from 3a4e0ebeb1ae217cae58fbd8c8e645eed6e1e7b0 to 45c55d6a05ec9305751db34bbb4ce24a3d04c2fc

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

40aab0bMerge branch 'develop' of git://github.com/sagemath/sage into public/manifolds/typing
45c55d6Fix patchbot warnings

comment:28 Changed 14 months ago by git

  • Commit changed from 45c55d6a05ec9305751db34bbb4ce24a3d04c2fc to 4827cd2c643327a228fbf5f19c9d097e379ba5b2

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

4827cd2Fix import problem

comment:29 Changed 14 months ago by gh-tobiasdiez

  • Status changed from needs_work to needs_review

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.

comment:30 Changed 13 months ago by chapoton

  • Status changed from needs_review to needs_work

red branch => needs work

comment:31 Changed 13 months ago by git

  • Commit changed from 4827cd2c643327a228fbf5f19c9d097e379ba5b2 to c9bf8620892a7ed243a6f4bebc0dafa801f1950f

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

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

comment:32 Changed 13 months ago by gh-tobiasdiez

  • Status changed from needs_work to needs_review

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

comment:33 Changed 12 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:34 Changed 12 months ago by chapoton

red branch => needs work

comment:35 Changed 12 months ago by chapoton

  • Status changed from needs_review to needs_work

comment:36 Changed 11 months ago by git

  • Commit changed from c9bf8620892a7ed243a6f4bebc0dafa801f1950f to 5db7ddfe2123b2bbf8500736548921ed3698ac35

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

5db7ddfMerge develop

comment:37 Changed 11 months ago by git

  • Commit changed from 5db7ddfe2123b2bbf8500736548921ed3698ac35 to cd9ef9e0cd24aaa748fdc470268f6ce8dc69aefe

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

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

comment:38 Changed 11 months ago by gh-tobiasdiez

  • Status changed from needs_work to needs_review

Merged upstream.

comment:39 follow-up: Changed 11 months ago by 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)

comment:40 in reply to: ↑ 39 Changed 11 months ago by 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 ?

comment:41 Changed 11 months ago by 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.

comment:42 follow-up: Changed 11 months ago by 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?

comment:43 in reply to: ↑ 42 Changed 11 months ago by 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.

comment:44 Changed 11 months ago by mkoeppe

  • Status changed from needs_review to needs_work

comment:45 Changed 11 months ago by git

  • Commit changed from cd9ef9e0cd24aaa748fdc470268f6ce8dc69aefe to 0c937af86321a0814134ee7181def881e3804ec2

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

db5a3fcReadd documentation
0c937afRemove none for metric

comment:46 Changed 11 months ago by gh-tobiasdiez

  • Description modified (diff)
  • Status changed from needs_work to needs_review

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.

comment:47 Changed 10 months ago by chapoton

  • Status changed from needs_review to needs_work

red branch

comment:48 Changed 10 months ago by git

  • Commit changed from 0c937af86321a0814134ee7181def881e3804ec2 to 06770cdb3dafa5f0833b96580675e2bf847d5ca3

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

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

comment:49 Changed 10 months ago by gh-tobiasdiez

  • Status changed from needs_work to needs_review

Merged develop branch.

comment:50 Changed 7 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.4

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

comment:51 Changed 6 months ago by gh-tobiasdiez

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

comment:52 Changed 6 months ago by egourgoulhon

The patchbot reports many doctest errors.

comment:53 Changed 6 months ago by mkoeppe

  • Status changed from needs_review to needs_work

comment:54 Changed 5 months ago by mkoeppe

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

comment:55 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.