#32058 closed enhancement (fixed)

add typing annotations in permutations

Reported by: Frédéric Chapoton Owned by:
Priority: major Milestone: sage-9.4
Component: combinatorics Keywords:
Cc: Travis Scrimshaw, Samuel Lelièvre, Tobias Diez Merged in:
Authors: Frédéric Chapoton Reviewers: Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: f0534fe (Commits, GitHub, GitLab) Commit: f0534fe010382260c100dc1cd684cb1a255a72d3
Dependencies: Stopgaps:

Status badges

Description

not everywhere, but a good start

Change History (33)

comment:1 Changed 18 months ago by Frédéric Chapoton

Branch: u/chapoton/32058
Commit: d7a9ea8781a336a25f1f7458dc21c01db9d8bafb
Status: newneeds_review

New commits:

d7a9ea8some typing annotation in permutation.py

comment:2 Changed 18 months ago by Frédéric Chapoton

Cc: Travis Scrimshaw Samuel Lelièvre added

bot is morally green, please have a look

comment:3 Changed 18 months ago by Samuel Lelièvre

Can you say why "Permutation" needs quotes but not Composition?

Some changes I would consider (unless they slow things down):

-        return [i + 1 for i in range(len(self)) if i + 1 == self[i]]
+        return [i for i, v in enumerate(self, start=1) if i == v]
-        return {i + 1: self[i] for i in range(len(self))}
+        return dict(enumerate(self, start=1))

But feel free to ignore as this leaves the ticket's scope.

Your changes in passing from the previous loops are a clear improvement.

comment:4 Changed 18 months ago by git

Commit: d7a9ea8781a336a25f1f7458dc21c01db9d8bafbd502d6b791844dad9e7440c212feb44c11dc3b27

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

d502d6bminor details in permutation.py

comment:5 Changed 18 months ago by Frédéric Chapoton

merci pour les suggestions. C'est fait.

Pour "Permutation", c'est parce que c'est nécessaire pour annoter les méthodes d'une classe par elle-même. Pour éviter une boucle. Enfin, je crois.

comment:6 Changed 18 months ago by Matthias Köppe

Cc: Tobias Diez added

Please add from __future__ import annotations to the top of files that use type annotations.

comment:7 Changed 18 months ago by Frédéric Chapoton

this is not necessary for the annotations applied here, if I am not mistaken

and this was not applied to the existing annotations:

:~/sage$ git grep -c ") -> .*:$" src/sage/combinat/
src/sage/combinat/affine_permutation.py:10
src/sage/combinat/combinat.py:35
src/sage/combinat/interval_posets.py:81
src/sage/combinat/knutson_tao_puzzles.py:28
src/sage/combinat/necklace.py:3
src/sage/combinat/parking_functions.py:22
src/sage/combinat/permutation.py:64
src/sage/combinat/plane_partition.py:19
src/sage/combinat/posets/posets.py:15
~/sage$ git grep -c "future__.*annota" src/sage/combinat/
Last edited 18 months ago by Frédéric Chapoton (previous) (diff)

comment:8 Changed 18 months ago by Matthias Köppe

The previous annotations were probably written before we dropped Python 3.6 support in #30551. Only since Python 3.7, from __future__ import annotations exists.

I would think it's better to write these type annotations to one language standard instead of waiting for the first incompatibility to happen...

comment:9 Changed 17 months ago by Tobias Diez

With the future import, you no longer have problems with the forward references. So in particular, you can simply write

- def inverse(self) -> "Permutation":
+ def inverse(self) -> Permutation:

Moreover, it would be good if the type of the list items would also be specified, i.e. list[int] instead of only list. Please also use list[int] instead of List[int] (with capital L). The latter is deprecated with Python 3.9, see https://docs.python.org/3/library/typing.html#typing.List.

comment:10 Changed 17 months ago by Tobias Diez

@mkoeppe: The future import is not always necessarily. If you only use simple types, then there is no practical difference (except for the minimal performance impact of the runtime evaluation of types), so I don't think we need to import it in every file that uses type information. But as soon as you would use string-literal typing or the soon-deprecated List etc, I would say the future import makes sense.

comment:11 Changed 17 months ago by Frédéric Chapoton

I agree that we should try to use typing annotations in a coherent way. Alas, this has been a moving ground, with behaviour changing a lot between 3.6 and 3.9. As long as we do not refuse python 3.7 and 3.8, we should keep things simple.

This ticket is intended only as a first approximation of typing on this file. So please, let us move on and keep enhancements for later.

comment:12 in reply to:  10 ; Changed 17 months ago by Matthias Köppe

Replying to gh-tobiasdiez:

@mkoeppe: The future import is not always necessarily.

My point is that -- whether it is necessary or not for a specific file -- it is better to add it, because that reduces both the cognitive load for developers, and reduces the chance that things get broken.

comment:13 in reply to:  11 Changed 17 months ago by Matthias Köppe

Status: needs_reviewneeds_work

Replying to chapoton:

I agree that we should try to use typing annotations in a coherent way. Alas, this has been a moving ground, with behaviour changing a lot between 3.6 and 3.9. As long as we do not refuse python 3.7 and 3.8, we should keep things simple.

There is no point in writing code to an already outdated standard when adding from __future__ import annotations makes the current standard available to all supported Python versions.

comment:14 in reply to:  9 ; Changed 17 months ago by Matthias Köppe

Replying to gh-tobiasdiez:

Please also use list[int] instead of List[int] (with capital L). The latter is deprecated with Python 3.9, see https://docs.python.org/3/library/typing.html#typing.List.

But list[int] was only introduced in Python 3.9, so we can't use it yet as we still support Python 3.7 and 3.8. So we have to either use List[int] -- or keep things simple and just use list.

comment:15 in reply to:  14 ; Changed 17 months ago by Tobias Diez

Replying to mkoeppe:

Replying to gh-tobiasdiez:

Please also use list[int] instead of List[int] (with capital L). The latter is deprecated with Python 3.9, see https://docs.python.org/3/library/typing.html#typing.List.

But list[int] was only introduced in Python 3.9, so we can't use it yet as we still support Python 3.7 and 3.8. So we have to either use List[int] -- or keep things simple and just use list.

It's backported to Python 3.7 using from __future__ import annotations, see https://www.python.org/dev/peps/pep-0585/#implementation

comment:16 in reply to:  12 ; Changed 17 months ago by Tobias Diez

Replying to mkoeppe:

Replying to gh-tobiasdiez:

@mkoeppe: The future import is not always necessarily.

My point is that -- whether it is necessary or not for a specific file -- it is better to add it, because that reduces both the cognitive load for developers, and reduces the chance that things get broken.

Good point. Maybe it's worthwhile to document this in the dev docs.

comment:17 in reply to:  16 Changed 17 months ago by Matthias Köppe

Replying to gh-tobiasdiez:

Maybe it's worthwhile to document this in the dev docs.

+1 on a section on best practices for type annotations! I've opened #32067 for this.

I think we may also want to add a patchbot plugin that enforces presence of from __future__ import annotations whenever something that looks like a typing annotation is found in the file.

comment:18 in reply to:  15 Changed 17 months ago by Matthias Köppe

Replying to gh-tobiasdiez:

Replying to mkoeppe:

Replying to gh-tobiasdiez:

Please also use list[int] instead of List[int] (with capital L). The latter is deprecated with Python 3.9, see https://docs.python.org/3/library/typing.html#typing.List.

But list[int] was only introduced in Python 3.9, so we can't use it yet as we still support Python 3.7 and 3.8. So we have to either use List[int] -- or keep things simple and just use list.

It's backported to Python 3.7 using from __future__ import annotations, see https://www.python.org/dev/peps/pep-0585/#implementation

Thanks for the pointer! I didn't know that.

comment:19 Changed 17 months ago by git

Commit: d502d6b791844dad9e7440c212feb44c11dc3b277b0ec5295c68a829c1772e7e1c49b8a87792852b

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

7b0ec52do not use soon-deprecated List annotation

comment:20 Changed 17 months ago by Frédéric Chapoton

Status: needs_workneeds_review

here is a version

  • not using "List", that is going soon to be deprecated
  • not using "list[int]" or anything that would require a __future__ import

so backs to needs review

comment:21 Changed 17 months ago by Matthias Köppe

Status: needs_reviewneeds_work

Please, let's use the __future__. Not doing so just sets the stage for a thousand "cleanup" tickets later.

comment:22 Changed 17 months ago by Frédéric Chapoton

I you insist, I can do that, but I do not see the point, because I am not doing here anything that will need any cleanup later.

comment:23 Changed 17 months ago by git

Commit: 7b0ec5295c68a829c1772e7e1c49b8a87792852b555c8ca2171d1c226ecea50a4d511284c723a20a

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

555c8caadding one future import that will need to be removed later

comment:24 Changed 17 months ago by Frédéric Chapoton

Status: needs_workneeds_review

here we go

comment:25 in reply to:  22 Changed 17 months ago by Matthias Köppe

Replying to chapoton:

I you insist, I can do that, but I do not see the point

comment 12 above

because I am not doing here anything that will need any cleanup later.

yes you do, comment 9 above.

comment:26 Changed 17 months ago by Matthias Köppe

Status: needs_reviewneeds_work

The type annotation "Permutation" is unnecessarily string-quoted - https://www.python.org/dev/peps/pep-0563/

comment:27 Changed 17 months ago by git

Commit: 555c8ca2171d1c226ecea50a4d511284c723a20a0dc73cb40f457ffb1f6afe6df128eb9e53defcb1

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

0dc73cbno need for quotes on annotations

comment:28 Changed 17 months ago by Frédéric Chapoton

Status: needs_workneeds_review

done


New commits:

0dc73cbno need for quotes on annotations

New commits:

0dc73cbno need for quotes on annotations

comment:29 Changed 17 months ago by Matthias Köppe

Reviewers: Matthias Koeppe
Status: needs_reviewpositive_review

Thank you, LGTM.

comment:30 Changed 17 months ago by Volker Braun

Status: positive_reviewneeds_work

merge conflict

comment:31 Changed 17 months ago by git

Commit: 0dc73cb40f457ffb1f6afe6df128eb9e53defcb1f0534fe010382260c100dc1cd684cb1a255a72d3

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

f0534feMerge branch 'u/chapoton/32058' in 9.4.b4

comment:32 Changed 17 months ago by Frédéric Chapoton

Status: needs_workpositive_review

setting back to positive after trivial rebase

comment:33 Changed 17 months ago by Volker Braun

Branch: u/chapoton/32058f0534fe010382260c100dc1cd684cb1a255a72d3
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.