Opened 12 months ago
Last modified 3 weeks ago
#29775 needs_work enhancement
Add a bit of typing to manifold code
Reported by:  ghtobiasdiez  Owned by:  

Priority:  minor  Milestone:  sage9.4 
Component:  geometry  Keywords:  typing 
Cc:  tscrim, nthiery, ghmjungmath, chapoton  Merged in:  
Authors:  Tobias Diez  Reviewers:  
Report Upstream:  N/A  Work issues:  
Branch:  public/manifolds/typing (Commits, GitHub, GitLab)  Commit:  06770cdb3dafa5f0833b96580675e2bf847d5ca3 
Dependencies:  Stopgaps: 
Description (last modified by )
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 (53)
comment:1 Changed 12 months ago by
comment:2 Changed 12 months ago by
 Status changed from new to needs_review
comment:3 followups: ↓ 5 ↓ 6 Changed 12 months ago by
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 12 months ago by
 Commit changed from e620d059f0f89f840fa2b0091477e7a7ddaa6630 to 23b6012d4b5a2f9dbd1b8a07f454ec29191bf0a3
Branch pushed to git repo; I updated commit sha1. New commits:
23b6012  Add more typing

comment:5 in reply to: ↑ 3 Changed 12 months ago by
 Cc tscrim nthiery added
Replying to egourgoulhon:
The addition of
identity_map
toDifferentiableManifold
is code duplication (the code is identical toidentity_map
provided by the mother classTopologicalManifold
). 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 12 months ago by
Replying to egourgoulhon:
The addition of
identity_map
toDifferentiableManifold
is code duplication (the code is identical toidentity_map
provided by the mother classTopologicalManifold
). 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 12 months ago by
 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 followup: ↓ 9 Changed 12 months ago by
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#advancedusesofselftypes).
comment:9 in reply to: ↑ 8 Changed 12 months ago by
Replying to ghtobiasdiez:
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 aContinuousMap
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 classesB(A)
as well (sob.hom_set
returns the hom set ofb
as an element of B).
No need for improvement, this is already the case:
sage: M = Manifold(2, 'M'); M 2dimensional 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 2dimensional differentiable manifold M to 2dimensional differentiable manifold M in Category of smooth manifolds over Real Field with 53 bits of precision
comment:10 Changed 11 months ago by
 Commit changed from 23b6012d4b5a2f9dbd1b8a07f454ec29191bf0a3 to 3f93d314779566116609cf208ba3651fbdadca03
comment:11 Changed 11 months ago by
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 followup issues to keep the size of the changes small and reviewable.
comment:12 Changed 11 months ago by
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 11 months ago by
Another thing: the typing information does not show up in the html documentation produced by Sphinx.
comment:14 Changed 11 months ago by
The file pyrightconfig.json
at the root of the Sage directory has probably been added to the commit by mistake.
comment:15 followups: ↓ 17 ↓ 18 Changed 11 months ago by
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/vimsyntastic/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/vimsyntastic/syntastic/issues/1667#issuecomment172538109). 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.
comment:16 Changed 10 months ago by
 Cc ghmjungmath added
comment:17 in reply to: ↑ 15 Changed 10 months ago by
Replying to ghtobiasdiez:
Finally, the
pyrightconfig.json
is the configuration file forpyright
, 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 10 months ago by
 Cc chapoton added
Replying to ghtobiasdiez:
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/sagepatchbot
comment:19 Changed 9 months ago by
 Status changed from needs_review to needs_work
branch is red and needs to be rebased on the latest beta
comment:20 Changed 9 months ago by
 Commit changed from 3f93d314779566116609cf208ba3651fbdadca03 to f348b54a4ddb2d0bf45836af3b12ab2b460558d0
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

comment:21 Changed 9 months ago by
 Commit changed from f348b54a4ddb2d0bf45836af3b12ab2b460558d0 to 3a4e0ebeb1ae217cae58fbd8c8e645eed6e1e7b0
Branch pushed to git repo; I updated commit sha1. New commits:
3a4e0eb  Remove pyright config

comment:22 Changed 9 months ago by
@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/sagepatchbot/issues/143. Can I disable pyflakes for the lines that are wrongly reported?
comment:23 Changed 9 months ago by
 Status changed from needs_work to needs_review
comment:24 Changed 9 months ago by
 Status changed from needs_review to needs_work
sage t long warnlong 63.8 randomseed=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/patchbotsage/local/lib/python3.7/sitepackages/sage/doctest/forker.py", line 715, in _run self.compile_and_execute(example, compiler, test.globs) File "/srv/public/patchbotsage/local/lib/python3.7/sitepackages/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/patchbotsage/local/lib/python3.7/sitepackages/sage/doctest/forker.py", line 715, in _run self.compile_and_execute(example, compiler, test.globs) File "/srv/public/patchbotsage/local/lib/python3.7/sitepackages/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 9 months ago by
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 9 months ago by
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 9 months ago by
 Commit changed from 3a4e0ebeb1ae217cae58fbd8c8e645eed6e1e7b0 to 45c55d6a05ec9305751db34bbb4ce24a3d04c2fc
comment:28 Changed 9 months ago by
 Commit changed from 45c55d6a05ec9305751db34bbb4ce24a3d04c2fc to 4827cd2c643327a228fbf5f19c9d097e379ba5b2
Branch pushed to git repo; I updated commit sha1. New commits:
4827cd2  Fix import problem

comment:29 Changed 9 months ago by
 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 @ghkliem 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 8 months ago by
 Status changed from needs_review to needs_work
red branch => needs work
comment:31 Changed 8 months ago by
 Commit changed from 4827cd2c643327a228fbf5f19c9d097e379ba5b2 to c9bf8620892a7ed243a6f4bebc0dafa801f1950f
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

comment:32 Changed 8 months ago by
 Status changed from needs_work to needs_review
Thanks. I've now merged upstream into this branch.
comment:33 Changed 7 months ago by
 Milestone changed from sage9.2 to sage9.3
comment:34 Changed 6 months ago by
red branch => needs work
comment:35 Changed 6 months ago by
 Status changed from needs_review to needs_work
comment:36 Changed 6 months ago by
 Commit changed from c9bf8620892a7ed243a6f4bebc0dafa801f1950f to 5db7ddfe2123b2bbf8500736548921ed3698ac35
Branch pushed to git repo; I updated commit sha1. New commits:
5db7ddf  Merge develop

comment:37 Changed 6 months ago by
 Commit changed from 5db7ddfe2123b2bbf8500736548921ed3698ac35 to cd9ef9e0cd24aaa748fdc470268f6ce8dc69aefe
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

comment:39 followup: ↓ 40 Changed 6 months ago by
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 6 months ago by
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 mettricDid 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 6 months ago by
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 followup: ↓ 43 Changed 6 months ago by
I'm currently working on a solution to show the typing information in the documentation using https://pypi.org/project/sphinxautodoctypehints/ (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 6 months ago by
Replying to ghtobiasdiez:
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 (LeviCivita 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 6 months ago by
 Status changed from needs_review to needs_work
comment:45 Changed 6 months ago by
 Commit changed from cd9ef9e0cd24aaa748fdc470268f6ce8dc69aefe to 0c937af86321a0814134ee7181def881e3804ec2
comment:46 Changed 6 months ago by
 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:48 Changed 5 months ago by
 Commit changed from 0c937af86321a0814134ee7181def881e3804ec2 to 06770cdb3dafa5f0833b96580675e2bf847d5ca3
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

comment:49 Changed 5 months ago by
 Status changed from needs_work to needs_review
Merged develop branch.
comment:50 Changed 2 months ago by
 Milestone changed from sage9.3 to sage9.4
Setting new milestone based on a cursory review of ticket status, priority, and last modification date.
comment:51 Changed 3 weeks ago by
It would be nice if this ticket could be reviewed. Thanks!
comment:52 Changed 3 weeks ago by
The patchbot reports many doctest errors.
comment:53 Changed 3 weeks ago by
 Status changed from needs_review to needs_work
In case people are interested in a general discussion of typing in the Sage library: #29756 Metaticket: Review of Python 3 features that sagelib should use systematically