Opened 3 years ago
Closed 8 months ago
#29775 closed enhancement (fixed)
Add a bit of typing to manifold code
Reported by:  Tobias Diez  Owned by:  

Priority:  minor  Milestone:  sage9.6 
Component:  manifolds  Keywords:  typing 
Cc:  Travis Scrimshaw, Nicolas M. Thiéry, Michael Jung, Frédéric 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: 
Description (last modified by )
This PR adds a bit of typing information to some of the methods in the manifolds module.
Change History (137)
comment:1 Changed 3 years ago by
comment:2 Changed 3 years ago by
Status:  new → needs_review 

comment:3 followups: 5 6 Changed 3 years 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 3 years ago by
Commit:  e620d059f0f89f840fa2b0091477e7a7ddaa6630 → 23b6012d4b5a2f9dbd1b8a07f454ec29191bf0a3 

Branch pushed to git repo; I updated commit sha1. New commits:
23b6012  Add more typing

comment:5 Changed 3 years ago by
Cc:  Travis Scrimshaw Nicolas M. Thiéry 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 Changed 3 years 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 3 years ago by
Description:  modified (diff) 

Summary:  Add a bit of typing to manifolds package → Add a bit of typing to manifold code 
comment:8 followup: 9 Changed 3 years 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 Changed 3 years 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 3 years ago by
Commit:  23b6012d4b5a2f9dbd1b8a07f454ec29191bf0a3 → 3f93d314779566116609cf208ba3651fbdadca03 

comment:11 Changed 3 years 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 3 years 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 3 years ago by
Another thing: the typing information does not show up in the html documentation produced by Sphinx.
comment:14 Changed 3 years 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 3 years 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 2 years ago by
Cc:  Michael Jung added 

comment:17 Changed 2 years 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 Changed 2 years ago by
Cc:  Frédéric 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 2 years ago by
Status:  needs_review → needs_work 

branch is red and needs to be rebased on the latest beta
comment:20 Changed 2 years ago by
Commit:  3f93d314779566116609cf208ba3651fbdadca03 → 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 2 years ago by
Commit:  f348b54a4ddb2d0bf45836af3b12ab2b460558d0 → 3a4e0ebeb1ae217cae58fbd8c8e645eed6e1e7b0 

Branch pushed to git repo; I updated commit sha1. New commits:
3a4e0eb  Remove pyright config

comment:22 Changed 2 years 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 2 years ago by
Status:  needs_work → needs_review 

comment:24 Changed 2 years ago by
Status:  needs_review → 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 2 years 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 2 years 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 2 years ago by
Commit:  3a4e0ebeb1ae217cae58fbd8c8e645eed6e1e7b0 → 45c55d6a05ec9305751db34bbb4ce24a3d04c2fc 

comment:28 Changed 2 years ago by
Commit:  45c55d6a05ec9305751db34bbb4ce24a3d04c2fc → 4827cd2c643327a228fbf5f19c9d097e379ba5b2 

Branch pushed to git repo; I updated commit sha1. New commits:
4827cd2  Fix import problem

comment:29 Changed 2 years ago by
Status:  needs_work → 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:31 Changed 2 years ago by
Commit:  4827cd2c643327a228fbf5f19c9d097e379ba5b2 → 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 2 years ago by
Status:  needs_work → needs_review 

Thanks. I've now merged upstream into this branch.
comment:33 Changed 2 years ago by
Milestone:  sage9.2 → sage9.3 

comment:35 Changed 2 years ago by
Status:  needs_review → needs_work 

comment:36 Changed 2 years ago by
Commit:  c9bf8620892a7ed243a6f4bebc0dafa801f1950f → 5db7ddfe2123b2bbf8500736548921ed3698ac35 

Branch pushed to git repo; I updated commit sha1. New commits:
5db7ddf  Merge develop

comment:37 Changed 2 years ago by
Commit:  5db7ddfe2123b2bbf8500736548921ed3698ac35 → 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 2 years 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 Changed 2 years 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 2 years 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 2 years 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 Changed 2 years 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 2 years ago by
Status:  needs_review → needs_work 

comment:45 Changed 2 years ago by
Commit:  cd9ef9e0cd24aaa748fdc470268f6ce8dc69aefe → 0c937af86321a0814134ee7181def881e3804ec2 

comment:46 Changed 2 years ago by
Description:  modified (diff) 

Status:  needs_work → 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 2 years ago by
Commit:  0c937af86321a0814134ee7181def881e3804ec2 → 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:50 Changed 21 months ago by
Milestone:  sage9.3 → sage9.4 

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.
comment:53 Changed 20 months ago by
Status:  needs_review → needs_work 

