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

Refactor math module to use C99+ special functions (erf, erfc, etc) #101678

Closed
skirpichev opened this issue Feb 8, 2023 · 20 comments
Closed

Refactor math module to use C99+ special functions (erf, erfc, etc) #101678

skirpichev opened this issue Feb 8, 2023 · 20 comments
Labels
extension-modules C modules in the Modules dir type-feature A feature request or enhancement

Comments

@skirpichev
Copy link
Member

skirpichev commented Feb 8, 2023

The PEP 7 documents, that we must use C11 (without optional features) since CPython 3.11. erf, erfc, lgamma and tgamma (math.gamma) are part of the C99 (and mandatory for C11 too).

Probably, it's not a bad idea to get rid of custom implementations of such functions or document why it's present (e.g. for some exotic platform, broken libc implementation, speed optimization, etc). Keep in mind, that this code isn't tested by CI if we optionally include using standard implementations (as for erf with #ifdef HAVE_ERF).

Linked PRs

@skirpichev skirpichev added the type-feature A feature request or enhancement label Feb 8, 2023
skirpichev added a commit to skirpichev/cpython that referenced this issue Feb 8, 2023
@arhadthedev arhadthedev added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Feb 8, 2023
@skirpichev
Copy link
Member Author

(@arhadthedev, I think it's more related to the stdlib.)

@arhadthedev
Copy link
Member

Should work because:

@arhadthedev arhadthedev added extension-modules C modules in the Modules dir and removed interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Feb 8, 2023
@arhadthedev
Copy link
Member

I think it's more related to the stdlib.

Tags are about affected directories, not user-visible effect. From descriptions of the labels:

  • extension-modules - C modules in the Modules dir
  • interpreter-core - Objects, Python, Grammar, and Parser dirs
  • stdlib Python modules in the Lib dir

@arhadthedev
Copy link
Member

cc @vstinner

@mdickinson
Copy link
Member

We've tried this before. The key issue is not availability, but quality. Both when I originally implemented these functions, and when we last tried using the libm versions, the libm versions turned out to be of unacceptably poor quality (cases where the computed results were many millions of ulps out from the correct results).

If you do this, please augment the tests to do very thorough testing of the relevant functions, and then run on all the buildbots. But really, I'd strongly recommend leaving this alone - there's no issue being fixed here. We're only increasing the uncertainty over the quality of the functions.

@mdickinson
Copy link
Member

See #70309 for the previous attempt.

@mdickinson
Copy link
Member

Probably, it's not a bad idea [...]

The rationale for making changes to stable, working code needs to be a lot stronger than this.

@skirpichev
Copy link
Member Author

the libm versions turned out to be of unacceptably poor quality

Isn't that the case where fixes should be submitted to upstream? Or libm implementations will be of poor quality forever...

there's no issue being fixed here

At least, there is no regular testing.

@arhadthedev
Copy link
Member

For history, here is an example from the failed macOS run:

======================================================================
FAIL: test_mtestfile (test.test_math.MathTests.test_mtestfile)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/runner/work/cpython/cpython/Lib/test/test_math.py", line 2053, in test_mtestfile
    self.fail('Failures in test_mtestfile:\n  ' +
AssertionError: Failures in test_mtestfile:
  lgam0080: lgamma(-0.9999999999999999): expected 36.7368005696771, got 36.25168935623487 (error = 0.485 (6[827](https://github.com/python/cpython/actions/runs/4121854833/jobs/7117957687#step:7:828)3333752865 ulps); permitted error = 1e-15 or 5 ulps)
  lgam0082: lgamma(-1.9999999999999998): expected 35.35050620855721, got 34.9797722396[830](https://github.com/python/cpython/actions/runs/4121854833/jobs/7117957687#step:7:831)3 (error = 0.371 (52176167627355 ulps); permitted error = 1e-15 or 5 ulps)
  lgam0085: lgamma(-99.99999999999999): expected -331.85460524980596, got -331.86198661322277 (error = 0.00738 (129854318490 ulps); permitted error = 1e-15 or 5 ulps)
  lgam0106: lgamma(2.5599[833](https://github.com/python/cpython/actions/runs/4121854833/jobs/7117957687#step:7:834)2785163e+305): expected 1.79769313486231e+308, got inf (not equal)
  lgam0107: lgamma(2.55998332785164e+305): expected 'OverflowError', got inf (not equal)
  lgam0108: lgamma(1.7e+308): expected 'OverflowError', got inf (not equal)
  gam0048: gamma(5.5e-309): expected 'OverflowError', got inf (not equal)
  gam0049: gamma(1e-309): expected 'OverflowError', got inf (not equal)
  gam0050: gamma(1e-323): expected 'OverflowError', got inf (not equal)
  gam0051: gamma(5e-324): expected 'OverflowError', got inf (not equal)
  gam0068: gamma(-5.5e-309): expected 'OverflowError', got -inf (not equal)
  gam0069: gamma(-1e-309): expected 'OverflowError', got -inf (not equal)
  gam0070: gamma(-1e-323): expected 'OverflowError', got -inf (not equal)
  gam0071: gamma(-5e-324): expected 'OverflowError', got -inf (not equal)
  gam0103: gamma(171.625): expected 'OverflowError', got inf (not equal)
  gam0104: gamma(172.0): expected 'OverflowError', got inf (not equal)
  gam0105: gamma(2000.0): expected 'OverflowError', got inf (not equal)
  gam0106: gamma(1.7e+308): expected 'OverflowError', got inf (not equal)
  gam0122: gamma(-170.5): expected -3.3127395215386074e-308, got -3.312739521538751e-308 (error = 1.44e-321 (291 ulps); permitted error = 0 or 20 ulps)

@arhadthedev
Copy link
Member

For Windows (x64):

======================================================================
FAIL: test_mtestfile (test.test_math.MathTests.test_mtestfile)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\a\cpython\cpython\Lib\test\test_math.py", line 2053, in test_mtestfile
    self.fail('Failures in test_mtestfile:\n  ' +
AssertionError: Failures in test_mtestfile:
  lgam0085: lgamma(-99.99999999999999): expected -331.85460524980596, got -331.8619866132228 (error = 0.00738 (129854318491 ulps); permitted error = 1e-15 or 5 ulps)
  gam0085: gamma(-99.99999999999999): expected 7.540083334884096e-145, got 7.484632144060727e-145 (error = 5.55e-147 (38979707393610 ulps); permitted error = 0 or 20 ulps)
  gam0124: gamma(-176.5): expected -1.196e-321, got 0.0 (error = 1.2e-321 (243 ulps); permitted error = 0 or 20 ulps)

For Windows (x86):

======================================================================
FAIL: test_mtestfile (test.test_math.MathTests.test_mtestfile)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\a\cpython\cpython\Lib\test\test_math.py", line 2053, in test_mtestfile
    self.fail('Failures in test_mtestfile:\n  ' +
AssertionError: Failures in test_mtestfile:
  lgam0085: lgamma(-99.99999999999999): expected -331.85460524980596, got -331.8619866132228 (error = 0.00738 (129854318491 ulps); permitted error = 1e-15 or 5 ulps)
  gam0085: gamma(-99.99999999999999): expected 7.540083334884096e-145, got 7.484632144060727e-145 (error = 5.55e-147 (389797073[936](https://github.com/python/cpython/actions/runs/4121854833/jobs/7117957822#step:5:936)10 ulps); permitted error = 0 or 20 ulps)
  gam0124: gamma(-176.5): expected -1.196e-321, got 0.0 (error = 1.2e-321 (243 ulps); permitted error = 0 or 20 ulps)
----------------------------------------------------------------------

@skirpichev
Copy link
Member Author

Yes, that's from that 5+ old PR, right? But not for erf/erfc.

@arhadthedev
Copy link
Member

arhadthedev commented Feb 8, 2023

Unfortunately, that's from your PR: https://github.com/python/cpython/actions/runs/4121854833. It seems that C standard library implementations didn't get better.

However, erf really has no flaws here.

@skirpichev
Copy link
Member Author

Indeed. Could we test the first commit only too?

@mdickinson
Copy link
Member

Isn't that the case where fixes should be submitted to upstream?

Good luck with that. Upstream often isn't open source. I've certainly made bug reports on these topics upstream in the past, with typically zero response.

@mdickinson
Copy link
Member

At least, there is no regular testing.

True - for erf and erfc, we're never not going to be using the libm versions on current platforms, and it seems that using the libm versions hasn't caused error reports in recent years. It does seem reasonable to drop the custom implementations there.

For gamma and lgamma, let's add a note to the source (possibly referencing the relevant GitHub issues / PRs) that explains why we're using a custom implementation.

@skirpichev
Copy link
Member Author

True - for erf and erfc

It seems, also for log2.

@mdickinson
Copy link
Member

It seems, also for log2.

Yes, agreed; we should be able to assume that HAVE_LOG2 is always defined at this point.

skirpichev added a commit to skirpichev/cpython that referenced this issue Feb 8, 2023
miss-islington pushed a commit that referenced this issue Feb 9, 2023
…GH-101679)

Shouldn't affect users, hence no news.

Automerge-Triggered-By: GH:mdickinson
mdickinson pushed a commit that referenced this issue Feb 9, 2023
`math_1_to_whatever()` is no longer useful, since all existing uses of it convert to `float`.
Earlier versions of Python used `math_1_to_whatever` with an integer target; see
gh-16991 for the PR where that use was removed.
@mdickinson
Copy link
Member

mdickinson commented Feb 9, 2023

Closed by #101678 #101679.

carljm added a commit to carljm/cpython that referenced this issue Feb 9, 2023
* main: (82 commits)
  pythongh-101670: typo fix in PyImport_ExtendInittab() (python#101723)
  pythonGH-99293: Document that `Py_TPFLAGS_VALID_VERSION_TAG` shouldn't be used. (#pythonGH-101736)
  no-issue: Add Dong-hee Na as the cjkcodecs codeowner (pythongh-101731)
  pythongh-101678: Merge math_1_to_whatever() and math_1() (python#101730)
  pythongh-101678: refactor the math module to use special functions from c11 (pythonGH-101679)
  pythongh-85984: Remove legacy Lib/pty.py code. (python#92365)
  pythongh-98831: Use opcode metadata for stack_effect() (python#101704)
  pythongh-101283: Version was just released, so should be changed in 3.11.3 (pythonGH-101719)
  pythongh-101283: Fix use of unbound variable (pythonGH-101712)
  pythongh-101283: Improved fallback logic for subprocess with shell=True on Windows (pythonGH-101286)
  pythongh-101277: Port more itertools static types to heap types (python#101304)
  pythongh-98831: Modernize CALL and family (python#101508)
  pythonGH-101696: invalidate type version tag in `_PyStaticType_Dealloc` (python#101697)
  pythongh-100221: Fix creating dirs in `make sharedinstall` (pythonGH-100329)
  pythongh-101670: typo fix in PyImport_AppendInittab() (pythonGH-101672)
  pythongh-101196: Make isdir/isfile/exists faster on Windows (pythonGH-101324)
  pythongh-101614: Don't treat python3_d.dll as a Python DLL when checking extension modules for incompatibility (pythonGH-101615)
  pythongh-100933: Improve `check_element` helper in `test_xml_etree` (python#100934)
  pythonGH-101578: Normalize the current exception (pythonGH-101607)
  pythongh-47937: Note that Popen attributes are read-only (python#93070)
  ...
@erlend-aasland

This comment was marked as outdated.

@mdickinson
Copy link
Member

@erlend-aasland Gah. Thanks! I'll edit the message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension-modules C modules in the Modules dir type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

4 participants