FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add itertool.combinations.__reduce__ method by seryoungshim17 · Pull Request #3931 · RustPython/RustPython · GitHub

  1. RustPython
  2. RustPython

Repository navigation

Add itertool.combinations.__reduce__ method - #3931

Merged
youknowone merged 1 commit into
RustPython:mainfrom
seryoungshim17:itertool_reduce
Nov 18, 2022
ConversationCommits (1)ChecksFiles changed
Merged

Add itertool.combinations.__reduce__ method#3931
youknowone merged 1 commit into
RustPython:mainfrom
seryoungshim17:itertool_reduce

Conversation

seryoungshim17 commented Jul 19, 2022

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

Snowapril mentioned this pull request Jul 19, 2022
Closed
22 of 27 tasks

Snowapril commented Jul 19, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

fanninpm Jul 19, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 added the z-ca-2022 Tag to track contrubution-academy 2022 label Jul 23, 2022
youknowone requested a review from fanninpm July 24, 2022 15:41
youknowone requested changes Jul 24, 2022
View reviewed changes

youknowone left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

Comment thread vm/src/stdlib/itertools.rs Outdated
Comment thread vm/src/stdlib/itertools.rs Outdated
seryoungshim17 marked this pull request as draft July 29, 2022 12:40

fanninpm commented Jul 29, 2022

Copy link
Copy Markdown
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 commented Jul 30, 2022

Copy link
Copy Markdown
Member

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

youknowone commented Jul 31, 2022

Copy link
Copy Markdown
Member

@fanninpm could you review this PR again?

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

fanninpm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Aug 1, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

@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 commented Aug 2, 2022

Copy link
Copy Markdown
Contributor Author

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

fanninpm commented Aug 22, 2022

Copy link
Copy Markdown
Contributor

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

fanninpm mentioned this pull request Nov 9, 2022
Merged

fanninpm commented Nov 9, 2022

Copy link
Copy Markdown
Contributor

@seryoungshim17 Are you still interested in pursuing this?

Add itertool.combinations.__reduce__ method
5608808 
Add result in struct PyItertoolsCombinations
youknowone merged commit 5608808 into RustPython:main Nov 18, 2022
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

fanninpm fanninpm left review comments
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

Back | FazBrowse Home | New Git URL