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

Accuracy issues of sum() specialization for floats/complexes #122234

Closed
skirpichev opened this issue Jul 24, 2024 · 9 comments
Closed

Accuracy issues of sum() specialization for floats/complexes #122234

skirpichev opened this issue Jul 24, 2024 · 9 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@skirpichev
Copy link
Member

skirpichev commented Jul 24, 2024

Bug report

Bug description:

Unfortunately, #121176 was merged with a bug:

cpython/Python/bltinmodule.c

Lines 2749 to 2755 in e968121

if (PyFloat_Check(item)) {
double value = PyFloat_AS_DOUBLE(item);
re_sum.hi += value;
im_sum.hi += 0.0;
_Py_DECREF_SPECIALIZED(item, _PyFloat_ExactDealloc);
continue;
}

L2751 lacks cs_add(). Sorry for that. Reproducer: sum([2j, 1., 10E100, 1., -10E100]) (should be 2+2j). I'll provide a patch.

But maybe cases for integer arguments also should use compensated summation? E.g.:

cpython/Python/bltinmodule.c

Lines 2689 to 2698 in e968121

if (PyLong_Check(item)) {
long value;
int overflow;
value = PyLong_AsLongAndOverflow(item, &overflow);
if (!overflow) {
re_sum.hi += (double)value;
Py_DECREF(item);
continue;
}
}

on L2694 (and use PyLong_AsDouble()). An example:

>>> sum([1.0, 10E100, 1.0, -10E100])
2.0
>>> sum([1.0, 10**100, 1.0, -10**100])  # huh?
0.0

I would guess, that integer values in this case are treated as exact and they are allowed to smash floating-point result to garbage. But... This looks as a bug for me. fsum() also chooses 2.0:

>>> math.fsum([1.0, 10**100, 1.0, -10**100])
2.0

CPython versions tested on:

CPython main branch

Operating systems tested on:

No response

Linked PRs

@skirpichev skirpichev added the type-bug An unexpected behavior, bug, or error label Jul 24, 2024
@skirpichev
Copy link
Member Author

CC @rhettinger for the second part (integer values). Is this an issue or a feature?

@picnixz
Copy link
Contributor

picnixz commented Jul 24, 2024

Actually, even without the sum, we have:

>>> x = 10 ** 100
>>> 1.0 + x + 1.0 - x
0.0

@skirpichev
Copy link
Member Author

skirpichev commented Jul 24, 2024

@picnixz, that's a feature.

Ok, it seems that in the second case PyLong_AsLongAndOverflow() just overflows and we fallback to the generic sum. That's something copied from the specialization for integers (in 8ce8a78) and it wasn't changed in #100426. Maybe it should?

@skirpichev
Copy link
Member Author

Hmm, with PyLong_AsDouble() + compensated summation it's even faster!

./python -m timeit -r11 -s 'xs=[1.0, 10**100, 1.0, -10**100]' 'sum(xs)'
500000 loops, best of 11: 634 nsec per loop

while in the main:

$ ./python -m timeit -r11 -s 'xs=[1.0, 10**100, 1.0, -10**100]' 'sum(xs)'
500000 loops, best of 11: 801 nsec per loop

skirpichev added a commit to skirpichev/cpython that referenced this issue Jul 24, 2024
* Use compensated summation for complex sums with floating-point items.
  This amends python#121176.

* sum() specializations for floats and complexes now use
  PyLong_AsDouble() instead of PyLong_AsLongAndOverflow() and
  compensated summation as well.
@skirpichev
Copy link
Member Author

PR is ready for review: #122236

It combines fixes for floats and integers. If second case requires more discussion or is "a feature", I can quickly revert that part.

@Eclips4 Eclips4 added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Jul 24, 2024
vstinner pushed a commit that referenced this issue Jul 29, 2024
* Use compensated summation for complex sums with floating-point items.
  This amends #121176.

* sum() specializations for floats and complexes now use
  PyLong_AsDouble() instead of PyLong_AsLongAndOverflow() and
  compensated summation as well.
encukou added a commit to encukou/cpython that referenced this issue Jul 29, 2024
@encukou
Copy link
Member

encukou commented Jul 29, 2024

The added error returns leak references. This was caught by the noGIL refleak buildbot (which runs more often than refleaks for regular-builds): https://buildbot.python.org/#/builders/1226/builds/2352/steps/6/logs/stdio

@Eclips4
Copy link
Member

Eclips4 commented Jul 29, 2024

The added error returns leak references. This was caught by the noGIL refleak buildbot (which runs more often than refleaks for regular-builds): https://buildbot.python.org/#/builders/1226/builds/2352/steps/6/logs/stdio

PR is ready: #122405

@encukou
Copy link
Member

encukou commented Jul 29, 2024

My PR #122406 looks exactly the same :)
IMO, it's better to associate the PR with this issue, so the fix-up PR is kept together with the original.

encukou added a commit that referenced this issue Jul 29, 2024
Co-Authored-By: Kirill Podoprigora <kirill.bast9@mail.ru>
@picnixz
Copy link
Contributor

picnixz commented Jul 29, 2024

Closing since the refleak is now fixed.

@picnixz picnixz closed this as completed Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

5 participants