Skip to content

Add itertool.combinations.__reduce__ method #3931

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 18, 2022
Merged

Add itertool.combinations.__reduce__ method #3931

merged 1 commit into from
Nov 18, 2022

Conversation

seryoungshim17
Copy link
Contributor 

>>>>> from itertools import combinations
>>>>> c = combinations([1,2,3,4], 2)
>>>>> c.__reduce__()
(<class 'itertools.combinations'>, ((1, 2, 3, 4), 2))

@Snowapril Snowapril mentioned this pull request Jul 19, 2022
Closed
27 tasks
@Snowapril
Copy link
Contributor

Snowapril commented Jul 19, 2022 •
edited
Loading

Looks good! But, did you check how itertools.combinations.__reduce__ work in cpython? They act differently according to several conditions. Please check the below codes.

CPython

>>> a = itertools.combinations(range(4), 3)
>>> a.__reduce__()
(<class 'itertools.combinations'>, ((0, 1, 2, 3), 3))
>>> next(a); a.__reduce__()
(0, 1, 2)
(<class 'itertools.combinations'>, ((0, 1, 2, 3), 3), (0, 1, 2))
>>> next(a); a.__reduce__()
(0, 1, 3)
(<class 'itertools.combinations'>, ((0, 1, 2, 3), 3), (0, 1, 3))
>>> next(a); a.__reduce__()
(0, 2, 3)
(<class 'itertools.combinations'>, ((0, 1, 2, 3), 3), (0, 2, 3))
>>> next(a); a.__reduce__()
(1, 2, 3)
(<class 'itertools.combinations'>, ((0, 1, 2, 3), 3), (1, 2, 3))
>>> next(a)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
StopIteration
>>> a.__reduce__()
(<class 'itertools.combinations'>, ((), 3))

But in current PR always return (<class 'itertools.combinations'>, ((0, 1, 2, 3), 3)) for the above case.
Please check this cpython impl

fanninpm
fanninpm reviewed Jul 19, 2022
View reviewed changes
vm/src/stdlib/itertools.rs Outdated
impl PyItertoolsCombinations {}
impl PyItertoolsCombinations {
#[pymethod(magic)]
fn reduce(zelf: PyRef<Self>, vm: &VirtualMachine) -> (PyTypeRef, (PyTupleRef, PyIntRef)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may want to change this function's signature so that the function returns PyTupleRef, a more flexible return type. Something like…

fn reduce(zelf: PyRef<Self>, vm: &VirtualMachine) -> PyTupleRef {
    if zelf.result.load().is_none() {
        return vm.new_tuple((IntoPyTuple::into_pytuple(zelf.pool.clone(), vm), vm.ctx.new_int(zelf.r.load())));
    } else if zelf.exhausted.load() {
        return vm.new_tuple(((), vm.ctx.new_int(zelf.r.load())));
    } else {
        ...
        return vm.new_tuple((IntoPyTuple::into_pytuple(zelf.pool.clone(), vm), vm.ctx.new_int(zelf.r.load()), ...));
    }
}

@youknowone youknowone added the z-ca-2022 Tag to track contrubution-academy 2022 label Jul 23, 2022
@youknowone youknowone requested a review from fanninpm July 24, 2022 15:41
youknowone
youknowone requested changes Jul 24, 2022
View reviewed changes
Copy link
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to look in details but it seems next algorithm is changed. did it have a bug?
I also want to know if this is actually fixing reduce test cases. Which test in test_itertools is related and how much is it fixed?

@seryoungshim17 seryoungshim17 marked this pull request as draft July 29, 2022 12:40
@fanninpm
Copy link
Contributor

Which test in test_itertools is related and how much is it fixed?

TestBasicOps.test_combinations is the test in question. (Unfortunately, I think the test functions in test_itertools.py are a bit too over-broad in scope. Ideally, a test function should be as minimal as possible.)

@youknowone
Copy link
Member

Yes. the tests unit are sometime too big. I think checking a few lines of them is fixed would be enough, practically.

@youknowone
Copy link
Member

@fanninpm could you review this PR again?

@youknowone youknowone self-requested a review July 31, 2022 22:46
fanninpm
fanninpm reviewed Aug 1, 2022
View reviewed changes
Copy link
Contributor

@fanninpm fanninpm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In lieu of removing a @unittest.expectedFailure decorator from the CPython test suite, can you add something to the extra_tests directory to ensure that this change matches CPython's behavior?

@youknowone
Copy link
Member

youknowone commented Aug 1, 2022 •
edited
Loading

@fanninpm If the test cases are included in unittest as part of TestBasicOps.test_combinations, I don't think that's necessary. The test will eventually pass and I don't want to make contributors pay too much cost for temporal test quality.

@seryoungshim17
Copy link
Contributor Author

221~224 line in TestBasicOps.test_combinations has been fixed.

@fanninpm
Copy link
Contributor

Could you please rebase this branch on a fresh copy of main?

@fanninpm fanninpm mentioned this pull request Nov 9, 2022
Merged
@fanninpm
Copy link
Contributor

fanninpm commented Nov 9, 2022

@seryoungshim17 Are you still interested in pursuing this?

@seryoungshim17 @youknowone
Add itertool.combinations.__reduce__ method
5608808 
Add result in struct PyItertoolsCombinations
@youknowone youknowone merged commit 5608808 into RustPython:main Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fanninpm fanninpm fanninpm left review comments

@youknowone youknowone Awaiting requested review from youknowone

Assignees
No one assigned
Labels
z-ca-2022 Tag to track contrubution-academy 2022
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@seryoungshim17 @Snowapril @fanninpm @youknowone