Skip to content

bpo-34659: Adds initial kwarg to itertools.accumulate()#9345

Merged
lisroach merged 9 commits into
python:masterfrom
lisroach:issue_34659
Sep 24, 2018
Merged

bpo-34659: Adds initial kwarg to itertools.accumulate()#9345
lisroach merged 9 commits into
python:masterfrom
lisroach:issue_34659

Conversation

@lisroach

@lisroach lisroach commented Sep 16, 2018 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

@rhettinger rhettinger self-assigned this Sep 16, 2018
@rhettinger rhettinger requested a review from tim-one September 17, 2018 03:53
tim-one
tim-one approved these changes Sep 17, 2018
View reviewed changes

@tim-one tim-one 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.

Looks good! Thank you - I'll even use it ;-)

serhiy-storchaka
serhiy-storchaka requested changes Sep 17, 2018
View reviewed changes

@serhiy-storchaka serhiy-storchaka 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.

There are following technical problems with current changes:

  1. The default value for initial is documented and exposed in the signature as None, but passing None is not the same as omitting the initial argument.
  2. Pickling and copying the accumulate iterator loses the initial value.

@bedevere-bot

bedevere-bot commented Sep 17, 2018

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@rhettinger

rhettinger commented Sep 17, 2018

Copy link
Copy Markdown
Contributor

The pickling and copying should be a separate tracker item for Kristjan Jonnson to update his own code (I think he is the only user of that feature and would be willing to shoulder some of the recurring maintenance burden arising from that code).

@serhiy-storchaka

serhiy-storchaka commented Sep 17, 2018

Copy link
Copy Markdown
Member

I don't think that ignoring initial=None is right. And you did not think this too, according to your initial implementation.

@rhettinger

rhettinger commented Sep 17, 2018

Copy link
Copy Markdown
Contributor

It depends on whether you think None would be a useful input value to an accumulation.

The start=_sentinel approach gives more flexibility but is problematic for complicating the pure python version in the docs and for wrestling with argument clinic (I suspect we would have to remove the clinic code entirely and forgo the signature it generates). If so, I'm inclined to leave the PR as is.

@serhiy-storchaka

serhiy-storchaka commented Sep 17, 2018

Copy link
Copy Markdown
Member

This will make a subtle difference between accumulate() and reduce(), and also max(), iter(), next(), getattr(), etc which accept an optional argument, but distinguish None from no argument. I think this will be a bug magnet.

As for the pure python version in the docs, the pure python version of reduce() uses None as a sentinel.

@rhettinger

rhettinger commented Sep 19, 2018

Copy link
Copy Markdown
Contributor

Added pickle support (when initial is set, use chain([initial], it)). Please take a look.

serhiy-storchaka
serhiy-storchaka reviewed Sep 20, 2018
View reviewed changes

@serhiy-storchaka serhiy-storchaka 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.

Please add an entry in What's New.

Comment thread Modules/itertoolsmodule.c Outdated
lz->initial, lz->it);
if (it == NULL)
return NULL;
return Py_BuildValue("O(OO)O", Py_TYPE(lz),

@serhiy-storchaka serhiy-storchaka Sep 20, 2018

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.

"O(NO)O"

@rhettinger rhettinger Sep 20, 2018

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.

IMO this is to minor to warrant a WhatsNew entry. Whatsnew should be much more selective. The reason we have whatsnew is that NEWS is already unmanageably lengthy to read.

@rhettinger rhettinger Sep 20, 2018

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.

Fixed.

Comment thread Modules/itertoolsmodule.c
if (it == NULL)
return NULL;
return Py_BuildValue("O(OO)O", Py_TYPE(lz),
it, lz->binop?lz->binop:Py_None, Py_None);

@serhiy-storchaka serhiy-storchaka Sep 20, 2018

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.

Please add spaces around ? and : for PEP 7.

@rhettinger rhettinger Sep 20, 2018

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.

It's better without the spaces to show the grouping and for consistency with the surrounding code.

@serhiy-storchaka

serhiy-storchaka commented Sep 21, 2018

Copy link
Copy Markdown
Member

The pickling part LGTM. But I think that initial=None should not be ignored.

@rhettinger

rhettinger commented Sep 21, 2018

Copy link
Copy Markdown
Contributor

Serhiy, thank you for the suggestion about initial=None, but I'm going to decline. I want to keep the argument clinic here and I agree with Tim's post on the subject. As module maintainer, I get to make the final call on the API.

Let's clear the way for Lisa to apply her first patch own her own. Please clear the review request so this can move forward. It has already consumed far too much time in relative to the value of the feature.

@serhiy-storchaka

serhiy-storchaka commented Sep 21, 2018

Copy link
Copy Markdown
Member

This feature is just too complex for its value.

@lisroach lisroach merged commit 9718b59 into python:master Sep 24, 2018
@bbayles bbayles mentioned this pull request Jun 12, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rhettinger rhettinger rhettinger approved these changes

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@tim-one tim-one tim-one approved these changes

Assignees

@rhettinger rhettinger

@lisroach lisroach

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@lisroach @bedevere-bot @rhettinger @serhiy-storchaka @tim-one @the-knights-who-say-ni