added as_tuples as an optional argument to Combinations to facilitate hashing in Combination objects #39633
Conversation
|
Please review and guide me over this @maxale |
|
Documentation preview for this PR (built with commit 836586e; changes) is ready! 🎉 |
As I said, I'm not much familiar with Sage internals and I cannot guide you here. |
|
Hey @fchapoton Please allow workflow run |
|
Hey @fchapoton now I have fixed the docstring lint issue |
|
Hey @fchapoton I have finally fixed the issue in the docstring lint and checked it |
|
Hey @fchapoton there was some problem in the code lint which I have fixed and checked it |
|
I'm sorry, but I don't think this is the right approach. We don't want another class that is almost the same as I think it will be ok to have combinations be tuples, rather than lists, so I think that's probably the change that should be made. However, this change may break some existing code, so I think there will need to be a deprecation period in which lists are the default, but tuples are available, perhaps by setting a keyword argument. |
|
@DaveWitteMorris yeah that will be the most organic way. Thank you for guiding me. I'll do it right away |
|
@DaveWitteMorris and @fchapoton can you please review the latest changes |
|
Hey @fchapoton do I need to make any changes? |
src/sage/combinat/combination.py
Outdated
| @@ -240,7 +256,7 @@ def __eq__(self, other) -> bool: | |||
|
|
|||
| def __ne__(self, other) -> bool: | |||
| """ | |||
| Test for unequality. | |||
| Test for inequality. | |||
There was a problem hiding this comment.
this was failing due to codespell
it suggested to use inequality
src/sage/combinat/combination.py
Outdated
| @cached_method | ||
| def __hash__(self) -> int: | ||
| """ | ||
| Returns the hash of combinations(mset,k) |
There was a problem hiding this comment.
first line of doc should use "Return" (imperative mode)
src/sage/combinat/combination.py
Outdated
| @cached_method | ||
| def __hash__(self) -> int: | ||
| """ | ||
| Returns the hash of combinations(mset) |
There was a problem hiding this comment.
Return, not Returns
and with a final dot
src/sage/combinat/combination.py
Outdated
| @@ -456,7 +501,7 @@ def _iterator(self, items, n): | |||
| [[1, 2, 3], [1, 2, 4], [1, 3, 4], [2, 3, 4]] | |||
| """ | |||
| for combination in itertools.combinations(items, n): | |||
| yield list(combination) | |||
| yield combination if self.as_tuples else list(combination) | |||
There was a problem hiding this comment.
put the if outside of the loop
There was a problem hiding this comment.
if self.as_tuples:
for combination in itertools.combinations(items, n):
yield combination
else:
for combination in itertools.combinations(items, n):
yield list(combination)
@fchapoton is this fine?
There was a problem hiding this comment.
@fchapoton please review latest changes and allow workflow run
Also meson conda build for python 3.11.x was failing
is it a necessary check
if so how can I fix it
src/sage/combinat/combination.py
Outdated
| """ | ||
| Return the combinatorial class of combinations of the multiset | ||
| ``mset``. If ``k`` is specified, then it returns the combinatorial | ||
| class of combinations of ``mset`` of size ``k``. | ||
|
|
||
| Return the Combination as tuples instead of lists if as_tuples |
There was a problem hiding this comment.
| Return the Combination as tuples instead of lists if as_tuples | |
| This returns the Combination as tuple instead of list if ``as_tuples`` |
src/sage/combinat/combination.py
Outdated
| sage: hash(Combinations([1,2,1],2)) == hash(c) | ||
| True | ||
| """ | ||
| return hash(str(self.__repr__())) |
There was a problem hiding this comment.
this looks wrong. maybe hash(tuple(self)) ?
There was a problem hiding this comment.
Yeah you are right
There was a problem hiding this comment.
@fchapoton Please review the latest changes as you had suggested and update me about some other changes if required
|
@fchapoton what can possibly be the problem with the check |
|
@DaveWitteMorris @fchapoton |
|
exit code 64 usually means there is some issue with build path (probably in docker) |
|
@fchapoton Please review this issue |
|
@DaveWitteMorris what is the problem |
|
@DaveWitteMorris I have managed to rebuild sage |
|
Can someone please tell me what has to be done for this code to be merged |
|
Please be patient. Very patient. Everybody here has many other duties. |
Understood. Just wanted to check if there are any specific blockers I should address. |
c374166 to
e87423b
Compare
src/sage/combinat/combination.py
Outdated
| The boolean keyword ``as_tuples`` determines whether each combination | ||
| is represented as a tuple or as a list. |
There was a problem hiding this comment.
| The boolean keyword ``as_tuples`` determines whether each combination | |
| is represented as a tuple or as a list. | |
| The boolean keyword ``as_tuples`` (default: ``False``) determines whether | |
| each combination is represented as a tuple or as a list. |
src/sage/combinat/combination.py
Outdated
| for combination in itertools.combinations(items, n): | ||
| yield combination |
There was a problem hiding this comment.
| for combination in itertools.combinations(items, n): | |
| yield combination | |
| yield from itertools.combinations(items, n) |
There was a problem hiding this comment.
Actually, when self.as_tuples is True, can you just return itertools.combinations(items, n)?
|
Thanks for making the changes. I think it is good now. |
sagemathgh-39633: added as_tuples as an optional argument to Combinations to facilitate hashing in Combination objects The as_tuples argument helps in achieving hashing of Combinations objects by producing elements of type tuple This refers to issue [39455](sagemath#39455) <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [x] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#39633 Reported by: Devansh Srivastava Reviewer(s): DaveWitteMorris, Devansh Srivastava, Frédéric Chapoton
sagemathgh-39633: added as_tuples as an optional argument to Combinations to facilitate hashing in Combination objects The as_tuples argument helps in achieving hashing of Combinations objects by producing elements of type tuple This refers to issue [39455](sagemath#39455) <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [x] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#39633 Reported by: Devansh Srivastava Reviewer(s): DaveWitteMorris, Devansh Srivastava, Frédéric Chapoton
sagemathgh-39633: added as_tuples as an optional argument to Combinations to facilitate hashing in Combination objects The as_tuples argument helps in achieving hashing of Combinations objects by producing elements of type tuple This refers to issue [39455](sagemath#39455) <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [x] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#39633 Reported by: Devansh Srivastava Reviewer(s): DaveWitteMorris, Devansh Srivastava, Frédéric Chapoton
The as_tuples argument helps in achieving hashing of Combinations objects by producing elements of type tuple
This refers to issue 39455
📝 Checklist
⌛ Dependencies