bpo-35606: Implement math.prod by pablogsal · Pull Request #11359 · python/cpython

pablogsal · 2018-12-29T22:06:48Z

Comparison between `math.prod` and `reduce` + `operator.mul`

❯ ./python ../experiment.py
.....................
math.prod: Mean +- std dev: 79.3 us +- 1.8 us
.....................
reduce+operator.mul: Mean +- std dev: 517 us +- 13 us

https://bugs.python.org/issue35606

Note: This PR is a reference implementation to further discuss the addition of prod to the mathmodule.

Doc/library/math.rst

eamanu · 2018-12-30T00:14:55Z

Doc/library/math.rst

unnecessary line?

Modules/clinic/mathmodule.c.h

eamanu · 2018-12-30T02:01:40Z

Look.

>>> math.prod([3,2,1], [2,1])
[2, 1, 2, 1, 2, 1, 2, 1, 2, 1, 2, 1]

This isn't normal, right?

pablogsal · 2018-12-30T02:14:15Z

@eamanu Is the same result you will get if you use a simple reduce algorithm:

>>> from functools import reduce
>>> reduce(lambda x,y: x * y, [3,2,1], [1,2])
[1, 2, 1, 2, 1, 2, 1, 2, 1, 2, 1, 2]

This raises the question if we should ban certain data types as lists/dicts...etc but that behaviour is totally normal for that input.

eamanu · 2018-12-30T03:40:27Z

@pablogsal If we based on sum(), list/tuples/etc should no be accepted

pablogsal · 2018-12-30T04:09:01Z

@eamanu Why not? This is perfectly valid and an analogous is in the test suite:

>>> sum([[1,2,3,4]], [1,2,3,3,4])
[1, 2, 3, 3, 4, 1, 2, 3, 4]

We can decide to limit the API to not accept list/tuples/etc but people can get away and subclass them or implement their own containers. I think the text in the doc and docstring is enough.

eamanu · 2018-12-30T05:34:25Z

I think the text in the doc and docstring is enough.

Ok, yes, you are right.

A test with the form math.prod([3,2,1], [2,1]) can be added?

eamanu

LGTM

Modules/mathmodule.c

rhettinger · 2019-02-05T21:32:08Z

Modules/mathmodule.c

Support for these macros was dropped in 3.7, so you can leave them out. We still define the macros to avoid breaking pre-existing third-party code but the macros don't do anything, they are empty.

bedevere-bot · 2019-02-05T21:32:46Z

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

rhettinger · 2019-02-05T21:33:23Z

Please add an entry to whatsnew/3.8.rst. Otherwise, this all looks good :-)

rhettinger · 2019-02-07T05:33:44Z

Can you please fix-up the CI bot failures. Likely, it wants the trailing whitespace cleaned-up.

s-cork · 2021-09-23T06:44:11Z

Lib/test/test_math.py

+        self.assertRaises(TypeError, prod, ['a', 'b', 'c'], '')
+        self.assertRaises(TypeError, prod, [b'a', b'c'], b'')
+        values = [bytearray(b'a'), bytearray(b'b')]
+        self.assertRaises(TypeError, prod, values, bytearray(b''))
+        self.assertRaises(TypeError, prod, [[1], [2], [3]])
+        self.assertRaises(TypeError, prod, [{2:3}])
+        self.assertRaises(TypeError, prod, [{2:3}]*2, {2:3})
+        self.assertRaises(TypeError, prod, [[1], [2], [3]], [])
+        with self.assertRaises(TypeError):
+            prod([10, 20], [30, 40])     # start is a keyword-only argument


@rhettinger / @pablogsal - Skulpt committer here - implementing these tests in our own project.

I was wondering if the intention wasn't quite right with some of these TypeError tests?
Most of them fail because start is a keyword only argument.
Whereas it looks like - for most of them except the final test - the intention is to check multiplication across incompatible types within the prod function.

serhiy-storchaka · 2018-12-30T20:27:20Z

Modules/mathmodule.c

You can use _PyLong_One.

serhiy-storchaka · 2021-09-28T08:27:34Z

Modules/mathmodule.c

+        }
+        /* Loop over all the items in the iterable until we finish, we overflow
+         * or we found a non integer element */
+        while(result == NULL) {


Add a space between while and (.

serhiy-storchaka · 2021-09-28T08:32:15Z

Modules/mathmodule.c

+            }
+            if (PyLong_CheckExact(item)) {
+                long b = PyLong_AsLongAndOverflow(item, &overflow);
+                long x = i_result * b;


Signed integer overflow is an undefined behavior.

serhiy-storchaka · 2021-09-28T08:34:22Z

Modules/mathmodule.c

+                /* Continue if there is no overflow */
+                if (overflow == 0
+                    && x < INT_MAX && x > INT_MIN
+                    && !(b != 0 && x / i_result != b)) {


Possible division by 0.

serhiy-storchaka · 2021-09-28T08:38:34Z

Modules/mathmodule.c

+        double f_result = PyFloat_AS_DOUBLE(result);
+        Py_DECREF(result);
+        result = NULL;
+        while(result == NULL) {


Add a space between while and (.

pablogsal · 2021-09-28T09:25:00Z

@serhiy-storchaka Thanks for the review. This PR is already merged so I can do a separate PR if you wish

serhiy-storchaka · 2021-09-28T09:42:40Z

Please address @s-cork comment. Tests do not work as intended.

serhiy-storchaka · 2021-09-28T09:47:31Z

Ah, I started reviewing after seen @s-cork comment, without noticing that this PR was merged years ago.

pablogsal · 2021-09-28T10:12:39Z

Yeah, most of these things are already fixed, although is possible that some of these still remain. I will do a check.

pablogsal self-assigned this Dec 29, 2018

pablogsal requested a review from rhettinger December 29, 2018 22:06

bedevere-bot added the awaiting merge label Dec 29, 2018

the-knights-who-say-ni added the CLA signed label Dec 29, 2018

pablogsal changed the title ~~bpo-bpo35606: Implement math.prod~~ bpo-35606: Implement math.prod Dec 29, 2018

eamanu reviewed Dec 30, 2018

View reviewed changes

eamanu approved these changes Dec 30, 2018

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting merge labels Dec 30, 2018

rhettinger self-assigned this Dec 30, 2018

pganssle reviewed Jan 2, 2019

View reviewed changes

Modules/mathmodule.c Outdated Show resolved Hide resolved

rhettinger requested changes Feb 5, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting core review labels Feb 5, 2019

pablogsal force-pushed the bpo35606 branch 3 times, most recently from a97b7f6 to a9ffb24 Compare February 7, 2019 01:18

Implement math.prod

6a3b62f

pablogsal force-pushed the bpo35606 branch from a9ffb24 to 6a3b62f Compare February 7, 2019 01:21

rhettinger approved these changes Feb 7, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Feb 7, 2019

rhettinger added 2 commits February 6, 2019 21:50

Clean-up trailing whitespace

7faed4c

Fix markup

61d8b6b

rhettinger merged commit bc09851 into python:master Feb 7, 2019

bedevere-bot removed the awaiting merge label Feb 7, 2019

pablogsal deleted the bpo35606 branch February 7, 2019 09:24

s-cork reviewed Sep 23, 2021

View reviewed changes

serhiy-storchaka reviewed Sep 28, 2021

View reviewed changes

Modules/mathmodule.c Outdated

Copy link

Member

serhiy-storchaka Dec 30, 2018

Choose a reason for hiding this comment

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

You can use _PyLong_One.

serhiy-storchaka reviewed Sep 28, 2021

View reviewed changes

Uh oh!

Conversation

pablogsal commented Dec 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comparison between math.prod and reduce + operator.mul

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eamanu commented Dec 30, 2018

Uh oh!

pablogsal commented Dec 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eamanu commented Dec 30, 2018

Uh oh!

pablogsal commented Dec 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eamanu commented Dec 30, 2018

Uh oh!

eamanu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Feb 5, 2019

Uh oh!

rhettinger commented Feb 5, 2019

Uh oh!

rhettinger commented Feb 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

s-cork Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablogsal commented Sep 28, 2021

Uh oh!

serhiy-storchaka commented Sep 28, 2021

Uh oh!

serhiy-storchaka commented Sep 28, 2021

Uh oh!

pablogsal commented Sep 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

pablogsal commented Dec 29, 2018 •

edited

Loading

Comparison between `math.prod` and `reduce` + `operator.mul`

pablogsal commented Dec 30, 2018 •

edited

Loading

pablogsal commented Dec 30, 2018 •

edited

Loading

rhettinger commented Feb 7, 2019 •

edited

Loading

s-cork Sep 23, 2021 •

edited

Loading