comment:54 Changed 19 months ago by
Also, now that we can use from __future__ import annotations
, I think some of the type annotations can be simplified
comment:55 Changed 17 months ago by
Milestone:  sage9.4 → sage9.5 

Setting a new milestone for this ticket based on a cursory review.
comment:56 Changed 12 months ago by
Status:  needs_work → 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 12 months ago by
Commit:  06770cdb3dafa5f0833b96580675e2bf847d5ca3 → fc15d33011d3b64a937f2e3b3f54610d66707741 

comment:58 Changed 12 months ago by
Commit:  fc15d33011d3b64a937f2e3b3f54610d66707741 → 3f16aee5f3d1ba819e7506deb4dc647cea8f1a86 

Branch pushed to git repo; I updated commit sha1. New commits:
3f16aee  Fix tests

comment:59 Changed 12 months ago by
Milestone:  sage9.5 → sage9.6 

Stalled in needs_review
or needs_info
; likely won't make it into Sage 9.5.
comment:61 Changed 11 months ago by
Status:  needs_work → 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 11 months ago by
Commit:  3f16aee5f3d1ba819e7506deb4dc647cea8f1a86 → 69c09434fd99d865c4740f864844e52778042267 

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

comment:63 Changed 10 months ago by
Component:  geometry → manifolds 

comment:65 Changed 10 months ago by
Status:  needs_work → 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 9 months ago by
Commit:  69c09434fd99d865c4740f864844e52778042267 → 229fde199d98d0eb71102ff413c827277b2c2c54 

Branch pushed to git repo; I updated commit sha1. New commits:
229fde1  Merge remotetracking branch 'origin/develop' into public/manifolds/typing

comment:67 followup: 71 Changed 9 months ago by
 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 9 months ago by
Status:  needs_review → needs_work 

comment:69 Changed 9 months ago by
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 9 months ago by
Commit:  229fde199d98d0eb71102ff413c827277b2c2c54 → 4290c246b93db15f99eb683fcbb6b44de326813d 

Branch pushed to git repo; I updated commit sha1. New commits:
4290c24  Fix style

comment:71 Changed 9 months ago by
Status:  needs_work → 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 followup: 75 Changed 9 months ago by
Status:  needs_review → needs_work 

You didn't address comment:69
comment:73 followup: 74 Changed 9 months ago by
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 followup: 76 Changed 9 months ago by
Replying to mkoeppe:
And this change is an API change (= introduces a bug).
The argument
tensor_type
does NOT haveTuple[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)
withk
being the contravariant rank
and
l
the covariant rank
which is quite clear about that it expects a 2tuple. 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 Changed 9 months ago by
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 Changed 9 months ago by
Replying to ghtobiasdiez:
Replying to mkoeppe:
And this change is an API change (= introduces a bug).
The argument
tensor_type
does NOT haveTuple[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)
withk
being the contravariant rankand
l
the covariant rankwhich is quite clear about that it expects a 2tuple.
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.
comment:77 Changed 9 months ago by
Commit:  4290c246b93db15f99eb683fcbb6b44de326813d → b7d121c6a5c178273069a0fc0b638e41aea86c65 

comment:78 Changed 9 months ago by
Status:  needs_work → needs_review 

comment:79 Changed 9 months ago by
I think these changes are not good. They are obscuring the internal data representation
@@ 847,11 +847,11 @@ class TopologicalSubmanifold(TopologicalManifold): 3dimensional 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): 3dimensional 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 9 months ago by
I'm personally in favor of these changes (the 80 character limit is a bit inflexible when module names or identifiers are already 40ish 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 9 months ago by
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 followup: 83 Changed 9 months ago by
It is an invariant of the internal representation (data structure) that _immersed or _embedded
implies that _immersion
is set.
comment:83 Changed 9 months ago by
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 followup: 85 Changed 9 months ago by
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
comment:85 Changed 9 months ago by
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 9 months ago by
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 9 months ago by
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:89 Changed 9 months ago by
Commit:  b7d121c6a5c178273069a0fc0b638e41aea86c65 → 3dd340d3c6e6c80ff1e5e576632aae61dff6b228 

Branch pushed to git repo; I updated commit sha1. New commits:
3dd340d  Use assert

comment:90 Changed 9 months ago by
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
.
comment:91 Changed 9 months ago by
Just tried it on gitpod: merge: 9.6.beta5  not something we can merge
comment:93 Changed 9 months ago by
Commit:  3dd340d3c6e6c80ff1e5e576632aae61dff6b228 → 0a521654d98ce20d8d5a24cb8f869a71f06a4767 

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

comment:94 followups: 96 97 104 Changed 9 months ago by
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 followup: 98 Changed 9 months ago by
I think time would be well spent on some git practice...
comment:96 Changed 9 months ago by
Status:  needs_review → 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 Changed 9 months ago by
Replying to ghtobiasdiez:
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 followup: 101 Changed 9 months ago by
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 9 months ago by
There's no need for a new ticket because the change is not an improvement.
comment:100 Changed 9 months ago by
Commit:  0a521654d98ce20d8d5a24cb8f869a71f06a4767 → c3239f7e07180b53e3958b8e6e99976f91b46ad3 

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

comment:102 Changed 9 months ago by
Status:  needs_work → needs_review 

comment:103 Changed 9 months ago by
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 Changed 9 months ago by
Replying to ghtobiasdiez:
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 9 months ago by
Reviewers:  → Matthias Koeppe 

comment:106 Changed 9 months ago by
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 9 months ago by
I've also successfully tested ./sage tp long src/sage/manifolds/
with python 3.7.
comment:108 Changed 9 months ago by
Status:  needs_review → positive_review 

Thanks for this contribution.
comment:110 Changed 9 months ago by
Status:  positive_review → 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:112 Changed 9 months ago by
Commit:  c3239f7e07180b53e3958b8e6e99976f91b46ad3 → 10bdad975caba590cd96cbbad73f3473346435a5 

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

comment:113 Changed 9 months ago by
Description:  modified (diff) 

Reviewers:  Matthias Koeppe → Matthias Koeppe, ... 
Status:  needs_info → needs_review 
comment:116 Changed 9 months ago by
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:118 Changed 9 months ago by
I believe this the currently preferred solution, at least until https://peps.python.org/pep0649/ hits. I don't think we need a Sagespecific policy on this.
comment:119 Changed 9 months ago by
Commit:  10bdad975caba590cd96cbbad73f3473346435a5 → a3905b5e1000496963225f17031a910cc7ffef17 

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'

comment:120 Changed 9 months ago by
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 9 months ago by
Commit:  a3905b5e1000496963225f17031a910cc7ffef17 → b2a68dc74673077d9d40ff287c7f01400a2b1985 

Branch pushed to git repo; I updated commit sha1. New commits:
b2a68dc  src/sage/manifolds/differentiable/manifold.py: Conditionalize imports with 'if TYPE_CHECKING:'

comment:122 Changed 9 months ago by
Commit:  b2a68dc74673077d9d40ff287c7f01400a2b1985 → 2061a5033457dd1ffd6f4aad3b5f4bbde3ace65f 

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

comment:123 Changed 9 months ago by
Commit:  2061a5033457dd1ffd6f4aad3b5f4bbde3ace65f → 056a69dbffe57c7b866cfc622556645c82b2afc5 

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

comment:124 Changed 9 months ago by
There's also one file that still manually stringquotes types instead of relying on from __future__ import annotations
.
comment:125 Changed 9 months ago by
Commit:  056a69dbffe57c7b866cfc622556645c82b2afc5 → f813f3c8e852a1ca33f9ccbc61eb20d4fbc3a249 

Branch pushed to git repo; I updated commit sha1. New commits:
f813f3c  src/sage/manifolds/differentiable/tensorfield.py: Don't stringquote types

comment:126 Changed 9 months ago by
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:128 Changed 9 months ago by
Reviewers:  Matthias Koeppe, ... → Matthias Koeppe, Tobias Diez 

Status:  needs_review → 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 9 months ago by
Status:  positive_review → needs_work 

There is a merge failure with Sage 9.6.beta6
comment:130 Changed 9 months ago by
Commit:  f813f3c8e852a1ca33f9ccbc61eb20d4fbc3a249 → 7c18c7f8cd8c123d850e1aeaae25d5e6e37e5cec 

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

comment:132 followup: 135 Changed 8 months ago by
Status:  needs_review → 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:134 Changed 8 months ago by
Reviewers:  Matthias Koeppe, Tobias Diez → Matthias Koeppe, Tobias Diez, Eric Gourgoulhon 

comment:135 followup: 136 Changed 8 months ago by
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'scoverage
plugin than about the present code.
Might be worth opening a ticket at
comment:136 Changed 8 months ago by
Replying to slelievre:
Might be worth opening a ticket at
Thanks for the suggestion; this is now https://github.com/sagemath/sagepatchbot/issues/161
comment:137 Changed 8 months ago by
Branch:  public/manifolds/typing → 7c18c7f8cd8c123d850e1aeaae25d5e6e37e5cec 

Resolution:  → fixed 
Status:  positive_review → closed 
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