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

gh-121149: improve accuracy of builtin sum() for complex inputs #121176

Merged
merged 10 commits into from
Jul 5, 2024

Conversation

skirpichev
Copy link
Member

@skirpichev skirpichev commented Jun 30, 2024

Copy link
Contributor

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Not much to say on that PR except some style comments.

Doc/library/functions.rst Outdated Show resolved Hide resolved
Python/bltinmodule.c Outdated Show resolved Hide resolved
Python/bltinmodule.c Outdated Show resolved Hide resolved
Python/bltinmodule.c Outdated Show resolved Hide resolved
Python/bltinmodule.c Outdated Show resolved Hide resolved
Python/bltinmodule.c Outdated Show resolved Hide resolved
Python/bltinmodule.c Outdated Show resolved Hide resolved
@rhettinger rhettinger self-assigned this Jul 1, 2024
Copy link
Contributor

@dg-pb dg-pb left a comment

Choose a reason for hiding this comment

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

item = PyIter_Next(iter);

has an overhead (which can be non trivial in certain cases) for each iteration compared to more direct

iternext = *Py_TYPE(it)->tp_iternext;
item = iternext(it);

and StopIteration handler in the end

Python/bltinmodule.c Outdated Show resolved Hide resolved
Python/bltinmodule.c Outdated Show resolved Hide resolved
@skirpichev
Copy link
Member Author

item = PyIter_Next(iter); has an overhead

That's out of the scope for this pr.

Python/bltinmodule.c Outdated Show resolved Hide resolved
@rhettinger
Copy link
Contributor

Overall this patch looks basically sound.

If you're open to change, I wish it was in more of a functional style than a mutate in place style. I find the latter harder to read and harder to debug.

Also the name csum is meaningful to you because you already know that you're implementing a compensated summation, but to fresh eyes, it is hard to not read csum as complex_sum which is incorrect.

I suggest something like this:

typedef struct {
    double hi;     /* high-order bits for a running sum */
    double lo;     /* a running compensation for lost low-order bits */
} CompensatedSum;

static inline CompensatedSum
cs_from_double(double x)
{
    return (CompensatedSum) {x, 0.0};
}

static inline CompensatedSum
cs_add(CompensatedSum total, double x)
{
    double t = total.hi + x;
    if (fabs(total.hi) >= fabs(x)) {
        c = (total.hi - t) + x;
    } else {
        c = (x - t) + total.hi;
    }
    return (CompensatedSum) {t, total.lo + c}
}

static inline double
cs_to_double(CompensatedSum total)
{
    if (total.lo && isfinite(total.lo))
        return total.hi + total.lo;
    return total.hi;
}

It could be used very plainly and readably, making it obvious what is changing:

CompensatedSum total = cs_from_double(start);
for (i=0 ; i<n ; i++) {
    total = cs_add(total, xarr[i]);
}
return cs_to_double(total);

I'm not attached to the names only the core concept. Use sum and c if you prefer those to hi and lo.

@skirpichev
Copy link
Member Author

I suggest something like this: ... CompensatedSum

Thanks, I like new names.

Use sum and c if you prefer those to hi and lo.

That was chosen for compatibility with the referenced wiki page, but the later variant does make sense too. And it's already used in the fsum().

2698be6 - renaming part

I wish it was in more of a functional style than a mutate in place style. I find the latter harder to read and harder to debug.

Does make sense. I did this change in the second commit, 5242bd6 (with a minor change: don't introduce temporary c variable, that has performance impact).

As we inline all helpers - this variant should have same performance as in the commit above.

Lib/test/test_builtin.py Outdated Show resolved Hide resolved
Lib/test/test_builtin.py Outdated Show resolved Hide resolved
rhettinger and others added 2 commits July 3, 2024 10:08
Co-authored-by: Sergey B Kirpichev <[email protected]>
Co-authored-by: Bénédikt Tran <[email protected]>
@rhettinger rhettinger merged commit d4faa7b into python:main Jul 5, 2024
36 checks passed
@skirpichev skirpichev deleted the sum-complex-121149 branch July 6, 2024 01:10
noahbkim pushed a commit to hudson-trading/cpython that referenced this pull request Jul 11, 2024
skirpichev added a commit to skirpichev/cpython that referenced this pull request 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.
vstinner pushed a commit that referenced this pull request 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants