Opened 2 years ago
Closed 7 weeks ago
#29775 closed enhancement (fixed)
Add a bit of typing to manifold code
Reported by:  ghtobiasdiez  Owned by:  

Priority:  minor  Milestone:  sage9.6 
Component:  manifolds  Keywords:  typing 
Cc:  tscrim, nthiery, ghmjungmath, 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 2 years ago by
comment:2 Changed 2 years ago by
 Status changed from new to needs_review
comment:3 followups: ↓ 5 ↓ 6 Changed 2 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 2 years 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 2 years 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 2 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 2 years 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 2 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 in reply to: ↑ 8 Changed 2 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 2 years ago by
 Commit changed from 23b6012d4b5a2f9dbd1b8a07f454ec29191bf0a3 to 3f93d314779566116609cf208ba3651fbdadca03
comment:11 Changed 2 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 2 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 2 years ago by
Another thing: the typing information does not show up in the html documentation produced by Sphinx.
comment:14 Changed 2 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 2 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 23 months ago by
 Cc ghmjungmath added
comment:17 in reply to: ↑ 15 Changed 22 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 22 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 22 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 22 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 22 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 22 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 22 months ago by
 Status changed from needs_work to needs_review
comment:24 Changed 21 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 21 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 21 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 21 months ago by
 Commit changed from 3a4e0ebeb1ae217cae58fbd8c8e645eed6e1e7b0 to 45c55d6a05ec9305751db34bbb4ce24a3d04c2fc
comment:28 Changed 21 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 21 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 20 months ago by
 Status changed from needs_review to needs_work
red branch => needs work
comment:31 Changed 20 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 20 months ago by
 Status changed from needs_work to needs_review
Thanks. I've now merged upstream into this branch.
comment:33 Changed 19 months ago by
 Milestone changed from sage9.2 to sage9.3
comment:34 Changed 19 months ago by
red branch => needs work
comment:35 Changed 19 months ago by
 Status changed from needs_review to needs_work
comment:36 Changed 19 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 19 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 19 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 19 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 19 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 19 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 19 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 19 months ago by
 Status changed from needs_review to needs_work
comment:45 Changed 18 months ago by
 Commit changed from cd9ef9e0cd24aaa748fdc470268f6ce8dc69aefe to 0c937af86321a0814134ee7181def881e3804ec2
comment:46 Changed 18 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 17 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 17 months ago by
 Status changed from needs_work to needs_review
Merged develop branch.
comment:50 Changed 14 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 13 months ago by
It would be nice if this ticket could be reviewed. Thanks!
comment:52 Changed 13 months ago by
The patchbot reports many doctest errors.
comment:53 Changed 13 months ago by
 Status changed from needs_review to needs_work
comment:54 Changed 12 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 10 months ago by
 Milestone changed from sage9.4 to sage9.5
Setting a new milestone for this ticket based on a cursory review.
comment:56 Changed 6 months ago by
 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
 Commit changed from 06770cdb3dafa5f0833b96580675e2bf847d5ca3 to fc15d33011d3b64a937f2e3b3f54610d66707741
comment:58 Changed 6 months ago by
 Commit changed from fc15d33011d3b64a937f2e3b3f54610d66707741 to 3f16aee5f3d1ba819e7506deb4dc647cea8f1a86
Branch pushed to git repo; I updated commit sha1. New commits:
3f16aee  Fix tests

comment:59 Changed 5 months ago by
 Milestone changed from sage9.5 to sage9.6
Stalled in needs_review
or needs_info
; likely won't make it into Sage 9.5.
comment:60 Changed 5 months ago by
 Status changed from needs_review to needs_work
red branch => needs work
comment:61 Changed 5 months ago by
 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
 Commit changed from 3f16aee5f3d1ba819e7506deb4dc647cea8f1a86 to 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 4 months ago by
 Component changed from geometry to manifolds
comment:65 Changed 3 months ago by
 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
 Commit changed from 69c09434fd99d865c4740f864844e52778042267 to 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 3 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 3 months ago by
 Status changed from needs_review to needs_work
comment:69 Changed 3 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 3 months ago by
 Commit changed from 229fde199d98d0eb71102ff413c827277b2c2c54 to 4290c246b93db15f99eb683fcbb6b44de326813d
Branch pushed to git repo; I updated commit sha1. New commits:
4290c24  Fix style

comment:71 in reply to: ↑ 67 Changed 3 months ago by
 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 followup: ↓ 75 Changed 3 months ago by
 Status changed from needs_review to needs_work
You didn't address comment:69
comment:73 followup: ↓ 74 Changed 3 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 in reply to: ↑ 73 ; followup: ↓ 76 Changed 3 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 in reply to: ↑ 72 Changed 3 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 in reply to: ↑ 74 Changed 3 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 2 months ago by
 Commit changed from 4290c246b93db15f99eb683fcbb6b44de326813d to b7d121c6a5c178273069a0fc0b638e41aea86c65
comment:78 Changed 2 months ago by
 Status changed from needs_work to needs_review
comment:79 Changed 2 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 2 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 2 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 2 months ago by
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
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 2 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 in reply to: ↑ 84 Changed 2 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 2 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 2 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:88 Changed 2 months ago by
Try assert
if you haven't yet.
comment:89 Changed 2 months ago by
 Commit changed from b7d121c6a5c178273069a0fc0b638e41aea86c65 to 3dd340d3c6e6c80ff1e5e576632aae61dff6b228
Branch pushed to git repo; I updated commit sha1. New commits:
3dd340d  Use assert

comment:90 Changed 2 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 2 months ago by
Just tried it on gitpod: merge: 9.6.beta5  not something we can merge
comment:92 Changed 2 months ago by
Fetch it first: git fetch trac 9.6.beta5
comment:93 Changed 2 months ago by
 Commit changed from 3dd340d3c6e6c80ff1e5e576632aae61dff6b228 to 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 2 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 2 months ago by
I think time would be well spent on some git practice...
comment:96 in reply to: ↑ 94 Changed 2 months ago by
 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
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 in reply to: ↑ 95 ; followup: ↓ 101 Changed 2 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 2 months ago by
There's no need for a new ticket because the change is not an improvement.
comment:100 Changed 2 months ago by
 Commit changed from 0a521654d98ce20d8d5a24cb8f869a71f06a4767 to 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:101 in reply to: ↑ 98 Changed 2 months ago by
comment:102 Changed 2 months ago by
 Status changed from needs_work to needs_review
comment:103 Changed 2 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 in reply to: ↑ 94 Changed 2 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 2 months ago by
 Reviewers set to Matthias Koeppe
comment:106 Changed 2 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 2 months ago by
I've also successfully tested ./sage tp long src/sage/manifolds/
with python 3.7.
comment:108 Changed 2 months ago by
 Status changed from needs_review to positive_review
Thanks for this contribution.
comment:109 Changed 2 months ago by
Thanks
comment:110 Changed 2 months ago by
 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
Here's the corresponding followup: #33542
comment:112 Changed 2 months ago by
 Commit changed from c3239f7e07180b53e3958b8e6e99976f91b46ad3 to 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 2 months ago by
 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
I've reduced the scope of the ticket as requested
comment:115 Changed 2 months ago by
The documentation changes are now on #33542.
comment:116 Changed 2 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:117 followup: ↓ 120 Changed 2 months ago by
We can move them into if TYPE_CHECKING:
blocks.
comment:118 Changed 2 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 2 months ago by
 Commit changed from 10bdad975caba590cd96cbbad73f3473346435a5 to 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 in reply to: ↑ 117 Changed 2 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 2 months ago by
 Commit changed from a3905b5e1000496963225f17031a910cc7ffef17 to 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 2 months ago by
 Commit changed from b2a68dc74673077d9d40ff287c7f01400a2b1985 to 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 2 months ago by
 Commit changed from 2061a5033457dd1ffd6f4aad3b5f4bbde3ace65f to 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 2 months ago by
There's also one file that still manually stringquotes types instead of relying on from __future__ import annotations
.
comment:125 Changed 2 months ago by
 Commit changed from 056a69dbffe57c7b866cfc622556645c82b2afc5 to 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 2 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:127 Changed 2 months ago by
Tests still OK with Python 3.7
comment:128 Changed 8 weeks ago by
 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
 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
 Commit changed from f813f3c8e852a1ca33f9ccbc61eb20d4fbc3a249 to 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 weeks ago by
 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
Thanks!
comment:134 Changed 8 weeks ago by
 Reviewers changed from Matthias Koeppe, Tobias Diez to Matthias Koeppe, Tobias Diez, Eric Gourgoulhon
comment:135 in reply to: ↑ 132 ; followup: ↓ 136 Changed 8 weeks 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 in reply to: ↑ 135 Changed 7 weeks 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 7 weeks ago by
 Branch changed from public/manifolds/typing to 7c18c7f8cd8c123d850e1aeaae25d5e6e37e5cec
 Resolution set to fixed
 Status changed from positive_review to 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