| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
Not much to say on that PR except some style comments.
Sorry, something went wrong.
There was a problem hiding this comment.
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
Sorry, something went wrong.
That's out of the scope for this pr. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
Thanks, I like new names.
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
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. |
Sorry, something went wrong.
Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
* 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.
| Back | FazBrowse Home | New Git URL |
📚 Documentation preview 📚: https://cpython-previews--121176.org.readthedocs.build/