sum_kbn(fn, A) (issue 3, in part) #6

JeffreySarnoff · 2017-12-11T21:22:14Z

No description provided.

ararslan · 2017-12-11T21:32:58Z

src/KahanSummation.jl

@@ -113,4 +114,6 @@ function sum_kbn(A)
    s - c
 end

+sum_kbn(f, A) = f.(A)


This isn't quite correct; f is applied elementwise to A but the result isn't summed. Perhaps you meant sum_kbn(f.(A))?

Even that will make two passes over the array, so we're likely able to do better. I'm not sure this is entirely correct either, but something like this would be a start:

diff --git a/src/KahanSummation.jl b/src/KahanSummation.jl index fde6700..26a7535 100644 --- a/src/KahanSummation.jl +++ b/src/KahanSummation.jl @@ -91,22 +91,23 @@ end Return the sum of all elements of `A`, using the Kahan-Babuska-Neumaier compensated summation algorithm for additional accuracy. """ -function sum_kbn(A) +function sum_kbn(f, A) T = @default_eltype(typeof(A)) - c = promote_sys_size_add(zero(T)::T) + c = f(promote_sys_size_add(zero(T)::T)) i = start(A) if done(A, i) return c end Ai, i = next(A, i) - s = Ai - c + s = f(Ai) - c while !(done(A, i)) Ai, i = next(A, i) - t = s + Ai - if abs(s) >= abs(Ai) - c -= ((s-t) + Ai) + fAi = f(Ai) + t = s + fAi + if abs(s) >= abs(fAi) + c -= ((s-t) + fAi) else - c -= ((Ai-t) + s) + c -= ((fAi-t) + s) end s = t end

is view applicable?

Hm, not exactly, I don't think, since the issue is iterating over the input multiple times, whereas view would be useful to avoid making copies.

ararslan · 2017-12-11T21:58:49Z

src/KahanSummation.jl

-sum_kbn(f, A) = f.(A)
+function sum_kbn(f,A)
+    T = @default_eltype(typeof(A))
+    c = promote_sys_size_add(zero(T)::T)


You'll need to apply the function here as well

ararslan · 2017-12-11T21:59:27Z

It would also be great if you could add some tests for this functionality. Thanks for taking this on!

stevengj · 2017-12-11T22:24:55Z

src/KahanSummation.jl

@@ -113,4 +114,26 @@ function sum_kbn(A)
    s - c
 end

+function sum_kbn(f,A)


Can we merge the two functions? i.e. define sum_kbn(A) = sum_kbn(identity, A)?

how is that done? how should I modify latest PR

oh -- I see~

JeffreySarnoff · 2017-12-11T23:30:05Z

@stevengj @ararslan I think this does it for now -- let me know if something should be changed.

stevengj · 2017-12-12T16:06:15Z

src/KahanSummation.jl

+function sum_kbn(f::Function, A)
+    f === identity && return sum_kbn(A)
+    T = @default_eltype(typeof(A))
+    c = promote_sys_size_add(zero(T)::T)


This is the wrong type. The output of f might not match the eltype of A.

how does one obtain the f mapped eltype from eltype(A)?

stevengj · 2017-12-12T16:10:55Z

src/KahanSummation.jl

@@ -115,6 +115,7 @@ function sum_kbn(A)
 end

 function sum_kbn(f::Function, A)
+    f === identity && return sum_kbn(A)


This is backwards. sum_kbn(A) should be implemented in terms of sum_kbn(identity, A), not the other way around. This way there will be only one implementation of the sum.

Do you mean that I should follow this pattern?

function sum_kbn(f, A) ... end sum_kbn(a) = sum_kbn(identity, A)

later comment says "yes" -- I will do that

ararslan · 2017-12-12T18:27:48Z

src/KahanSummation.jl

@@ -113,4 +114,27 @@ function sum_kbn(A)
    s - c
 end

+function sum_kbn(f::Function, A)


This needn't be a function; for example, one could also pass a type.

I know -- I do not know how to annotate Function or TypeConstructor. When I had it as sum_kbn(f, A) things got murky with sum_kbn(identity, A) and there was no clean dispatch resolution. What is the correct way to approach this while following the guide that sum_kbn(A) should be defined in terms of sum_kbn(identity, A)?

I think the issue there is that the definition with identity was backwards. It should be sum_kbn(A) = sub_kbn(identity, A), with sum_kbn(f, A) as the only long-form definition of the function. That should remove any ambiguities.

does the application of identity disappear always?

e.g. map(identity, [....])

Yes, the function call should get optimized out

simonbyrne · 2017-12-12T18:52:58Z

I've managed to do this largely in #7, though don't have tests yet.

JeffreySarnoff · 2017-12-12T18:57:29Z

@simonbyrne 👍

JeffreySarnoff · 2017-12-12T19:02:15Z

what are
Base.r_promote_type(::typeof(plus_kbn), ::Type{T}) where {T<:AbstractFloat} = TwicePrecision{T}
Base.mr_empty(f, ::typeof(plus_kbn), T) = TwicePrecision{T}
and what is their equivalent for v0.7?

JeffreySarnoff · 2017-12-12T19:13:01Z

is this correct:

    T = Base.return_types(f, (@default_eltype(typeof(A)),))[1]

stevengj · 2017-12-12T19:29:52Z

See how the reduce code does it in base. If the collection is non empty, you shouldn’t use inference at all

JeffreySarnoff · 2017-12-12T19:46:57Z

I do not know what more I can do to make coveralls happy. I put in what it seems to want -- then again I am not conversant with coveralls.

JeffreySarnoff · 2018-03-23T02:22:19Z

this is ok to close

JeffreySarnoff · 2018-03-23T02:25:34Z

hmm -- this is applesauce

JeffreySarnoff added 2 commits December 11, 2017 15:48

vec -> avec (vec is a function)

ef5aef4

JuliaMath#3 (in part)

4876eb7

ararslan reviewed Dec 11, 2017

View reviewed changes

Update KahanSummation.jl

32b1e76

ararslan reviewed Dec 11, 2017

View reviewed changes

stevengj reviewed Dec 11, 2017

View reviewed changes

JeffreySarnoff added 9 commits December 11, 2017 17:27

test sum_kbn(f, A)

863d46c

respond to comment

d345164

fixup

ee5abb3

correct for Method definition sum_kbn(Any, Any) overwrite

423929c

change name of testset

3d93013

combine test sets

733f0ea

test identity

f8915eb

try to make coveralls happy

39bc274

fold special case for identity into sum_kbn

ad48228

stevengj reviewed Dec 12, 2017

View reviewed changes

ararslan reviewed Dec 12, 2017

View reviewed changes

JeffreySarnoff added 2 commits December 12, 2017 14:15

Update KahanSummation.jl

03c5bc2

maybe ok

cbbcbcc

add more cumsum_kbr(A, dim) tests

5cbd33a

JeffreySarnoff closed this Mar 23, 2018

JeffreySarnoff reopened this Mar 23, 2018

JeffreySarnoff closed this Jan 30, 2019

sum_kbn(fn, A) (issue 3, in part) #6

sum_kbn(fn, A) (issue 3, in part) #6

Uh oh!

Conversation

JeffreySarnoff commented Dec 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ararslan commented Dec 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeffreySarnoff commented Dec 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonbyrne commented Dec 12, 2017

Uh oh!

JeffreySarnoff commented Dec 12, 2017

Uh oh!

JeffreySarnoff commented Dec 12, 2017

Uh oh!

JeffreySarnoff commented Dec 12, 2017

Uh oh!

stevengj commented Dec 12, 2017

Uh oh!

JeffreySarnoff commented Dec 12, 2017

Uh oh!

JeffreySarnoff commented Mar 23, 2018

Uh oh!

JeffreySarnoff commented Mar 23, 2018

Uh oh!

Uh oh!