Add type annotations for rings and parents by tobiasdiez · Pull Request #41232 · sagemath/sage

tobiasdiez · 2025-11-25T08:09:12Z

Add a bit more typing annotations/stubs for a few fundamental objects (with the goal to make typing of the higher level Python interface easier). They are not meant to be 100% accurate and contain a few to many Anys for my taste, but it should serve as a good start.

📝 Checklist

The title is concise and informative.
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.
I have updated the documentation and checked the documentation preview.

⌛ Dependencies

vincentmacri

Good overall. Just a few questions to make sure I understand this right.

My biggest comment is more of a question. I believe type checkers try to enforce some kind of Liskov substitution principle stuff, which would mean that annotations like def __hash__(self) -> int: are redundant if that annotation is already present in SageObject. So some of the type annotations would be redundant if I'm correct about that.

vincentmacri · 2025-11-26T19:25:48Z

+
+    def __init__(self, parent: Any, codomain: Parent[Any] | None = ...) -> None: ...
+    def __copy__(self) -> Self: ...
+    def parent(self) -> Any: ...


Why isn't this annotated to return Parent?

If I understand the code correctly, its actually always a Homset. I've updated the code now to reflect this.

vincentmacri · 2025-11-26T19:31:03Z

+DomainElementT_contra = TypeVar("DomainElementT_contra", contravariant=True)
+CodomainElementT_co = TypeVar("CodomainElementT_co", covariant=True)


I've seen contravariant and covariant in the Python docs before but haven't really seen them used. For the sake of helping me review this, could you briefly explain what these are doing?

This specifies what concrete types can be substituted, with

Covariance means that a type can be substituted with a more specific type

Contravariance means that a type can be substituted with a more general type

So if you specify Map[Animal, Animal], then for an implementation to be substitutable:

It must accept at least all Animal keys (contravariance)

It must return at most Animal values (covariance)

That being said, since we now use Python 3.12 we can use the new Generic syntax which doesn't need this variance anymore.

vincentmacri · 2025-11-26T19:32:50Z

+    def __reduce__(self) -> tuple[Any, tuple[Any, ...]]: ...
+    def _repr_type(self) -> str: ...
+    def _repr_defn(self) -> str: ...
+    def _repr_(self) -> str: ...


Shouldn't the type annotation for _repr_ (and other methods like __hash__) be inherited from SageObject? I though type checkers enforced that through the Liskov substitution principle (at least some do), which would mean that this annotation is redundant.

If I'm correct about the behaviour of type annotations with inheritance, that would mean that we would get the most value for the amount of work by annotating classes higher up in the inheritance hierarchy (like you've done in this PR).

Shouldn't the type annotation for _repr_ (and other methods like __hash__) be inherited from SageObject? I though type checkers enforced that through the Liskov substitution principle (at least some do), which would mean that this annotation is redundant.

I was playing around with adding annotations to Integer and Element and at least mypy does indeed give an error if the annotations for Integer conflict with the annotations for Element. I didn't check for pyright but I assume it's the same. So I think it should suffice to only annotate methods like _repr_ in SageObject, you don't need to duplicate the annotation (or I think include it in the .pyi file at all if it's already in SageObject. The type checker will be aware of the existing annotation in the superclass.

You can use @override if you want to be more specific about types in a subclass.

That's correct, but there is currently no typing info for Element. I've added @override now, which is good practice anyway (typing checkers will complain if the type in the subclass is different or the method doesn't exist in the parent). Thanks!

That's correct, but there is currently no typing info for Element.

To avoid potential duplicate efforts, just a heads up that I'm working on adding typing info for Element (vincentmacri#22 if you're curious, but it is not remotely ready yet, I'm still experimenting with type checker behaviour and how to get type checking to interact properly with the coercion system).

To avoid potential duplicate efforts, just a heads up that I'm working on adding typing info for Element (vincentmacri#22 if you're curious, but it is not remotely ready yet, I'm still experimenting with type checker behaviour and how to get type checking to interact properly with the coercion system).

Typing for elements seems to kind of depend on this PR. Between mypy, pyright, and ty, only mypy is able to properly handle when a correctly-typed file interacts with an incorrectly-typed file.

fchapoton · 2025-11-28T18:16:20Z

note that "ParentWithGens" is something we should get rid of. Efforts in this direction are currentl blocked at the ticket about moving "fraction_field" method #40213

github-actions · 2025-12-19T05:56:04Z

Documentation preview for this PR (built with commit c2cc5e7; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

tobiasdiez · 2026-01-01T22:36:39Z

@vincentmacri Thanks for the review, and sorry that it took me so long to come back to you (here and in other PRs). I hope I've addressed all your remarks now.

vincentmacri

I noticed a few recurring issues that I want to ask about before I finish reviewing this.

Is there a reason why you use ... for the default value of methods that have an explicit default value in the implementation?
I was under the impression that type stub files should only list Python-accessible functions. For Cython code, this means that cdef functions/methods should not be included in the type stub file, but you seem to have included at least a few cdef functions/methods (there might be more that I didn't notice than just the ones I flagged). Am I misunderstanding type stub files or were these cdef functions/methods included in the stub files by mistake?

vincentmacri · 2026-01-02T17:54:49Z

+        names: NameSpec = ...,
+        normalize: bool = ...,
+        category: Category | None = ...,


Suggested change

names: NameSpec = ...,

normalize: bool = ...,

category: Category | None = ...,

names: NameSpec = None,

normalize: bool = True,

category: Category | None = None,

These are the actual defaults used in the implementation file. Is there a reason not to use the actual defaults here?

I thought pyi files should not contain default values, but you and python/typeshed#8988 proved me wrong. Thanks!

vincentmacri · 2026-01-02T18:02:21Z

+        names: NameSpec = ...,
+        normalize: bool = ...,
+        category: Category | None = ...,


Suggested change

names: NameSpec = ...,

normalize: bool = ...,

category: Category | None = ...,

names: NameSpec = None,

normalize: bool = True,

category: Category | None = None,

Same as above.

vincentmacri · 2026-01-02T18:03:00Z

+        name: str | None = ...,
+        names: NameSpec = ...,


Suggested change

name: str | None = ...,

names: NameSpec = ...,

name: str | None = None,

names: NameSpec = None,

vincentmacri · 2026-01-02T18:08:52Z

+def is_Integer(x: Any) -> bool: ...
+def is_Parent(x: Any) -> bool: ...
+def good_as_coerce_domain(S: Any) -> bool: ...
+def good_as_convert_domain(S: Any) -> bool: ...


Suggested change

def is_Integer(x: Any) -> bool: ...

def is_Parent(x: Any) -> bool: ...

def good_as_coerce_domain(S: Any) -> bool: ...

def good_as_convert_domain(S: Any) -> bool: ...

def is_Parent(x: Any) -> bool: ...

I thought only Python-accessible names go in the type stub files? Aren't the functions is_Integer, good_as_coerce_domain, and good_as_convert_domain all cdef functions that can't be called from Python-space?

Should be fixed now, thanks!

vincentmacri · 2026-01-02T18:09:45Z

+        names: NameSpec = ...,
+        normalize: bool = ...,


Suggested change

names: NameSpec = ...,

normalize: bool = ...,

names: NameSpec = None,

normalize: bool = True,

vincentmacri · 2026-01-02T18:12:49Z

+    def category(self) -> Category: ...
+    def _test_category(self, **options: Any) -> None: ...
+    def _test_eq(self, **options: Any) -> None: ...
+    def init_coerce(self, warn: bool = ...) -> None: ...


Suggested change

def init_coerce(self, warn: bool = ...) -> None: ...

This is a cdef method.

vincentmacri · 2026-01-02T18:17:38Z

@vincentmacri Thanks for the review, and sorry that it took me so long to come back to you (here and in other PRs).

No worries! I've had plenty of other work to do lately as well.

vincentmacri · 2026-01-02T18:18:59Z

I think this is s: needs work but perhaps I'm just misunderstanding something, so s: needs info for now.

vincentmacri · 2026-01-14T18:19:48Z

 from sage.structure.unique_representation import UniqueRepresentation

-_cache: TripleDict[SageObject, SageObject, Category | None, "Homset"] = TripleDict(weak_values=True)
+_cache: "TripleDict[SageObject, SageObject, Category | None, Homset]" = TripleDict(weak_values=True)


Add from __future__ import annotations to the top of the file and you can remove the quotes here.

vincentmacri · 2026-01-15T16:12:50Z

+    def __mul__(self, right: Map[Any, Any]) -> Map[Any, Any]: ...
+    def _composition(self, right: Map[Any, Any]) -> Map[Any, Any]: ...
+    def _composition_(self, right: Map[Any, Any], homset: Any) -> Map[Any, Any]: ...


Not necessary for this PR, just pointing out that these functions would be could candidates for overloads utilizing generics in the future, i.e. if $f \colon B \to C$ and $g \colon A \to B$ then $f \circ g \colon A \to C$.

vincentmacri · 2026-01-15T16:20:43Z

There are a couple suggestions I made for default values in parent.pyi that I think you forgot to accept. Otherwise looks good. There are parts that could have more precise types, but this is a great start and we can build upon it in the future. You can set to positive review after adding the suggested values to parent.pyi.

Thanks for the contribution! Once this is merged I think I can finish the type annotations for Integer.

Co-authored-by: Vincent Macri <vincent.macri@ucalgary.ca>

vincentmacri · 2026-01-15T22:21:48Z

Test failure seems irrelevant.

tobiasdiez · 2026-01-16T08:13:48Z

Thanks!

sagemathgh-41232: Add type annotations for rings and parents       Add a bit more typing annotations/stubs for a few fundamental objects (with the goal to make typing of the higher level Python interface easier). They are not meant to be 100% accurate and contain a few to many `Any`s for my taste, but it should serve as a good start. ### 📝 Checklist  - [ ] The title is concise and informative. - [ ] 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. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies    URL: sagemath#41232 Reported by: Tobias Diez Reviewer(s): Tobias Diez, Vincent Macri

github-actions bot added the s: needs review label Nov 25, 2025

tobiasdiez force-pushed the typing-catrings branch 2 times, most recently from c7768dd to fab7698 Compare November 25, 2025 08:11

tobiasdiez requested review from fchapoton and vincentmacri November 25, 2025 08:11

Add type annotations for rings and parents

315aa5c

tobiasdiez force-pushed the typing-catrings branch from fab7698 to 315aa5c Compare November 25, 2025 08:14

vincentmacri reviewed Nov 26, 2025

View reviewed changes

Merge branch 'develop' into typing-catrings

a1c4b27

tobiasdiez and others added 2 commits December 29, 2025 15:58

Merge branch 'develop' into typing-catrings

aaba4da

Enhance type annotations for Homset and Map classes

b6070a6

Fix linter

aacd66a

vincentmacri reviewed Jan 2, 2026

View reviewed changes

vincentmacri added s: needs info and removed s: needs review labels Jan 2, 2026

Improve typing info (in particular add default values)

d2e37e2

github-actions bot added s: needs review and removed s: needs info labels Jan 3, 2026

tobiasdiez added 2 commits January 3, 2026 23:16

Lazy type annotation for _cache in Homset class

e996198

Fix linter

e7f2f30

vincentmacri reviewed Jan 14, 2026

View reviewed changes

vincentmacri added s: needs work t: refactoring and removed s: needs review labels Jan 14, 2026

Merge remote-tracking branch 'origin/develop' into pr/tobiasdiez/41232

c274c32

github-actions bot added s: needs review and removed s: needs work labels Jan 15, 2026

Further improve typing in homset

ad9baaa

vincentmacri reviewed Jan 15, 2026

View reviewed changes

vincentmacri added s: needs work and removed s: needs review labels Jan 15, 2026

Apply suggestions from code review

c2cc5e7

Co-authored-by: Vincent Macri <vincent.macri@ucalgary.ca>

github-actions bot added s: needs review and removed s: needs work labels Jan 15, 2026

vincentmacri added s: positive review and removed s: needs review labels Jan 15, 2026

vbraun merged commit ab9bc81 into sagemath:develop Jan 25, 2026
22 of 23 checks passed

github-actions bot removed the s: positive review label Jan 25, 2026

		DomainElementT_contra = TypeVar("DomainElementT_contra", contravariant=True)
		CodomainElementT_co = TypeVar("CodomainElementT_co", covariant=True)

Uh oh!

Conversation

tobiasdiez commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📝 Checklist

⌛ Dependencies

Uh oh!

vincentmacri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vincentmacri Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fchapoton commented Nov 28, 2025

Uh oh!

github-actions bot commented Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tobiasdiez commented Jan 1, 2026

Uh oh!

vincentmacri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vincentmacri commented Jan 2, 2026

Uh oh!

vincentmacri commented Jan 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vincentmacri commented Jan 15, 2026

Uh oh!

vincentmacri commented Jan 15, 2026

Uh oh!

tobiasdiez commented Jan 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

tobiasdiez commented Nov 25, 2025 •

edited

Loading

vincentmacri Dec 11, 2025 •

edited

Loading

github-actions bot commented Dec 19, 2025 •

edited

Loading