Add typing to chart and calculus method #35188

tobiasdiez · 2023-02-24T12:33:48Z

📚 Description

Adds (almost complete) typing information for charts (and the related calculus method).

📝 Checklist

I have made sure that the title is self-explanatory and the description concisely explains the PR.
I have linked an issue or discussion.
I have created tests covering the changes.
I have updated the documentation accordingly.

⌛ Dependencies

codecov-commenter · 2023-02-24T15:28:53Z

Codecov Report

Base: 88.59% // Head: 88.58% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (820440f) compared to base (05329f6).
Patch coverage: 91.73% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #35188      +/-   ##
===========================================
- Coverage    88.59%   88.58%   -0.02%     
===========================================
  Files         2140     2140              
  Lines       396998   397304     +306     
===========================================
+ Hits        351740   351943     +203     
- Misses       45258    45361     +103

Impacted Files	Coverage Δ
src/sage/manifolds/manifold.py	`85.38% <87.50%> (ø)`
src/sage/manifolds/subset.py	`94.00% <88.88%> (-0.19%)`	⬇️
src/sage/manifolds/point.py	`92.27% <90.00%> (-0.32%)`	⬇️
src/sage/manifolds/chart.py	`90.48% <91.54%> (-0.51%)`	⬇️
src/sage/manifolds/calculus_method.py	`97.46% <95.65%> (-1.15%)`	⬇️
src/sage/groups/affine_gps/euclidean_group.py	`88.88% <0.00%> (-11.12%)`	⬇️
...e/combinat/cluster_algebra_quiver/mutation_type.py	`50.35% <0.00%> (-4.28%)`	⬇️
src/sage/categories/super_modules_with_basis.py	`96.29% <0.00%> (-3.71%)`	⬇️
src/sage/plot/histogram.py	`96.77% <0.00%> (-3.23%)`	⬇️
src/sage/groups/affine_gps/affine_group.py	`96.62% <0.00%> (-2.25%)`	⬇️
... and 47 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

### 📚 Description In preparation for important package upgrades, we remove support for Python v3.8. (Almost) all occurrences of "Python 3.8" are removed from the codebase. There are still some comments left that essentially say that in newer versions things can be simplified, but I didn't touch those and leave them for follow-up PRs. This will help with: - PRs that require Python 3.9 or above, such as sagemath#34973 and sagemath#35188. - Upgrades of Python packages that have dropped support for Python 3.8 because they follow NEP 29 and have already released a new major version that drops support according to the NEP 29 schedule. - NetworkX 3.2 (expected 2023-??; version 3.1 was released 2023-04, see sagemath#35671) - sagemath#34816 - sagemath#35703 - See also sagemath#32074. Test run: https://github.com/tobiasdiez/sage/actions/runs/4586981626     ### 📝 Checklist  - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [x] I have updated the documentation accordingly. ### ⌛ Dependencies   URL: sagemath#35404 Reported by: Tobias Diez Reviewer(s): Dima Pasechnik, Matthias Köppe

github-actions · 2023-09-29T15:16:51Z

Documentation preview for this PR (built with commit c930dca; changes) is ready! 🎉

tobiasdiez · 2023-09-30T01:20:36Z

Now that Python 3.8 is dropped, this PR is finally good to go. @egourgoulhon could you please review it? Thanks!

(The two failing tests are happening on all PRs right now)

mkoeppe · 2023-10-01T18:14:23Z

src/sage/manifolds/calculus_method.py


 # Conversion functions
-def _SR_to_Sympy(expression):
+def _SR_to_Sympy(expression: Expression) -> Expression:


The declared return type is wrong. It returns a sympy object, not an Expression.

And also the declared input is wrong because this function first marshals the input into SR. It is allowed to be all kinds of things.

mkoeppe · 2023-10-01T18:16:30Z

src/sage/manifolds/calculus_method.py

-    def is_trivial_zero(self, expression, method=None):
+    def is_trivial_zero(
+        self, expression: Expression, method: Optional[CalculusMethodName] = None
+    ) -> bool:


Also here, the type declaration Expression is wrong. It depends on method what type is allowed.

mkoeppe · 2023-10-01T18:19:39Z

src/sage/manifolds/chart.py

+    def __classcall__(
+        cls: Type[Chart],
+        domain: TopologicalManifold,
+        coordinates: str = "",


No, str is only one of the allowed types, see function body

mkoeppe · 2023-10-01T18:24:33Z

src/sage/manifolds/chart.py

@@ -3416,7 +3520,7 @@ def _latex_(self):
        """
        return latex(self._chart1) + r' \rightarrow ' + latex(self._chart2)

-    def __eq__(self, other):
+    def __eq__(self, other: CoordChange) -> bool:


Not correct. It is allowed to pass an object of a different type as other. The method returns False in this case.

There seems to be two conventions for this. Either you use indeed object for other, or you restrict other to some meaningful subset. The later is employed by e.g numpy and many others. The logic here seems to be that its more helpful to have your static checker complain that you compare apples and bananas, then to correctly handle the relatively rare case of comparing two completely different object (in which case you can ignore the error).

mkoeppe · 2023-10-01T18:26:30Z

src/sage/manifolds/chart.py

@@ -3848,7 +3954,7 @@ def restrict(self, dom1, dom2=None):
        return type(self)(self._chart1.restrict(dom1),
                          self._chart2.restrict(dom2), *(self._transf.expr()))

-    def display(self):
+    def display(self) -> str:


not correct; a FormattedExpansion is not an str.

mkoeppe · 2023-10-01T18:27:34Z

src/sage/manifolds/subset.py

@@ -1320,7 +1326,7 @@ def get_subset(self, name):

    #### End of accessors

-    def is_subset(self, other):
+    def is_subset(self, other: TopologicalManifold):


Not correct. other is allowed to be a mere ManifoldSubset

mkoeppe · 2023-10-01T18:36:22Z

src/sage/manifolds/chart.py

@@ -553,7 +602,7 @@ def _latex_(self):
        description += latex(self._xx[n-1]).strip() + r')\right)'
        return description

-    def _first_ngens(self, n):
+    def _first_ngens(self, n: int):


int is not correct; here and in many other places. A Sage Integer is definitely allowed.
I'd suggest to use the numeric ABCs; https://docs.python.org/3/library/numbers.html

these abc's are not supported (in the way you want) by pyright and mypy, see microsoft/pyright#1575 and references therein. We also cannot use Integer since its a cython class and there are no stubs for that (yet). So yeah, its a partial information that is still better than nothing (since ints are allowed).

I'm pretty sure that the mantra "don't lie to your static checker" should apply.

Acceptable "partial information" would be using a supertype, not a subtype, of what is allowed. If our Integer cannot be represented, then this will just have to be an Any or object.

@tobiasdiez Do you have an opinion on what tools to use for generating the typing stubs (pyi) for our Cython classes? A quick search led me to https://github.com/RaubCamaioni/CythonPEG, but I'm guessing there must be more available

Acceptable "partial information" would be using a supertype, not a subtype, of what is allowed. If our Integer cannot be represented, then this will just have to be an Any or object.

With any you don't get an error if you pass in a string. With int you can pass in any sage integer (which is treated as any). Since there are so many open challenges with even getting basic type checks to work, the mantra would be more "over time lie less and less to your static checker".

@tobiasdiez Do you have an opinion on what tools to use for generating the typing stubs (pyi) for our Cython classes? A quick search led me to https://github.com/RaubCamaioni/CythonPEG, but I'm guessing there must be more available

There is some movement to implement this directly in Cython. I would wait for this. There is an open issue in the Cython repo (that also compares different workarounds/packages in case you are interested)...

Acceptable "partial information" would be using a supertype, not a subtype, of what is allowed. If our Integer cannot be represented, then this will just have to be an Any or object.

+1

With any you don't get an error if you pass in a string. With int you can pass in any sage integer (which is treated as any). Since there are so many open challenges with even getting basic type checks to work, the mantra would be more "over time lie less and less to your static checker".

It's not only that you are lying to your static checker: you are also lying to the user/developer examining the source code. IMHO, no typing information is better than a false one.

Note that the objective of these type checks is to discover bugs. In this case, that you get notified about passing something wrong into the method. The idea that the types should strictly represent the runtime behavior is not very helpful. The numpy docs for example state

NumPy is very flexible. Trying to describe the full range of possibilities statically would result in types that are not very helpful. For that reason, the typed NumPy API is often stricter than the runtime NumPy API.

But yes, I agree that finding a solution for the integers would be nice. Let me think about it (and propose ideas in case you have some).

mkoeppe · 2023-10-01T18:37:28Z

src/sage/manifolds/chart.py

@@ -3448,7 +3552,7 @@ def __eq__(self, other):
                and (self._chart2 == other._chart2)
                and (self._transf == other._transf))

-    def __ne__(self, other):
+    def __ne__(self, other: CoordChange) -> bool:


Likewise here

tobiasdiez added 3 commits February 24, 2023 09:55

Add typing to chart and calculus method

4d8f6dc

Update python version

f8d4648

Minor fixes

a30565a

tobiasdiez added s: needs review c: manifolds labels Feb 24, 2023

tobiasdiez requested a review from egourgoulhon February 24, 2023 12:33

tobiasdiez mentioned this pull request Feb 24, 2023

Add typing to chart and calculus method #35187

Closed

4 tasks

tobiasdiez added 3 commits February 25, 2023 05:08

Fix import

c5e1080

Fix support for python 3.8

820440f

add debug info

b5886e1

tobiasdiez removed the s: needs review label Mar 1, 2023

add more logging

71bdece

tobiasdiez mentioned this pull request Mar 31, 2023

Drop support for Python 3.8 #35404

Merged

5 tasks

Merge branch 'develop' into typing_charts

c930dca

tobiasdiez added the s: needs review label Sep 30, 2023

mkoeppe reviewed Oct 1, 2023

View reviewed changes

vbraun force-pushed the develop branch from 883e05f to e349b00 Compare November 12, 2023 16:25

vbraun force-pushed the develop branch from eba5e19 to e5f42fa Compare June 3, 2024 22:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add typing to chart and calculus method #35188

Add typing to chart and calculus method #35188

tobiasdiez commented Feb 24, 2023

codecov-commenter commented Feb 24, 2023 •

edited

Loading

github-actions bot commented Sep 29, 2023

tobiasdiez commented Sep 30, 2023

mkoeppe Oct 1, 2023

mkoeppe Oct 1, 2023

mkoeppe Oct 1, 2023

mkoeppe Oct 1, 2023

tobiasdiez Oct 2, 2023

mkoeppe Oct 1, 2023

mkoeppe Oct 1, 2023

mkoeppe Oct 1, 2023

tobiasdiez Oct 2, 2023

mkoeppe Oct 2, 2023

mkoeppe Oct 2, 2023

tobiasdiez Oct 2, 2023

egourgoulhon Oct 2, 2023

tobiasdiez Oct 2, 2023

mkoeppe Oct 1, 2023

Add typing to chart and calculus method #35188

Are you sure you want to change the base?

Add typing to chart and calculus method #35188

Conversation

tobiasdiez commented Feb 24, 2023

📚 Description

📝 Checklist

⌛ Dependencies

codecov-commenter commented Feb 24, 2023 • edited Loading

Codecov Report

github-actions bot commented Sep 29, 2023

tobiasdiez commented Sep 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 24, 2023 •

edited

Loading