Skip to content

Conversation

@Akuli
Copy link
Contributor

@Akuli Akuli commented Feb 16, 2025

Docs-only change. Simplifies math.sumprod documentation so that it doesn't assume the reader knows about operator.mul, making it clear what kind of comprehensions can be replaced with math.sumprod().


📚 Documentation preview 📚: https://cpython-previews--130206.org.readthedocs.build/

Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

I'm neutral on this change but I think it's fine to keep it as it can be used without any import.

@skirpichev
Copy link
Contributor

I don't think it's a good idea.

Example in the current main is clear and short. The stdlib here is to be used, if people don't know about operator.mul - it's a good opportunity to learn.

@picnixz
Copy link
Member

picnixz commented Feb 17, 2025

@AlexWaygood Since you reacted with 👍, what do you think?

@python/editorial-board What would be the recommendation in general for such snippets? do we want to introduce more notion in a "roughly equivalent" snippet or should such snippets be as pure as possible (and copy-pastable directly?)

@gvanrossum
Copy link
Member

gvanrossum commented Feb 17, 2025 via email

@picnixz
Copy link
Member

picnixz commented Feb 17, 2025

Honestly I don’t think this is a good question for the EB, each case is
likely different.

Fair enough. Sorry for the ping!


For me, I'm fine to merge this change so that one can just C/C the code without the needs to import operator, but I'm also fine with keeping the operator since it gives the possibility of knowing about operator.mul and map(strict=True) (this one is for me).

So, while I approved the PR, I'll wait for Alex's opinion as well.

Copy link
Contributor

@willingc willingc left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @Akuli and the reviews @picnixz @AlexWaygood @skirpichev.

Roughly equivalent to::

sum(map(operator.mul, p, q, strict=True))
sum(px * qx for px, qx in zip(p, q, strict=True))
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest leaving as is but adding a sentence below such as:

sum(map(operator.mul, p, q, strict=True))

where the multiply operator is used to multiply each item in the *p* and *q* iterables together 
and sum the products.

I'm not sure but I suspect the original command is more performant. I'm fine also with adding the proposed item as an alternative illustration.

I would leave the .c file unchanged using map and operator.mul.

Copy link
Contributor

Choose a reason for hiding this comment

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

ISTM the proposed extra sentence just repeats the first sentence before the rough equivalent.

@rhettinger
Copy link
Contributor

I'm disinclined to accept this edit because it doesn't look nice. It adds tuple unpacking, introduces two new variables, and forces the reader to relate the px and py back to the inputs p and q. None of that is hard, but then the original wasn't hard either. Also, the original lived for a long time in the itertools docs and no one ever reported difficulty understanding it. So, I don't think there is a real problem to be solved here.

Also, I like the current version because the words in the rough equivalent match the words in the preceding text:

sum()            Return the sum of values 
mul()            of products
map(..., p, q)   from two iterables *p* and *q*.    

Also ISTM that the function being described is simple to grasp. When people see sumprod() in Excel, the name itself suffices for an explanation. If this were a more challenging, we could break it down further:

def sumprod(p, q):
    total = 0
    for x, y in zip(p, q, strict=True):
        total += x * y
    return total

But I think that would insult the intelligence of our readers and not raise them up from where they began.

Looking back at the thread, no one really supports this edit. skirpichev doesn't think it is a good idea. carol says to leave as-is will possible further elaboration. picnixz is neutral. And I'm against it as well. So marking as closed and thanking everyone for looking at this.

@rhettinger rhettinger closed this Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants