Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-118179: Added implementation for nwise to itertools #118162

Closed
wants to merge 2 commits into from

Conversation

lsproule
Copy link

@lsproule lsproule commented Apr 22, 2024

Copy link

cpython-cla-bot bot commented Apr 22, 2024

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app
Copy link

bedevere-app bot commented Apr 22, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@lsproule
Copy link
Author

lsproule commented Apr 22, 2024

I am starting this pull request to show my implementation, I am looking for feedback and help to follow the procedures. Thank you this is my first contribution to python.
@rhettinger

@eendebakpt
Copy link
Contributor

@lsproule Thanks contributing! You can find more information about the cpython procedures in the developers guide: https://devguide.python.org/. In particular, for this PR you need to create an issue this PR can link to. And in general for additions to cpython a good place to discuss new ideas is https://discuss.python.org/c/ideas/6

@eendebakpt
Copy link
Contributor

From the docstring in your PR it looks like the nwise is (roughly) equivelent to the sliding_window method from more-itertools (see https://more-itertools.readthedocs.io/en/stable/api.html#more_itertools.sliding_window)

Is that correct?

@lsproule
Copy link
Author

lsproule commented Apr 22, 2024

@eendebakpt sliding_window doesn't exist in the itertools module. In the docs, they show you how to implement it using the itertools module. However, since they added pairwise. I thought having a C version of nwise or yes a sliding window, would make some sense. It is functionally equivalent to sliding_window in the more_itertools module, but more_itertools is written in python. This would bring it to the standard library and be written in C.

@lsproule lsproule changed the title added implementation for nwise to itertools gh-118179: Added implementation for nwise to itertools Apr 23, 2024
@bedevere-app
Copy link

bedevere-app bot commented Apr 23, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@lsproule
Copy link
Author

so for this clinic input should it be Py_ssize_t? Actually how am I supposed to go about generating the clinic stuff appropriately

https://github.com/lsproule/cpython/blob/874d06821711fe99d8e765d78eca4702ced41c80/Modules/itertoolsmodule.c#L398C1-L409C2

also, can we check my allocations and de-allocations because I want to do this well.

https://github.com/lsproule/cpython/blob/874d06821711fe99d8e765d78eca4702ced41c80/Modules/itertoolsmodule.c#L452C1-L516C2

@lsproule lsproule marked this pull request as draft April 23, 2024 14:06
@rhettinger
Copy link
Contributor

rhettinger commented Apr 23, 2024

We haven't decided to add sliding_window() to itertools, so it is premature to submit a PR to add it. When pairwise() was added, there was a deliberate decision NOT to add the general case. The jury is still out on whether this would be a worthwhile itertool. I'm monitoring how people use the more-itertools package to see how it is used and whether it fits in well with the rest of the module.

@rhettinger rhettinger added type-feature A feature request or enhancement DO-NOT-MERGE labels Apr 23, 2024
@lsproule
Copy link
Author

@lsproule
Copy link
Author

Well, I would like to revisit that. I think more-itertools is great, but I personally would like to have this in the itertools toolbelt especially because we do have pairwise, and it would be nice to revisit. I changed it to a draft instead of a pull request while we discuss it. I want this to be a quality addition. I personally have used it for analyzing data. I think itertools is an amazing module, so not having to reach for a library like more-iterools is nice for me. I like to try to stick to the standard library as much as possible.

@lsproule lsproule closed this Apr 29, 2024
@lsproule lsproule deleted the nwise branch April 29, 2024 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO-NOT-MERGE type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants