added support for an arbitrary number of fraction bits#44
added support for an arbitrary number of fraction bits#44timholy merged 2 commits intoJuliaMath:masterfrom
Conversation
|
A lot of the generated functions are unnecessary and I think we could rework most of the eval-for loops by using methods like Will put up an PR for that momentarily. Thank you for your help. |
ddff03b to
f9218ff
Compare
|
doh! it should've occurred to me that generated functions whose body only contains the returned expression could be rewritten as normal functions. thanks! i've added more tests, updated the docs, and included a commit, which could be a separate PR if preferred, which fixes the type returned by any ideas for benchmarks? |
f9218ff to
4eb9be2
Compare
src/ufixed.jl
Outdated
| one(::Type{$T}) = $T($(2^f-1),0) | ||
| end | ||
| end | ||
| one{T<:UFixed}(::Type{T}) = T((2^nbitsfrac(T)-1),0) |
There was a problem hiding this comment.
This will be costly on v0.4. On v0.5 we can constant fold this, but only if we outline the 2^nbitsfrac(T) into a pure function Base.@pure one_pattern(f)=(2^f-1), but this won't work on v0.4.
One solution would be for now to use the current way on v0.4 with your method as a fallback, and on v0.5 and higher use pure functions on the type-parameter to get the same effect.
There was a problem hiding this comment.
good catch. thanks. it was 4x slower. fixed for 0.4 and 0.5 with a generated function. that seems a simpler solution to me than having to switch on the version.
|
Thank you for tackling this. The added generality will be valuable, but we have to make sure that this comes at no extra cost, because FixedPointNumbers is used in performance sensitive places. Would you mind also adding a few benchmarks with |
4eb9be2 to
340ca82
Compare
|
thanks for pointing out BenchmarkTools to me. i used it to fix a few things you pointed out. is it customary to commit benchmarks? if so, where? |
| round{T<:Integer}(::Type{T}, x::UFixed) = round(T, reinterpret(x)/rawone(x)) | ||
| floor{T<:Integer}(::Type{T}, x::UFixed) = trunc(T, x) | ||
| ceil{T<:Integer}(::Type{T}, x::UFixed) = ceil(T, reinterpret(x)/rawone(x)) | ||
| trunc(x::UFixed) = trunc(Int, x) |
There was a problem hiding this comment.
Any reason for these to be removed?
There was a problem hiding this comment.
this is part of the second commit. removing them fixes the bug that round(UFixed) returns an Int, not a UFixed.
340ca82 to
bd212ea
Compare
|
This looks quite good to me. I do not personally have a problem with the generated functions. Some could become |
bd212ea to
b36f86b
Compare
|
anything else? |
src/FixedPointNumbers.jl
Outdated
| convert(Tdual, b/one(T)), reinterpret(rawtype(T), x) | ||
|
|
||
| # printing | ||
| @generated function show{T,f}(io::IO, x::FixedPoint{T,f}) |
There was a problem hiding this comment.
Why is this a @generated function? I would think it is unnecessary here ;)
There was a problem hiding this comment.
Right, this should not be @generated, it's not performance-critical and in any case being @generated won't help here.
|
Looks good to me. |
b36f86b to
7caa8ee
Compare
|
is this what you had in mind instead of a generated function? |
README.md
Outdated
| `0xffff`). | ||
|
|
||
| To construct such a number, use `convert(UFixed12, 1.3)`, `ufixed12(1.3)` (a | ||
| convenience function), `UFixed{UInt16,12}`, or the literal syntax `0x14ccuf12`. |
There was a problem hiding this comment.
UFixed{UInt16,12} is a type, not a value.
|
Sorry to keep pestering you with review comments. I was about to hit merge and thought I should give it another once-over. I must be less tired this time. |
7caa8ee to
994d96e
Compare
i need to add four
Image{Gray{UFixed16}}together without overflowing. could convert toFloat32, but thought aUFixed{UInt32,16}would be more graceful. so before realizing that the snazzy new@generatedwas taboo, i refactoredufixed.jlto support arbitrary fractional bit depths. anything goes, likeUFixed{UInt64,3}, as an additional example.all of the tests pass in julia 0.5-rc2.
so why are generated functions to be avoided?