Opened 2 years ago

Closed 7 weeks ago

#29775 closed enhancement (fixed)

Add a bit of typing to manifold code

Reported by: gh-tobiasdiez Owned by:
Priority: minor Milestone: sage-9.6
Component: manifolds Keywords: typing
Cc: tscrim, nthiery, gh-mjungmath, chapoton Merged in:
Authors: Tobias Diez Reviewers: Matthias Koeppe, Tobias Diez, Eric Gourgoulhon
Report Upstream: N/A Work issues:
Branch: 7c18c7f (Commits, GitHub, GitLab) Commit: 7c18c7f8cd8c123d850e1aeaae25d5e6e37e5cec
Dependencies: Stopgaps:

Status badges

Description (last modified by mkoeppe)

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

Change History (137)

comment:1 Changed 2 years 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 2 years ago by gh-tobiasdiez

  • Status changed from new to needs_review

comment:3 follow-ups: Changed 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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 2 years ago by egourgoulhon

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

comment:14 Changed 2 years 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 2 years 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 2 years ago by gh-tobiasdiez (previous) (diff)

comment:16 Changed 23 months ago by gh-mjungmath

  • Cc gh-mjungmath added

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

  • Status changed from needs_work to needs_review

comment:24 Changed 21 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 21 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 21 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 21 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 21 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 21 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 20 months ago by chapoton

  • Status changed from needs_review to needs_work

red branch => needs work

comment:31 Changed 20 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 20 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 19 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:34 Changed 19 months ago by chapoton

red branch => needs work

comment:35 Changed 19 months ago by chapoton

  • Status changed from needs_review to needs_work

comment:36 Changed 19 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 19 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 19 months ago by gh-tobiasdiez

  • Status changed from needs_work to needs_review

Merged upstream.

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

  • Status changed from needs_review to needs_work

comment:45 Changed 18 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 18 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 17 months ago by chapoton

  • Status changed from needs_review to needs_work

red branch

comment:48 Changed 17 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 17 months ago by gh-tobiasdiez

  • Status changed from needs_work to needs_review

Merged develop branch.

comment:50 Changed 14 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 13 months ago by gh-tobiasdiez

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

comment:52 Changed 13 months ago by egourgoulhon

The patchbot reports many doctest errors.

comment:53 Changed 13 months ago by mkoeppe

  • Status changed from needs_review to needs_work

comment:54 Changed 12 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 10 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.

comment:56 Changed 6 months ago by gh-tobiasdiez

  • Status changed from needs_work to needs_review

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.

comment:57 Changed 6 months ago by git

  • Commit changed from 06770cdb3dafa5f0833b96580675e2bf847d5ca3 to fc15d33011d3b64a937f2e3b3f54610d66707741

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

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

comment:58 Changed 6 months ago by git

  • Commit changed from fc15d33011d3b64a937f2e3b3f54610d66707741 to 3f16aee5f3d1ba819e7506deb4dc647cea8f1a86

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

3f16aeeFix tests

comment:59 Changed 5 months ago by mkoeppe

  • Milestone changed from sage-9.5 to sage-9.6

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

comment:60 Changed 5 months ago by chapoton

  • Status changed from needs_review to needs_work

red branch => needs work

comment:61 Changed 5 months ago by gh-tobiasdiez

  • Status changed from needs_work to needs_review

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.

comment:62 Changed 5 months ago by git

  • Commit changed from 3f16aee5f3d1ba819e7506deb4dc647cea8f1a86 to 69c09434fd99d865c4740f864844e52778042267

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

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

comment:63 Changed 4 months ago by gh-tobiasdiez

  • Component changed from geometry to manifolds

comment:64 Changed 3 months ago by mkoeppe

  • Status changed from needs_review to needs_work

red branch

comment:65 Changed 3 months ago by gh-tobiasdiez

  • Status changed from needs_work to needs_review

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

comment:66 Changed 3 months ago by git

  • Commit changed from 69c09434fd99d865c4740f864844e52778042267 to 229fde199d98d0eb71102ff413c827277b2c2c54

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

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

comment:67 follow-up: Changed 3 months ago by mkoeppe

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

comment:68 Changed 3 months ago by mkoeppe

  • Status changed from needs_review to needs_work

comment:69 Changed 3 months ago by mkoeppe

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"

comment:70 Changed 3 months ago by git

  • Commit changed from 229fde199d98d0eb71102ff413c827277b2c2c54 to 4290c246b93db15f99eb683fcbb6b44de326813d

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

4290c24Fix style

comment:71 in reply to: ↑ 67 Changed 3 months ago by gh-tobiasdiez

  • Status changed from needs_work to needs_review

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.

comment:72 follow-up: Changed 3 months ago by mkoeppe

  • Status changed from needs_review to needs_work

You didn't address comment:69

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

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__()

comment:74 in reply to: ↑ 73 ; follow-up: Changed 3 months ago by 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. 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.

comment:75 in reply to: ↑ 72 Changed 3 months ago by gh-tobiasdiez

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.

comment:76 in reply to: ↑ 74 Changed 3 months ago by mkoeppe

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.

Last edited 3 months ago by mkoeppe (previous) (diff)

comment:77 Changed 2 months ago by git

  • Commit changed from 4290c246b93db15f99eb683fcbb6b44de326813d to b7d121c6a5c178273069a0fc0b638e41aea86c65

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

8e911a4Readd tuple
b7d121cMerge remote-tracking branch 'origin/develop' into public/manifolds/typing

comment:78 Changed 2 months ago by gh-tobiasdiez

  • Status changed from needs_work to needs_review

comment:79 Changed 2 months ago by mkoeppe

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
 

comment:80 Changed 2 months ago by mkoeppe

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:

comment:81 Changed 2 months ago by gh-tobiasdiez

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?

comment:82 follow-up: Changed 2 months ago by mkoeppe

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

comment:83 in reply to: ↑ 82 Changed 2 months ago by gh-tobiasdiez

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.

comment:84 follow-up: Changed 2 months ago by 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

Last edited 2 months ago by mkoeppe (previous) (diff)

comment:85 in reply to: ↑ 84 Changed 2 months ago by gh-tobiasdiez

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.

comment:86 Changed 2 months ago by mkoeppe

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

comment:87 Changed 2 months ago by gh-tobiasdiez

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.

comment:88 Changed 2 months ago by mkoeppe

Try assert if you haven't yet.

comment:89 Changed 2 months ago by git

  • Commit changed from b7d121c6a5c178273069a0fc0b638e41aea86c65 to 3dd340d3c6e6c80ff1e5e576632aae61dff6b228

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

3dd340dUse assert

comment:90 Changed 2 months ago by mkoeppe

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.

Last edited 2 months ago by mkoeppe (previous) (diff)

comment:91 Changed 2 months ago by gh-tobiasdiez

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

comment:92 Changed 2 months ago by mkoeppe

Fetch it first: git fetch trac 9.6.beta5

comment:93 Changed 2 months ago by git

  • Commit changed from 3dd340d3c6e6c80ff1e5e576632aae61dff6b228 to 0a521654d98ce20d8d5a24cb8f869a71f06a4767

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

0a52165Add a bit of typing to manifold code

comment:94 follow-ups: Changed 2 months ago by gh-tobiasdiez

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.

comment:95 follow-up: Changed 2 months ago by mkoeppe

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

comment:96 in reply to: ↑ 94 Changed 2 months ago by mkoeppe

  • Status changed from needs_review to needs_work

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.

comment:97 in reply to: ↑ 94 Changed 2 months ago by mkoeppe

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.

comment:98 in reply to: ↑ 95 ; follow-up: Changed 2 months ago by gh-tobiasdiez

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.

comment:99 Changed 2 months ago by mkoeppe

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

comment:100 Changed 2 months ago by git

  • Commit changed from 0a521654d98ce20d8d5a24cb8f869a71f06a4767 to c3239f7e07180b53e3958b8e6e99976f91b46ad3

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

c3239f7Add a bit of typing to manifold code

comment:101 in reply to: ↑ 98 Changed 2 months ago by mkoeppe

Replying to gh-tobiasdiez:

please be my guest.

here we go

comment:102 Changed 2 months ago by mkoeppe

  • Status changed from needs_work to needs_review

comment:103 Changed 2 months ago by mkoeppe

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

comment:104 in reply to: ↑ 94 Changed 2 months ago by mkoeppe

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.

comment:105 Changed 2 months ago by mkoeppe

  • Reviewers set to Matthias Koeppe

comment:106 Changed 2 months ago by mkoeppe

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.

comment:107 Changed 2 months ago by mkoeppe

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

comment:108 Changed 2 months ago by mkoeppe

  • Status changed from needs_review to positive_review

Thanks for this contribution.

comment:109 Changed 2 months ago by gh-tobiasdiez

Thanks

comment:110 Changed 2 months ago by gh-mjungmath

  • Status changed from positive_review to needs_info

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.

comment:111 Changed 2 months ago by gh-mjungmath

Here's the corresponding follow-up: #33542

comment:112 Changed 2 months ago by git

  • Commit changed from c3239f7e07180b53e3958b8e6e99976f91b46ad3 to 10bdad975caba590cd96cbbad73f3473346435a5

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

10bdad9Add a bit of typing to manifold code

comment:113 Changed 2 months ago by mkoeppe

  • Description modified (diff)
  • Reviewers changed from Matthias Koeppe to Matthias Koeppe, ...
  • Status changed from needs_info to needs_review

comment:114 Changed 2 months ago by mkoeppe

I've reduced the scope of the ticket as requested

comment:115 Changed 2 months ago by mkoeppe

The documentation changes are now on #33542.

comment:116 Changed 2 months ago by egourgoulhon

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?

comment:117 follow-up: Changed 2 months ago by mkoeppe

We can move them into if TYPE_CHECKING: blocks.

comment:118 Changed 2 months ago by mkoeppe

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.

comment:119 Changed 2 months ago by git

  • Commit changed from 10bdad975caba590cd96cbbad73f3473346435a5 to a3905b5e1000496963225f17031a910cc7ffef17

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

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

comment:120 in reply to: ↑ 117 Changed 2 months ago by egourgoulhon

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.

comment:121 Changed 2 months ago by git

  • Commit changed from a3905b5e1000496963225f17031a910cc7ffef17 to b2a68dc74673077d9d40ff287c7f01400a2b1985

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

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

comment:122 Changed 2 months ago by git

  • Commit changed from b2a68dc74673077d9d40ff287c7f01400a2b1985 to 2061a5033457dd1ffd6f4aad3b5f4bbde3ace65f

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

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

comment:123 Changed 2 months ago by git

  • Commit changed from 2061a5033457dd1ffd6f4aad3b5f4bbde3ace65f to 056a69dbffe57c7b866cfc622556645c82b2afc5

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

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

comment:124 Changed 2 months ago by mkoeppe

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

comment:125 Changed 2 months ago by git

  • Commit changed from 056a69dbffe57c7b866cfc622556645c82b2afc5 to f813f3c8e852a1ca33f9ccbc61eb20d4fbc3a249

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

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

comment:126 Changed 2 months ago by mkoeppe

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.

comment:127 Changed 2 months ago by mkoeppe

Tests still OK with Python 3.7

comment:128 Changed 8 weeks ago by gh-tobiasdiez

  • Reviewers changed from Matthias Koeppe, ... to Matthias Koeppe, Tobias Diez
  • Status changed from needs_review to positive_review

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.

comment:129 Changed 8 weeks ago by egourgoulhon

  • Status changed from positive_review to needs_work

There is a merge failure with Sage 9.6.beta6

comment:130 Changed 8 weeks ago by git

  • Commit changed from f813f3c8e852a1ca33f9ccbc61eb20d4fbc3a249 to 7c18c7f8cd8c123d850e1aeaae25d5e6e37e5cec

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

51ee92bAdd a bit of typing to manifold code
974ec0asrc/sage/tensor/modules/free_module_tensor.py: Conditionalize an import with 'if TYPE_CHECKING:', add 'from __future__ import annotations'
9905adesrc/sage/manifolds/differentiable/manifold.py: Conditionalize imports with 'if TYPE_CHECKING:'
ba751f1src/sage/manifolds/differentiable/degenerate.py: Conditionalize an import with 'if TYPE_CHECKING:'
3beb020src/sage/manifolds/differentiable/degenerate_submanifold.py: Conditionalize an import with 'if TYPE_CHECKING:'
7c18c7fsrc/sage/manifolds/differentiable/tensorfield.py: Don't string-quote types

comment:131 Changed 8 weeks ago by mkoeppe

  • Status changed from needs_work to needs_review

rebased

comment:132 follow-up: Changed 8 weeks ago by egourgoulhon

  • Status changed from needs_review to positive_review

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.

comment:133 Changed 8 weeks ago by gh-tobiasdiez

Thanks!

comment:134 Changed 8 weeks ago by mkoeppe

  • Reviewers changed from Matthias Koeppe, Tobias Diez to Matthias Koeppe, Tobias Diez, Eric Gourgoulhon

comment:135 in reply to: ↑ 132 ; follow-up: Changed 8 weeks ago by slelievre

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

comment:136 in reply to: ↑ 135 Changed 7 weeks ago by egourgoulhon

Replying to slelievre:

Might be worth opening a ticket at

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

comment:137 Changed 7 weeks ago by vbraun

  • Branch changed from public/manifolds/typing to 7c18c7f8cd8c123d850e1aeaae25d5e6e37e5cec
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.