-
-
Notifications
You must be signed in to change notification settings - Fork 514
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
Make Composition
s into a collections.abc.Sequence
#34527
Comments
comment:1
The methods required according to the collections documentation (https://docs.python.org/3/library/collections.abc.html#collections-abstract-base-classes) are present except for
|
New commits:
|
Commit: |
Branch: u/tkarn/composition-34527 |
This comment has been minimized.
This comment has been minimized.
comment:4
Maybe some doctests to illustrate the supported operations would be good, to help other Sage users understand the Sequence protocol? |
comment:5
Also in the doctest I would suggest to use the fully qualified name |
comment:6
+1 to Matthias’s comments; in particular the It might make more sense to simply inherit from |
comment:7
It looks like things are sufficiently fixed just by calling |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:9
Replying to Trevor Karn:
Yes, this is equivalent to inheriting from |
comment:10
comment:4? |
comment:11
Replying to Matthias Köppe:
Sorry, I think I misunderstood. Since we are registering it as a |
comment:12
Replying to Trevor Karn:
Does it also pass the
What is the type of the result from applying |
comment:13
Replying to Matthias Köppe:
I realize that I have to clarify this: It is equivalent as far as |
comment:14
Replying to Travis Scrimshaw:
Yes, of course, that should be added as a test maybe:
|
comment:15
Replying to Travis Scrimshaw:
|
comment:16
Replying to Trevor Karn:
It is followed. The "mixin" methods are not part of the protocol. |
comment:17
Replying to Matthias Köppe:
No, sorry, my mistake. It's getting late here. You are right, it is not allowed to register it without implementing these mixin methods. |
comment:18
Replying to Matthias Köppe:
Ah, right, duh. It is an iterator and does not return the reversed object. |
comment:19
After subclassing:
Should this be fixed at the |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:21
Replying to Trevor Karn:
Ouch. Python used a metaclass for their (collections) ABCs
No, that is going to go away at some point.
This won't solve the problem:
|
comment:22
Replying to Travis Scrimshaw:
Got it - then I think this is ready for review. I tried to inclued the doctests which were relevant to this conversation, but please let me know if I need more.
Huh. Should |
comment:23
Replying to Trevor Karn:
This is saying it also has a metaclass. |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:26
Documentation is updated. |
comment:27
Thanks. I am happy with this (modulo the one change below). Matthias? Sphinx treats the backtick as normal text when between alpha(numeric?) characters: Typically, instances of ``collections.abc.Sequence`` have a ``.count`` method.
- This is *not* the case for ``Composition``s::
+ This is *not* the case for a ``Composition``:: |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:29
I think in comment:17, I concluded that all mix-in methods must be provided. Too tired this evening to re-read the Python reference to confirm it |
comment:30
Replying to Matthias Köppe:
I understand this python discussion post to mean that mix-in methods are not required. On the other hand it is easy enough to implement |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:32
Replying to Matthias Köppe:
Ok. I think |
comment:33
Replying to Matthias Köppe:
This is in point (2) of the main Python ABC doc in comment:1. |
Reviewer: Travis Scrimshaw, Matthias Köppe |
comment:34
That takes care of all of the mixin methods. So I am good with this. Anything objections to a positive review Matthias? |
comment:35
LGTM, thanks. |
Changed branch from u/tkarn/composition-34527 to |
Currently Sage does not understand
Composition
s as acollections.abc.Sequence
:This ticket registers
Composition
as aSequence
.CC: @tscrim @trevorkarn
Component: combinatorics
Keywords: gsoc2022 compositions sequence
Author: Trevor K. Karn
Branch/Commit:
78f3a92
Reviewer: Travis Scrimshaw, Matthias Köppe
Issue created by migration from https://trac.sagemath.org/ticket/34527
The text was updated successfully, but these errors were encountered: