-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Overflow when rounding rationals in edge case #15215
Comments
This should certainly be fixed. For Rationals we generally favour correctness over performance optimisations. |
I'll submit a pr later then. |
Fantastic, thanks! |
I'll make a PR soon, but I just have a style question. In the current code, the numerator and denominator are retrived as: x.num
x.den but wouldn't functions (they are already defined!) be more Julian than dots? num(x)
den(x) ? |
Yes, particularly if we make the changes discussed in #11522 |
Rounding rationals uses the good ol' right bit shift
This is fine, except the
+one(x.den)
addition gives a problematic edge case, whenx.den == typemax(T)
whereT = typeof(x.den)
. It's problematic because it overflows, and we get:The result is independent of the realization of
rand(IntType)
as the overflow will happen no matter what, resulting in a negative rounded half denominator which is always strictly smaller thanabs(r)
. This means that even if you get1//1
whennum>den
it is not because of a correct calculation, but a buggy result, that coincides with the correct answer.I would love to fix it, but I'm not sure what people would prefer. I know it is not Matlab, and Julia doesn't warn users of
typemax(::Type)+one(::Type)
, but inround()
we can actually find the answer if we are careful in how we compare whenden(r)==typemax(typeof(r))
. It will come with a perf cost, but I guess theround
of aRational
is rarely important performance wise.The text was updated successfully, but these errors were encountered: