-
-
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
Fix edge cases in rounding of Rational(num, den) #15227
Conversation
cc @simonbyrne |
I expanded the ternaries, and used |
If I understand the question correctly, I think you're referring to parametric types. Looks like there's a syntax error, mismatched parens probably? |
Thank you, how embarrassing; fixed and working locally. Edit: great, travis survived. I'll try to think of further tests to make sure that everything is OK, but with the original tests and my additions, I think it's quite probable that there is nothing wrong. Performance suggestions are welcome. |
I like having the renaming be a separate commit from the functional change, but could you squash out the commit that had the typo and failed to build? |
d84a064
to
5e8a869
Compare
Yes, I thought I would "spell it out". I've squashed out the error. |
function round{Tf, Tr}(::Type{Tf}, x::Rational{Tr}, ::RoundingMode{:Nearest}) | ||
q,r = divrem(num(x), den(x)) | ||
s = q | ||
if abs(r) >= abs((den(x)-copysign(Tr(4), num(x))+one(Tr)+iseven(q))>>1 + copysign(Tr(2), num(x))) |
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.
What if Tr == Bool
?
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.
That is actually a good question. I can {Tf, Tr<Integer}
in the above and add something like
round{Tf}(::Type{Tf}, x::Rational{Bool}) = convert(Tf, x)
Since one(Bool) == true
and zero(Bool) == false
, so I think the conversion should make sense. You're also making me think of another edge case here. If den(x) == 0
no matter Tr
, the rational is well defined, but divrem
will fail, although round
should return -Inf
or Inf
depending on the sign of num(x)
. If num(x)
is also 0, then the rational is not well-defined, so I'm not sure what rounding it should mean (it would fail at construction).
Thanks to @simonbyrne 's comments above, I've added support for rounding of In a separate commit, I've added checks for Edit: and that seems to have made one of the old tests using Edit2: I see the error (if |
ac03343
to
05ca00a
Compare
Hm, I think the failure is unrelated. |
if someone wants to back up and post the log of the failure, any committer can restart the build. though aren't there some to be squashed commits still? to get linux 32 bit to pass needs to rebase in my recent .travis.yml changes anyway. |
Yes, there are certainly commits to squash, as I tried to addres the comments on |
390b9e2
to
96256b2
Compare
96256b2
to
a88dbae
Compare
Should the commits be further squashed? |
Would the intermediate commits pass tests without having the fix in the last commit? |
You asking if the tests that are in each commit will pass right? Tests are added in commit 1, and the first, second, and third commits pass those tests. In the fourth commit new tests and fixes are added, and they pass with that commit, but wouldn't pass on the earlier commits without the latest fix. I can checkout each commit later (or tomorrow) and run the tests to be sure, but that's how it should be. |
That sounds good to me. The main thing I want to avoid is accidentally landing at one of the intermediate commits when doing @simonbyrne if this looks all good to you, I say we merge. |
Fix edge cases in rounding of Rational(num, den)
One of the tests introduced by this PR fails all the time: https://travis-ci.org/JuliaLang/julia/jobs/112617751 Since the tests passed before merging, has another commit broken this recently? |
Got it, this is a bad interaction with #15228. We should just implement checked operations for |
Whoops. Well, that's an argument for the Rust/bors/homu workflow, since technically green CI results are stale and may be misleading as soon as there have been any newer commits to master since the CI last ran. |
Sorry if this was on my end. I'm pretty sure this got rebased a fairly short time before the merge, so maybe super bad luck? What was the offending change? |
That's just bad luck. You added tests using booleans with rationals, and the other PR changed rationals to use checked arithmetic -- but it didn't support booleans. It's actually surprising this kind of thing doesn't happen more often. If we see this again, maybe we can think about using a more robust workflow indeed. |
Fixes #15215 .
I would love someone to look this through, and give feedback. The first commit is the actual change, the second commit simply changes
x.num
andx.den
tonum(x)
andden(x)
.Basically the idea is to remove the number being bit shifted towards zero, do the bit shift, and add half the correction outside. The idea is very much like
log(exp(a-K)+exp(b-K))+K = log(exp(a)+exp(b))
for stability. The reason why it's 4 and not 2 is because two of theRoundingMode
s can add2
totypemax
, so...Update
The implementation seems to pass all tests, also the new tests which did not pass with the old method. Performance and style suggestions are still welcome. Can be merged if no one objects.
Update 2 What has happened so far
To fix #15215 I fixed the edge case if
x.den
wastypemax
. This was done by subtracting a number before doing the bit shift, and the adding half that number afterwards. Added tests.Changed the use of dots to functions:
x.num -> num(x)
andx.den -> den(x)
.Changed from ternary control flow to explicit
if
statements.Found a different edge case thanks to a comment by Simon Byrne (won't ping) on
Rational{Bool}
(see below). This edge case is whenden(x) == 0
. In this case we throw aDivideError()
if the return type is chosen to be anInteger
, and returns the appropriate±Inf
for floating point types.Made a
round
forRational{Bool}
. Uses the fact thatone(Bool) == true
andzero(Bool) == false
to returnInf
fortrue//false
,true
or1
fortrue//true
andfalse
or0
forfalse//true
. There should be tests for this depending on the wanted return type.This also reminds me that I should take care of the
true//false
case if the return type isInteger
orBool
, as they don't have anInf
value.Tf
back toT
, as I didn't think of the fact that they can beInteger
(f
was forfloat
)Came to fix an edge case, left dizzy :)