-
Notifications
You must be signed in to change notification settings - Fork 11
sum_kbn(fn, A) (issue 3, in part) #6
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
Changes from 12 commits
ef5aef4
4876eb7
32b1e76
863d46c
d345164
ee5abb3
423929c
3d93013
733f0ea
f8915eb
39bc274
ad48228
03c5bc2
cbbcbcc
5cbd33a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -86,10 +86,11 @@ function cumsum_kbn(v::AbstractVector{T}) where T<:AbstractFloat | |
end | ||
|
||
""" | ||
sum_kbn(A) | ||
sum_kbn([f,] A) | ||
|
||
Return the sum of all elements of `A`, using the Kahan-Babuska-Neumaier compensated | ||
summation algorithm for additional accuracy. | ||
summation algorithm for additional accuracy. When a function `f` is supplied, the | ||
result of calling f on each element of `A` is summed. | ||
""" | ||
function sum_kbn(A) | ||
T = @default_eltype(typeof(A)) | ||
|
@@ -113,4 +114,27 @@ function sum_kbn(A) | |
s - c | ||
end | ||
|
||
function sum_kbn(f::Function, A) | ||
f === identity && return sum_kbn(A) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean that I should follow this pattern? function sum_kbn(f, A)
...
end
sum_kbn(a) = sum_kbn(identity, A) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. later comment says "yes" -- I will do that |
||
T = @default_eltype(typeof(A)) | ||
c = promote_sys_size_add(zero(T)::T) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You'll need to apply the function here as well There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the wrong type. The output of f might not match the eltype of A. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how does one obtain the f mapped eltype from eltype(A)? |
||
i = start(A) | ||
if done(A, i) | ||
return c | ||
end | ||
Ai, i = next(A, i) | ||
s = f(Ai) - c | ||
while !(done(A, i)) | ||
Ai, i = next(A, i) | ||
fAi = f(Ai); t = s + fAi | ||
if abs(s) >= abs(fAi) | ||
c -= ((s-t) + fAi) | ||
else | ||
c -= ((fAi-t) + s) | ||
end | ||
s = t | ||
end | ||
s - c | ||
end | ||
|
||
end # module |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needn't be a function; for example, one could also pass a type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the issue there is that the definition with
identity
was backwards. It should besum_kbn(A) = sub_kbn(identity, A)
, withsum_kbn(f, A)
as the only long-form definition of the function. That should remove any ambiguities.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does the application of identity disappear always?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.g. map(identity, [....])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the function call should get optimized out