Update combinations docs by ruaridhw · Pull Request #19732 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation17 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
- The order of the iterable is not modified within
combinations()as these docs imply - Instead, the order of elements is used as-is
from itertools import combinations list(combinations(list(range(4)), 2))
[(0, 1), (0, 2), (0, 3), (1, 2), (1, 3), (2, 3)]
list(combinations(reversed(list(range(4))), 2))
[(3, 2), (3, 1), (3, 0), (2, 1), (2, 0), (1, 0)]
I would have expected that should these docs be true, the last line starts with (0, 1) or perhaps (2, 3) if we are to interpret that the tuple itself is sorted. Instead, the order of iterable is used (as expected).
Hello, and thanks for your contribution!
I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).
Recognized GitHub username
We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:
This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.
You can check yourself to see if the CLA has been received.
Thanks again for the contribution, we look forward to reviewing it!
Found these docs a little misleading, please let me know if this requires a BPO entry.
The same can be said of combinations_with_replacement(), permutations(), and product() so if this is applicable, probably best to add changes to those docs as well.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ruaridhw, thanks for taking the time to improve the documentation.
I'm not sure this new wording is much better though, I think we should keep the reference to the lexicographic order. Maybe we should move the note that elements are used based on their order and not their value here instead?
I think we should keep the reference to the lexicographic order
@remilapeyre, what was your reasoning for this? Personally, this was the confusing part for me and the only reference that I'd like to remove. I think I'm missing something because I can't see how these methods care about lexicographic order at all.
Take for example, a completely unsortable class:
from itertools import combinations
class NotSortable: def init(self, id_): self.id = id_
def __repr__(self):
return f'{self.id}'sorted([NotSortable(3), NotSortable(2)])
TypeError: '<' not supported between instances of 'NotSortable' and 'NotSortable'
combs = list(combinations([NotSortable(3), NotSortable(2), NotSortable(4), NotSortable(1)], 2)) print(combs)
[(3, 2), (3, 4), (3, 1), (2, 4), (2, 1), (4, 1)]
Surely any "lexicographic order" (or any ordering for that matter other than the one provided) of this input iterable is undefined?
With this unsortable class we can have the objects
a = NotSortable(3) b = NotSortable(2) c = NotSortable(4) d = NotSortable(1)
and we can define pos() so that
pos(a) = 1 pos(b) = 2 pos(c) = 3 pos(d) = 4
and pos() defines an order over {a, b, c, d} and combinations() will output pairs in lexicographic order. It's just not the order defined by NotSortable.__lt__() but the one defined by list.index(). It can be confusing but it seems to me that it is correct and we should maybe improve the phrasing but not remove the whole paragraph.
Hmmm that does seem highly confusing. My point is that there are no cases where it output order is defined by anything other than list.index(). Hence we should explicitly state this.
Stating "combinations are emitted in lexicographic sort order" reads to me as if the elements of the iterable or the iterable itself must define a lexicographic sort order and this is not the case. Especially because if the elements do define a lexicographic sort order (eg. str), this is not the order of the output pairs:
print(list(combinations('dbca', 2)))
[('d', 'b'), ('d', 'c'), ('d', 'a'), ('b', 'c'), ('b', 'a'), ('c', 'a')]
| input *iterable* is sorted, the combination tuples will be produced |
|---|
| in sorted order. |
| The combination tuples are emitted in the order of the input |
| *iterable*. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep the second sentence.
@ruaridhw, please address the code review comments. Thank you!
@ruaridhw, thank you for making the change. I apologize for not seeing this before, but I just noticed you made this PR against the 3.8 branch instead of the master branch. Generally, all of our PRs are against master and then we have a bot that will backport changes to other releases. Based on that, please change this to be over the master branch. It's a little more involved than just changing the base branch. Thanks!
- The order of the iterable is not modified within
combinations()as these docs imply - Instead, the order of elements is used as-is
Update Update itertools combinations docscombinations docs
Update itertools [3.9] Update combinations docscombinations docs
[3.9] Update Update combinations docscombinations docs
Attempting to restart the bots by closing and reopening.
Thanks @ruaridhw for the PR, and @rhettinger for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8, 3.9.
🐍🍒⛏🤖
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request
(cherry picked from commit 5e0ed8a)
Co-authored-by: Ruaridh Williamson ruaridhw@users.noreply.github.com
rhettinger pushed a commit that referenced this pull request
rhettinger pushed a commit that referenced this pull request
CuriousLearner added a commit to CuriousLearner/cpython that referenced this pull request
- 'master' of github.com:python/cpython: (497 commits) bpo-40061: Fix a possible refleak in _asynciomodule.c (pythonGH-19748) bpo-40798: Generate a different message for already removed elements (pythonGH-20483) closes bpo-29017: Update the bindings for Qt information with PySide2 (pythonGH-20149) bpo-39885: Make IDLE context menu cut and copy work again (pythonGH-18951) bpo-29882: Add an efficient popcount method for integers (python#771) Further de-linting of zoneinfo module (python#20499) bpo-40780: Fix failure of _Py_dg_dtoa to remove trailing zeros (pythonGH-20435) Indicate that abs() method accept argument that implement abs(), just like call() method in the docs (pythonGH-20509) bpo-39040: Fix parsing of email mime headers with whitespace between encoded-words. (pythongh-17620) bpo-40784: Fix sqlite3 deterministic test (pythonGH-20448) bpo-30064: Properly skip unstable loop.sock_connect() racing test (pythonGH-20494) Note the output ordering of combinatoric functions (pythonGH-19732) bpo-40474: Updated coverage.yml to better report coverage stats (python#19851) bpo-40806: Clarify that itertools.product immediately consumes its inpt (pythonGH-20492) bpo-1294959: Try to clarify the meaning of platlibdir (pythonGH-20332) bpo-37878: PyThreadState_DeleteCurrent() was not removed (pythonGH-20489) bpo-40777: Initialize PyDateTime_IsoCalendarDateType.tp_base at run-time (pythonGH-20493) bpo-40755: Add missing multiset operations to Counter() (pythonGH-20339) bpo-25920: Remove socket.getaddrinfo() lock on macOS (pythonGH-20177) bpo-40275: Fix test.support.threading_helper (pythonGH-20488) ...
| in sorted order. |
|---|
| The permutation tuples are emitted in lexicographic ordering according to |
| the order of the input *iterable*. So, if the input *iterable* is sorted, |
| the combination tuples will be produced in sorted order. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should "combination tuples" be "permutation tuples" here?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good spot 🤦