Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
rhettinger merged 10 commits intopython:mainfromskirpichev:sum-complex-121149
Jul 5, 2024

Conversation

@skirpichev
Copy link
Contributor

@skirpichevskirpichev commentedJun 30, 2024
edited by github-actionsbot
Loading

Copy link
Member

@picnixzpicnixz 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.

@rhettingerrhettinger self-assigned thisJul 1, 2024
Copy link
Contributor

@dg-pbdg-pb left a comment
edited
Loading

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

@skirpichev
Copy link
ContributorAuthor

item = PyIter_Next(iter); has an overhead

That's out of the scope for this pr.

dg-pb reacted with thumbs up emoji

@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 namecsum is meaningful to you because you already know that you're implementing a compensated summation, but to fresh eyes, it is hard to not readcsum ascomplex_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 CompensatedSumcs_from_double(double x){    return (CompensatedSum) {x, 0.0};}static inline CompensatedSumcs_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 doublecs_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. Usesum andc if you prefer those tohi andlo.

@skirpichev
Copy link
ContributorAuthor

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 temporaryc variable, that has performance impact).

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

rhettinger reacted with thumbs up emoji

rhettingerand others added2 commitsJuly 3, 2024 10:08
Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@rhettingerrhettinger merged commitd4faa7b intopython:mainJul 5, 2024
@skirpichevskirpichev deleted the sum-complex-121149 branchJuly 6, 2024 01:10
noahbkim pushed a commit to hudson-trading/cpython that referenced this pull requestJul 11, 2024
skirpichev added a commit to skirpichev/cpython that referenced this pull requestJul 24, 2024
* Use compensated summation for complex sums with floating-point items.  This amendspython#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 requestJul 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 freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@picnixzpicnixzpicnixz left review comments

@rhettingerrhettingerrhettinger approved these changes

+1 more reviewer

@dg-pbdg-pbdg-pb requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@rhettingerrhettinger

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@skirpichev@rhettinger@dg-pb@picnixz

[8]ページ先頭

©2009-2025 Movatter.jp