classification
Title: Split the math module into _math (C) + math (py)
Type: enhancement Stage: patch review
Components: Versions: Python 3.5
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: NeilGirdhar, belopolsky, brett.cannon, ethan.furman, ezio.melotti, gvanrossum, mark.dickinson, serhiy.storchaka, vstinner
Priority: normal Keywords: patch

Created on 2015-03-05 22:57 by vstinner, last changed 2015-03-10 16:37 by vstinner. This issue is now closed.

Files
File name Uploaded Description Edit
math.patch vstinner, 2015-03-05 22:57 review
math-isclose.patch belopolsky, 2015-03-06 05:02
Messages (16)
msg237300 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-05 22:57
To implement the PEP 485 (math.isclose), I proposed to split the math module into two parts: _math (C) and math (py). The current C module is renamed to _math and a new "math" module implementd in Python is added. The math module contains "from _math import *".

Attached patch implements this idea.

math.py contains the Python implementation of the following functions:

- degrees
- fsum
- factorial
- radians

I propose to add these functions to "document" the implementation of these functions, I don't want to use the Python implementation by default.

Maybe _math.degrees() and _math.radians() can be removed, the Python implementation is enough.

I'm not sure that the C implementation of factorial() is much faster, since most part of the C code calls Python functions (boxing/unboxing). But it's probably better to keep the C implementation :-)

I don't understand how _factorial_partial_product() decide to divide the computation using the number of bits. Does it make sense to compute the number of bits in Python, since the Python int type has no limit? (only memory)

The C and Python implementations are tested by test_math.

math.py cannot be used alone: it requires floor(), isfinite(), isinf() and isnan() functions of the _math module. The "try: import _math except ImportError: pass" is only useful for unit tests.

TODO: remove SIZEOF_LONG from math.py.

We may rename Module/mathmodule.c to Module/_mathmodule.c, but it's not convinient to generate a patch if a file is renamed.
msg237301 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-05 22:59
I didn't benchmark the overhead of the patch on "import math" (Python startup time)
msg237302 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-05 23:03
We may add tests on fsum() with nan:

assert fsum([nan, nan, nan]) == nan
assert fsum([nan, 1.0, inf]) == nan
assert fsum([nan, 1.0, -inf]) == nan
msg237305 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2015-03-05 23:06
I have no problem with having Python versions, but we should not remove anything from the C implementation -- at a minimum they should be good for testing against.
msg237327 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2015-03-06 04:08
Didn't Guido veto the idea of math.py?

In my opinion, isclose() is way too simple to justify refactoring.

If math.py is ever deemed to be a good idea, it should be vetted and implemented on its own merits on not use PEP 485 as a backdoor.
msg237329 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2015-03-06 05:02
I am attaching a quick implementation of math.isclose in C.  It may not cover all corner cases correctly, but that should be easy to fix.
msg237330 - (view) Author: Neil Girdhar (NeilGirdhar) * Date: 2015-03-06 05:21
Nice work Alexander.  I believe what's left is for it to work with complex numbers.
msg237331 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2015-03-06 06:19
If there is a complex number version it will live in cmath, not math.
msg237332 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-06 06:52
@Alex: you should open a new issue for isclose.
msg237333 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2015-03-06 08:22
> I'm not sure that the C implementation of factorial() is much faster

The inner loop (see the for loop in factorial_partial_product) operates on C unsigned longs, so I'd be surprised if a Python implementation were competitive.  But it would be interesting to see timings.
msg237334 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2015-03-06 08:34
> Didn't Guido veto the idea of math.py?

It looks like it: https://mail.python.org/pipermail/python-dev/2015-March/138616.html
msg237336 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-06 08:43
I experimented with different Python implementations of factorial() in 2.5/2.6. But then came 2.7 with C implementation, and it beat them all. Python is not the best language to implement low-level computational algorithms.
msg237342 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-06 11:17
> > Didn't Guido veto the idea of math.py?
> It looks like it: https://mail.python.org/pipermail/python-dev/2015-March/138616.html

Ah. I missed this email.

So what do you think of my patch? Does it make sense to provide math.py?

--

For math.py without _math: isnan(), isfinite(), etc. can be implemented in pure Python using the struct module. See for example the old fpconst module:
https://pypi.python.org/pypi/fpconst

Well, it would be specific to IEEE 754, but most platforms support IEEE 754, and other Python implementations can implement their own _math module to support more platforms (like platforms not supporting IEEE 754 floats).
msg237361 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2015-03-06 15:48
I interpret Guido's email as vetoing the skipping of the C implementation of PEP 485, not on outright banning a PEP 399 math.py if someone like Victor wanted to put the work into implementing some things on top of the C code in Python. So as long as everything in the math module has a C equivalent I say implement whatever you want in Python code as long as maintenance won't be a burden.
msg237363 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2015-03-06 15:52
I think it's pretty silly to have math.py. And why would there be a
pure-Python implementation of factorial() (like anybody is ever going to
use that) instead of example implementations of sin() etc.? Please don't go
down this path for this particular module. Put your code in a recipe or
something.

On Fri, Mar 6, 2015 at 7:48 AM, Brett Cannon <report@bugs.python.org> wrote:

>
> Brett Cannon added the comment:
>
> I interpret Guido's email as vetoing the skipping of the C implementation
> of PEP 485, not on outright banning a PEP 399 math.py if someone like
> Victor wanted to put the work into implementing some things on top of the C
> code in Python. So as long as everything in the math module has a C
> equivalent I say implement whatever you want in Python code as long as
> maintenance won't be a burden.
>
> ----------
> nosy: +brett.cannon
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue23595>
> _______________________________________
>
msg237778 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-10 16:37
Ok, fair enough, I abandon my change.

I agree that a partial implementation of math.py (only a few functions) is not useful. To implement more math functions, struct and ctypes modules are probably needed. But it sounds overkill, since CPython already has a full implementation of math written in C.

It's quite easy to implement the math module for other Python implementations (Jython, IronPython, PyPy, etc.). So I agree that math.py is useless. We all want a super fast (optimized) math module ;-)
History
Date User Action Args
2015-03-10 16:37:50vstinnersetstatus: open -> closed
resolution: rejected
messages: + msg237778
2015-03-06 15:52:20gvanrossumsetmessages: + msg237363
2015-03-06 15:48:47brett.cannonsetnosy: + brett.cannon
messages: + msg237361
2015-03-06 11:17:27vstinnersetmessages: + msg237342
2015-03-06 08:43:34serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg237336
2015-03-06 08:34:27mark.dickinsonsetmessages: + msg237334
2015-03-06 08:22:24mark.dickinsonsetmessages: + msg237333
2015-03-06 06:52:48vstinnersetmessages: + msg237332
2015-03-06 06:44:36ezio.melottisetnosy: + ezio.melotti

type: enhancement
stage: patch review
2015-03-06 06:19:33ethan.furmansetmessages: + msg237331
2015-03-06 05:21:08NeilGirdharsetnosy: + NeilGirdhar
messages: + msg237330
2015-03-06 05:02:30belopolskysetfiles: + math-isclose.patch

messages: + msg237329
2015-03-06 04:08:04belopolskysetnosy: + gvanrossum, belopolsky
messages: + msg237327
2015-03-05 23:06:08ethan.furmansetnosy: + ethan.furman
messages: + msg237305
2015-03-05 23:03:44vstinnersetmessages: + msg237302
2015-03-05 22:59:07vstinnersetmessages: + msg237301
2015-03-05 22:57:31vstinnercreate