-
Notifications
You must be signed in to change notification settings - Fork 33
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
WIP: add support for CheckedArithmetic #146
base: master
Are you sure you want to change the base?
Conversation
I'm against depending on
Originally posted by @timholy in #41 (comment) In my opinion, the first thing we should do is implement the checked and unchecked methods. There is a common interface FixedPointNumbers.jl/src/FixedPointNumbers.jl Lines 154 to 160 in 49627e2
|
Yes, you're right, I hadn't yet gotten to implementing the checked methods. Definitely an oversight.
Can you explain why? Unless we depend on CheckedArithmetic, then the algorithm-writer is going to have to know whether to call |
First of all, |
So basically you're saying |
Generally speaking, it is better to modularize different things individually. However, it is another matter and I do not think it should be done right now. The new package containing I didn't understand what IMO, |
Using the julia> @btime mysum(UInt(0), $A)
8.320 ns (0 allocations: 0 bytes)
0x0000000000003566
julia> @btime mysum(BigInt(0), $A)
6.048 μs (301 allocations: 4.72 KiB)
13670 Given that it's a 1000x slower, I don't think BigInt is a viable
For As long as we cover the common element types used in image processing, I'd be fine with restricting |
I fully understand that, but that's not the answer I'm looking for. In fact, 18 years is extremely short in my industry.:laughing: In order to avoid such a barren discussion, formal specifications are necessary. Edit:
For now, why does not |
This is mentioned in PR #143, etc., but as far as accumulation is concerned, it is faster and theoretically more accurate not to convert the elements to function _sum(a::AbstractArray{N}) where N <: Normed{UInt8}
if length(a) > 0x01010101
mapreduce(reinterpret, +, a, init = UInt64(0)) / FixedPointNumbers.rawone(N)
else
mapreduce(reinterpret, +, a, init = UInt32(0)) / FixedPointNumbers.rawone(N)
end
end julia> x1 = collect(rand(N0f8, 4096, 4096));
julia> x2 = collect(rand(N0f8, 8192, 4096));
julia> @btime sum($x1);
2.795 ms (0 allocations: 0 bytes)
julia> @btime sum($x2);
5.688 ms (0 allocations: 0 bytes)
julia> @btime _sum($x1);
933.800 μs (0 allocations: 0 bytes)
julia> @btime _sum($x2);
2.624 ms (0 allocations: 0 bytes) The reason I mentioned this PR after a long time is that, IIUC, we are moving towards making |
Now that we have the package extension mechanism and Preferences.jl, we need to revisit this. |
Here's a draft supporting CheckedArithmetic. Perhaps the key question is resolving the discussion in #143, determining which type should be used as the
accumulatortype
. For*
and/
, IMO the only candidate isfloattype
. We could useNormed{UInt, f}
for+
andfloattype(T)
for-
, or useNormed{Int, f}
for both+
and-
if we decide to go with #143.