-
Notifications
You must be signed in to change notification settings - Fork 25
Create FixedRational
type for faster dimensions
#21
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
Conversation
Benchmark Results
Benchmark PlotsA plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR. |
src/fixed_rational.jl
Outdated
Base.convert(::Type{FixedRational{T,den}}, x::Integer) where {T,den} = unsafe_fixed_rational(x * den, T, Val(den)) | ||
Base.convert(::Type{FixedRational{T,den}}, x::Rational) where {T,den} = FixedRational{T,den}(x) | ||
Base.convert(::Type{Rational}, x::FixedRational{T,den}) where {T,den} = Rational{T}(x.num, den) | ||
Base.convert(::Type{AF}, x::FixedRational{T,den}) where {AF<:AbstractFloat,T,den} = convert(AF, x.num / den) |
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.
x.num//den
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.
It converts to an AbstractFloat
, so x.num//den
would be significantly slower as it would have to compute divgcd
first
I still think this should probably use |
Thanks for the review!
I agree but I am just worried about introducing bugs for that 10% of users who don't expect such significant rounding. Maybe a compromise would be to recommend |
I think it will be faster even if you make it throw errors for rounding or overflow. |
Tricky to throw errors for rounding, since it happens quite frequently (e.g., I'll just merge for now so we can at least improve speeds over |
That's easy. That turns into a division by 2 which just a check to see if the input is odd (if so, error) is a bitshift by 1. |
Hm, I see. Okay I will think more about this. In principle I am on board with your suggestion; just want to be safe! |
Oof, this is tricky. struct FixedRational{T<:Integer,den::T} <: Real
num::T
end is illegal syntax. But otherwise, just writing denom(::FixedRational{T,den}) = convert(T, den) and hope the compiler can propagate it |
I don't think the type of |
Not sure which one you mean, but I'll answer both:
|
Okay fixed with this: denom(::Type{F}) where {T,den,F<:FixedRational{T,den}} = convert(T, den) which should be used to access the denominator of |
I meant 2, but it's good to know that this fixes it. The other possible fix would be to define |
Rational{Int16}
is about 8x slower than simply usingInt32/C
for a constantC
. This PR defines this type asFixedRational{T,denominator}
. The default type is nowFixedRational{Int32, 2^4 * 3^2 * 5^2 * 7}
, which seems like a reasonable choice.The downside is that one can not represent, e.g.,
1/11
in a dimension.Users can of course use their own type for the dimension type parameter, including the faster
FixedRational{Int8,6}
. (Though I chose a safer default.)@oscardssmith could you review this?