add some typing information to interval-posets

[fchapoton]

just as a little experiment

CC: @tobiasdiez

Component: combinatorics

Author: Frédéric Chapoton

Branch/Commit: 2272bc3

Reviewer: Tobias Diez

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

[fchapoton]

New commits:

99c5273

add some basic typing information to Tamari interval posets

[fchapoton]

Branch: u/chapoton/30989

[fchapoton]

Commit: 99c5273

[sagetrac-git]

Changed commit from 99c5273 to 3d6644a

[sagetrac-git]

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

3d6644a

fix bad types

[tobiasdiez]

comment:4

In general, this looks good to me (I have admit I didn't went through the code in detail to check whether the added typing information is correct.)

One suggestion for improvement: for lists, tuple and iterators there are special typing annotation that allow to specify also the type of elements in these lists, e.g. https://docs.python.org/3/library/typing.html#typing.Iterator
After from future import annotations, you can simply write list[str] for a list of strings.

Moreover, it would be good to also annotate the method parameters. For example, https://github.com/microsoft/pyright/blob/master/docs/getting-started.md comments

The annotations that provide most value are on function input parameters, instance variables, and return parameters (in that order).

[sagetrac-git]

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

e1faf42

import annotations from future

[sagetrac-git]

Changed commit from 3d6644a to e1faf42

[fchapoton]

comment:6

Thanks for the hint.

I think it would be slightly premature to add more typing here, without thinking in general about the relationship with the Parent/Element framework.

[tobiasdiez]

Reviewer: Tobias Diez

[tobiasdiez]

comment:7

Sure, it's already better than no typing at all.

Feel free to set it to "positive review" after the tests pass.

[fchapoton]

comment:8

ok, thanks. For sure, only a first step.

I was wondering how to add typing information explaining that the output type has the same "Parent" as self.

[tobiasdiez]

comment:10

Replying to @fchapoton:

I was wondering how to add typing information explaining that the output type has the same "Parent" as self.

In general you can use @overload for methods to provide a more refined typing information depending on the type of the arguments, in particular, also of self.
For example,

    @overload
    def method(self: 'MyClass', other: 'OtherClass') -> 'ReturnClass':
        pass

    def method(self: Any, other: Any) -> Any:
        ....

The category framework probably needs further work to be able to provide meaningful typing information. One way path would be to use generics. For example, sage.categories.action.Action could be declared as

G = TypeVar('G') # possibly with some constraints
S = TypeVar('S')

class Action(Generic[G, S]):

   def __init__(G: G, S: S):
       ....
   
   def actor(self) -> G:
       ....

   def codomain(self) -> S:
       ...

[fchapoton]

Dependencies: #30551

[mkoeppe]

comment:12

To my understanding, type annotations themselves are supported in Python 3.6.
from future import annotations is for "postponed evaluation of annotations" (https://www.python.org/dev/peps/pep-0563/)

I think the annotations added on this ticket do not need from future import annotations - so this line can be removed and the dependency can be dropped

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. This was a forced push. New commits:

fdadfa8

add some basic typing information to Tamari interval posets

[sagetrac-git]

Changed commit from e1faf42 to fdadfa8

[fchapoton]

comment:14

ok, here is a more basic annotation, without future import

Using python 3.8.5, I needed to change back list[tuple] to just list

please review again

[fchapoton]

Changed dependencies from #30551 to none

[tobiasdiez]

comment:15

If you don't want to use __future__, then you have to use List[Tuple[...]] from typing, and also sometimes wrap typing information in strings (e.g. so that you don't run into issues with forward references).
I'm in favor of using the future import, since this can easily be migrated once only >= python 3.9 is supported (by simply removing the import).

I think it's a good idea to specify the complete typing information, so also specify the type of the list items, i.e. List[str] instead of only List. Same for the iterator.

[sagetrac-git]

Changed commit from fdadfa8 to e74fa70

[sagetrac-git]

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

e74fa70

use List and Tuple

[fchapoton]

comment:17

ok, here is a commit for using List[Tuple]

[tobiasdiez]

comment:19

There are merged conflicts now.

[sagetrac-git]

Changed commit from e74fa70 to 2272bc3

[sagetrac-git]

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

2272bc3

Merge branch 'u/chapoton/30989' in 9.3.b3

[fchapoton]

comment:21

merged

[tobiasdiez]

comment:22

LGTM. The patchbot fails, but the errors seem not relevant to the changes on the branch.

[vbraun]

Changed branch from u/chapoton/30989 to 2272bc3