Description

skirpichev

opened on Jul 24, 2024

Issue body actions

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

• gh-122234: fix accuracy issues for sum() #122236
• gh-122234: Add DECREFs to error paths #122406

Activity

1. skirpichev

   added

   type-bugAn unexpected behavior, bug, or errorAn unexpected behavior, bug, or error

   on Jul 24, 2024
2. skirpichev commented on Jul 24, 2024

   

   skirpichev

   on Jul 24, 2024

   MemberAuthor

   More actions

   CC @rhettinger for the second part (integer values). Is this an issue or a feature?
3. picnixz commented on Jul 24, 2024

   

   picnixz

   on Jul 24, 2024

   Member

   More actions

   Actually, even without the sum, we have:

   >>> x = 10 ** 100
   >>> 1.0 + x + 1.0 - x
   0.0
4. skirpichev commented on Jul 24, 2024

   

   skirpichev

   on Jul 24, 2024

   MemberAuthor

   More actions

   @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?
5. skirpichev commented on Jul 24, 2024

   

   skirpichev

   on Jul 24, 2024

   MemberAuthor

   More actions

   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
   
6. skirpichev

   added a commit that references this issue on Jul 24, 2024

   pythongh-122234: fix accuracy issues for sum()

   a458432
7. skirpichev commented on Jul 24, 2024

   

   skirpichev

   on Jul 24, 2024

   MemberAuthor

   More actions

   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.
8. Eclips4

   added

   interpreter-core(Objects, Python, Grammar, and Parser dirs)(Objects, Python, Grammar, and Parser dirs)

   on Jul 24, 2024
9. vstinner

   added a commit that references this issue on Jul 29, 2024

   gh-122234: fix accuracy issues for sum() (#122236)

   169e713
10. vstinner

    closed this as completedon Jul 29, 2024
11. encukou

    added a commit that references this issue on Jul 29, 2024

    pythongh-122234: Add DECREFs to error paths

    a51d251
12. encukou commented on Jul 29, 2024

    

    encukou

    on Jul 29, 2024

    Member

    More actions

    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
13. encukou

    reopened this on Jul 29, 2024
14. Eclips4 commented on Jul 29, 2024

    

    Eclips4

    on Jul 29, 2024

    Member

    More actions

    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
15. encukou commented on Jul 29, 2024

    

    encukou

    on Jul 29, 2024

    Member

    More actions

    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.

    Reacted by Kirill Podoprigora and Sergey B Kirpichev
16. encukou

    added a commit that references this issue on Jul 29, 2024

    gh-122234: Add DECREFs to error paths (#122406)

    89fa05f
17. picnixz commented on Jul 29, 2024

    

    picnixz

    on Jul 29, 2024

    Member

    More actions

    Closing since the refleak is now fixed.

    Reacted by Kirill Podoprigora
18. picnixz

    closed this as completedon Jul 29, 2024

Issue actions