Add itertool.combinations.__reduce__ method#3931
Conversation
seryoungshim17
commented
Jul 19, 2022
|
Looks good! But, did you check how 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 |
vm/src/stdlib/itertools.rs
Outdated
| impl PyItertoolsCombinations {} | ||
| impl PyItertoolsCombinations { | ||
| #[pymethod(magic)] | ||
| fn reduce(zelf: PyRef<Self>, vm: &VirtualMachine) -> (PyTypeRef, (PyTupleRef, PyIntRef)) { |
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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?
|
|
Yes. the tests unit are sometime too big. I think checking a few lines of them is fixed would be enough, practically. |
|
@fanninpm could you review this PR again? |
fanninpm
left a comment
There was a problem hiding this comment.
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?
|
@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. |
|
221~224 line in |
|
Could you please rebase this branch on a fresh copy of |
|
@seryoungshim17 Are you still interested in pursuing this? |
Add result in struct PyItertoolsCombinations
5758146 to
5608808
Compare