Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add more typing information to manifold package #31006

Closed
tobiasdiez opened this issue Dec 4, 2020 · 74 comments
Closed

Add more typing information to manifold package #31006

tobiasdiez opened this issue Dec 4, 2020 · 74 comments

Comments

@tobiasdiez
Copy link
Contributor

In extension of #29775, more typing information is added to the manifold package (and its dependencies).

Depends on #29775

CC: @tscrim @nthiery @mjungmath @egourgoulhon @fchapoton

Component: manifolds

Author: Tobias Diez

Branch/Commit: c403910

Reviewer: Matthias Koeppe, Tobias Diez, Michael Jung, Eric Gourgoulhon

Issue created by migration from https://trac.sagemath.org/ticket/31006

@tobiasdiez tobiasdiez added this to the sage-9.3 milestone Dec 4, 2020
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 4, 2020

Changed commit from 8cfb932 to 874f853

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 4, 2020

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

874f853Remove symplectic stuff

@mkoeppe
Copy link
Contributor

mkoeppe commented Dec 4, 2020

comment:3

from future import annotations is not available in Python 3.6, which we still support, so I am adding #30551 (Drop Python 3.6 support) as a dependency

@fchapoton
Copy link
Contributor

comment:4

same in #30989

@tobiasdiez
Copy link
Contributor Author

Dependencies: #30551

@tobiasdiez
Copy link
Contributor Author

comment:5

Good to know, (but isn't #30053 already breaking 3.6 support?)

@mkoeppe
Copy link
Contributor

mkoeppe commented Dec 5, 2020

comment:6

Replying to @tobiasdiez:

isn't #30053 already breaking 3.6 support?

That was temporary, just for one beta during the 9.2 cycle. Python 3.6 support was restored in #30576, #30740, #30758.

@fchapoton
Copy link
Contributor

comment:7

red branch

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 20, 2020

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

d74f6c9Merge branch 'develop' of git://github.com/sagemath/sage into public/manifold/typing_more

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 20, 2020

Changed commit from 874f853 to d74f6c9

@tobiasdiez
Copy link
Contributor Author

comment:9

Merged develop branch.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 2, 2021

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

5364cd6Add a bit more typing

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 2, 2021

Changed commit from d74f6c9 to 5364cd6

@mkoeppe mkoeppe modified the milestones: sage-9.3, sage-9.4 Feb 2, 2021
@mkoeppe
Copy link
Contributor

mkoeppe commented May 27, 2021

comment:12

Now that we have dropped support for Python 3.6 (#30551), there is no obstacle to using from future import annotations any more.

@mkoeppe
Copy link
Contributor

mkoeppe commented May 27, 2021

Changed dependencies from #30551 to none

@fchapoton
Copy link
Contributor

comment:13

red branch => needs work

@mkoeppe mkoeppe removed this from the sage-9.4 milestone Jul 19, 2021
@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 21, 2022

comment:48

Rebased onto narrowed #29775

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 21, 2022

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

a3905b5src/sage/tensor/modules/free_module_tensor.py: Conditionalize an import with 'if TYPE_CHECKING:', add 'from `__future__` import annotations'
b2a68dcsrc/sage/manifolds/differentiable/manifold.py: Conditionalize imports with 'if TYPE_CHECKING:'
2061a50src/sage/manifolds/differentiable/degenerate.py: Conditionalize an import with 'if TYPE_CHECKING:'
056a69dsrc/sage/manifolds/differentiable/degenerate_submanifold.py: Conditionalize an import with 'if TYPE_CHECKING:'
f813f3csrc/sage/manifolds/differentiable/tensorfield.py: Don't string-quote types
196f705Add a bit more typing
afd661asrc/sage/tensor/modules: Add missing 'from `__future__` import annotations'
37da606src/sage/tensor/modules/free_module_tensor.py: Fixup

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 21, 2022

Changed commit from b28af09 to 37da606

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 21, 2022

comment:50

Rebased onto updated #29775

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 21, 2022

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

eb83a23src/sage/manifolds/differentiable/manifold.py: Conditionalize an import with 'if TYPE_CHECKING:'
6d382dfsrc/sage/manifolds/differentiable/tensorfield_paral.py: Conditionalize imports with 'if TYPE_CHECKING:'
5a92de8src/sage/manifolds/differentiable/vectorfield_module.py: Conditionalize an import with 'if TYPE_CHECKING:'
71138ffsrc/sage/manifolds/manifold.py: Conditionalize imports with 'if TYPE_CHECKING:'
2527267src/sage/tensor/modules/format_utilities.py: Conditionalize an import with 'if TYPE_CHECKING:'
79a7003src/sage/tensor/modules/free_module_tensor.py: Merge two 'if TYPE_CHECKING:' blocks

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 21, 2022

Changed commit from 37da606 to 79a7003

@tobiasdiez
Copy link
Contributor Author

comment:52

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.

@tobiasdiez
Copy link
Contributor Author

Changed reviewer from Matthias Koeppe, ... to Matthias Koeppe, Tobias Diez

@egourgoulhon
Copy link
Member

comment:53

Merge failure with Sage 9.6.beta6

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 29, 2022

Changed commit from 79a7003 to c403910

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 29, 2022

Branch pushed to git repo; I updated commit sha1. 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
c403910Merge remote-tracking branch 'origin/public/manifolds/typing' into public/manifold/typing_more

@tobiasdiez
Copy link
Contributor Author

comment:55

Merged latest version of #29775

@egourgoulhon
Copy link
Member

comment:56

As in #29775, the coverage issue reported by the patchbot is a spurious one, due to the use of @overload.

Michael, do you agree with the positive review?

@egourgoulhon
Copy link
Member

Changed reviewer from Matthias Koeppe, Tobias Diez to Matthias Koeppe, Tobias Diez, Michael Jung, Eric Gourgoulhon

@tobiasdiez
Copy link
Contributor Author

comment:57

Thanks!

@mjungmath
Copy link

comment:58

Replying to @egourgoulhon:

Michael, do you agree with the positive review?

Yes. I still find some style changes unnecessary, e.g.

-    def set_embedding(self, phi, inverse=None, var=None,
-                      t_inverse=None):
+    def set_embedding(
+        self, phi: ContinuousMap, inverse=None, var=None, t_inverse=None
+    ):

leaving a glimpse of inconsistency behind. If that could be changed: I am happy. But I can also live with this. I would just appreciated it if we can stick to previously established styling rules in the future. Otherwise the code becomes more a patchwork rug than it already is.

@vbraun
Copy link
Member

vbraun commented Apr 3, 2022

Changed branch from public/manifold/typing_more to c403910

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants