-
-
Notifications
You must be signed in to change notification settings - Fork 15
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
Specialize sum over arrays of static numbers #51
Comments
Also, julia> using Static, BenchmarkTools
julia> @btime mapreduce(identity, +, fill(1, 10^5))
89.239 μs (2 allocations: 781.30 KiB)
100000
julia> @btime mapreduce(identity, +, fill(static(1), 10^5))
17.578 ms (1 allocation: 48 bytes)
static(100000) |
Yeah, that's not great. Is this something you've been running into or is it just a result of thorough testing? |
I plan to use static numbers for statistical weights in BAT.jl (I often have scenarios where samples may or may not be weighted, depending on sampling algorithm), so I played around a bit to make sure that operations like Somewhat related: JuliaArrays/FillArrays.jl#176 |
This is what Line 187 in 747cc66
Edit: oops, arrays, not tuples. Is your array type stable? IMO, it should return |
If you have 10^5 unique That's not to say there's nothing we can do here to make the situation better, but it seems like one of those corner cases where an unintuitive or involved solution may be worse than just explicitly telling people that they should avoid that pattern. Just my initial thoughts. |
You can easily end up with that with genric code, e.g. starting from |
Oh, so you're starting off with an ideal structure where the default is for everything to be I still think it could be problematic to implement a fix on our end. We'd have to somehow be aware of a dynamic number of iterations through a loop or awkwardly define |
Yes, exactly. I have a lot of generic code that handles statistical samples (and will have more). The samples may all have weight 1 - in which case important optimizations can be made or they may have arbitrary integer of floating point weights. So it would seem natural to use something like |
Hm A = fill(static(1), 10)
sum(A) comes down to
I think the culprit may be
This seems to be due to the return-type assertion in the definition of add_sum(x::Real, y::Real)::Real = x + y Since julia> foo(a::Real, b::Real) = a + b
foo (generic function with 1 method)
julia> foo(x, x)
static(2)
julia> bar(a::Real, b::Real)::Real = a + b
bar (generic function with 1 method)
julia> bar(x, x)
2 which I don't get, since |
A bit off-topic in this thread: Isn't this what StatsBase.UnitWeights is designed for? |
@devmotion In principle, yes. Though if we can get this to work, we could get rid of But I actually had meant to pull to talk to the |
I didn't want to initiate a longer discussion here, I don't think it's the right place for it. Just briefly: I don't think it's possible, and I don't even think it's desirable, to get rid of |
Sorry, I shouldn't have said "get rid of", I agree that the additional semantics (beyond "array of ones") of |
Like Chris pointed out, this also depends on knowing the size array in some cases to be type stable. It's hard to know what the best approach is for addressing this until we get some closure on JuliaLang/julia#44538. This seems like the sort of thing that should be part of the methods internal interface with something like function _add_sum(A::AbstractArray{StaticInt{N}}) where {N}
if known_length(A) === nothing
return length(A) * N
else
return StaticInt{known_length(A) * N}()
end
end because it's usually a bad idea to dispatch on the element type of an abstract collection. |
Currently, julia> @btime mapreduce(sqrt, Base.add_sum, A)
17.152 ms (0 allocations: 0 bytes)
static(204939.01531919395) Could we add a specialization like function Base.mapreduce(f, ::typeof(Base.add_sum), A::AbstractVector{T}) where {T<:StaticFloat64}
length(eachindex(A)) * T()
end it would get us julia> @btime mapreduce(sqrt, Base.add_sum, A)
73.947 ns (1 allocation: 16 bytes)
420000.0 |
Darn, we'd get an ambiguity with StaticArrays with that (possibly other packages too):
|
We currently have
but
The text was updated successfully, but these errors were encountered: