-
-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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
Specialization for accurate complex summation in sum()? #121149
Comments
My opinion is that the goal of being accurate should be delegated to a specialized function and that the built in function The change introduced in #100425 increased the complexity of explaining what Sum of finite precision floating point has its surprises, when one haven't learned that it is not real numbers what is being added. But in my opinion, the built in |
+1 from me. As a test, the result should match |
One reason for builtin function to be a builtin - well, be useful. Just be "simple-to-explain" is not enough. What is a point in preserving huge accumulated rounding error? Say, if you want to reproduce inaccurate sum with some specific order of operations - you can just use something like Also, is it "simple-to-explain" for you when
There is also math.fsum(), it's mentioned in sphinx docs for sum(). But it uses much more complex algorithm. (BTW, it looks faster on random data.)
FYI, pr is ready for review: #121176 |
Clap, clap, clap. Everyone implicitly being forced to opt in doing a compensated sum for Please don't ever "enhance" it to subtypes. Those who need a fast and proper sum of class R(float): pass
class C(complex): pass
sum(L, start=R(0.0)) # or sum(L, start=C(0.0)) Regarding
That is the type of thing that you want to have to explain. |
Your abusive posts are not welcome here. |
Probably, this doesn't make sense just because >>> class myint(int):
... def __add__(self, other):
... print("Nobody expects the Spanish Inquisition!")
...
>>> myint()+myint()
Nobody expects the Spanish Inquisition! I can't imagine many practical cases to do that... Perhaps, implementing some parallel algorithm for addition of big ints? But that's all. More generic checks (say, PyFloat_Check()) are costly in case of negative results. But in sum() we stop specialization of given type and fall though to more generic one (say, to complex) or to using PyNumber_Add(). So, I think that more generic checks are cheap in this type of workflow. @rhettinger, is this extension of specialization in sum() does make sense for you? If not, please note that after 88bdb92 we have PyLong_Check()'s in some places. I think that was a bug (CC @serhiy-storchaka). This was extended in my pr with one PyFloat_Check() and I forgot to fix that, sorry:( a patch to fix few instances of "soft" type checksdiff --git a/Python/bltinmodule.c b/Python/bltinmodule.c
index a5b45e358d..49675c224c 100644
--- a/Python/bltinmodule.c
+++ b/Python/bltinmodule.c
@@ -2686,7 +2686,7 @@ builtin_sum_impl(PyObject *module, PyObject *iterable, PyObject *start)
_Py_DECREF_SPECIALIZED(item, _PyFloat_ExactDealloc);
continue;
}
- if (PyLong_Check(item)) {
+ if (PyLong_CheckExact(item) || PyBool_Check(item)) {
long value;
int overflow;
value = PyLong_AsLongAndOverflow(item, &overflow);
@@ -2735,7 +2735,7 @@ builtin_sum_impl(PyObject *module, PyObject *iterable, PyObject *start)
Py_DECREF(item);
continue;
}
- if (PyLong_Check(item)) {
+ if (PyLong_CheckExact(item) || PyBool_Check(item)) {
long value;
int overflow;
value = PyLong_AsLongAndOverflow(item, &overflow);
@@ -2746,7 +2746,7 @@ builtin_sum_impl(PyObject *module, PyObject *iterable, PyObject *start)
continue;
}
}
- if (PyFloat_Check(item)) {
+ if (PyFloat_CheckExact(item)) {
double value = PyFloat_AS_DOUBLE(item);
re_sum.hi += value;
im_sum.hi += 0.0;
Not at all! >>> res, xs = 0.0, [0.1]*10
>>> for _ in xs:
... res += _
...
>>> res
0.9999999999999999 |
Do you have an example where this makes difference? |
Hmm, you are right, sorry for a false alarm. All such examples looks broken. |
After python/cpython#121149 this was broken on CPython 3.14: _______________ [doctest] mpmath.functions.orthogonal.spherharm ____________ EXAMPLE LOCATION UNKNOWN, not showing all tests of that example ??? >>> fp.chop(fp.quad(lambda t,p: Y1(t,p)*Y2(t,p)*dS(t,p), *sphere)) Expected: 1.0000000000000007 Got: 1.0000000000000004 This particular failure could be fixed by: diff --git a/mpmath/ctx_base.py b/mpmath/ctx_base.py index 3309bfa..2ca2fac 100644 --- a/mpmath/ctx_base.py +++ b/mpmath/ctx_base.py @@ -110,7 +110,8 @@ def fdot(ctx, xs, ys=None, conjugate=False): cf = ctx.conj return sum((x*cf(y) for (x,y) in xs), ctx.zero) else: - return sum((x*y for (x,y) in xs), ctx.zero) + return ctx.mpc(sum(((x*y).real for (x,y) in xs), ctx.zero), + sum(((x*y).imag for (x,y) in xs), ctx.zero)) def fprod(ctx, args): prod = ctx.one But this is just one case for the whole test suite...
Feature or enhancement
Proposal:
Currently, sum() builtin lacks any specialization for complex numbers, yet it's usually faster than better pure-python alternatives.
benchmark sum() wrt pure-python version
Hardly using sum() component-wise is an option:
Unfortunately, direct using this builtin in numeric code doesn't make sense, as results are (usually) inaccurate. It's not too hard to do summation component-wise with math.fsum(), but it's slow and there might be a better way.
In #100425 simple algorithm, using compensated summation, was implemented in sum() for floats. I propose (1) make specialization in sum() for complex numbers, and (2) reuse #100425 code to implement accurate summation of complexes.
(1) is simple and strightforward, yet it will give some measurable performance boost
benchmark sum() in the main wrt added specialization for complex
main:
with specialization:
(2) also seems to be a no-brain task: simple refactoring of PyFloat specialization should allows us use same core for PyComplex specialization.
If there are no objections against - I'll work on a patch.
Has this already been discussed elsewhere?
This is a minor feature, which does not need previous discussion elsewhere
Links to previous discussion of this feature:
No response
Linked PRs
The text was updated successfully, but these errors were encountered: