Fixing abs for Rational{<:Unsigned} #29561
Conversation
fredrikekre
added
rationals
base/rational.jl
Outdated
| @@ -226,6 +226,8 @@ signbit(x::Rational) = signbit(x.num) | |||
| copysign(x::Rational, y::Real) = copysign(x.num,y) // x.den | |||
| copysign(x::Rational, y::Rational) = copysign(x.num,y.num) // x.den | |||
|
|
|||
| abs(x::Rational{T}) where {T<:Unsigned} = x | |||
There was a problem hiding this comment.
An alternative approach (and maybe more general?) would be to define
abs(x::Rational) = Rational(abs(x.num), x.den)
but I don't know what is best.
I also wonder if the ifelse in numbers.jl could simply be a ternary, i.e.
abs(x::Real) = signbit(x) ? -x : x
since, as noted in this PR, not all numbers can be negated.
There was a problem hiding this comment.
When T is unsigned, it is true that the ratio cannot be negative, so I guess this is fine. It's a bit weird to be using unsigned integers for rationals in the first place though, isn't it? I do think that @fredrikekre's suggestion for a generic definition that works for all rationals by calling abs on the numerator is better.
There was a problem hiding this comment.
Indeed @fredrikekre's proposal appears to be more general. Should I add another commit to the same PR making the change or resubmit a cleaner one?
By the way, I was using unsigned rationals since it is a possible tag type for EXIF metadata. I agree that they are a strange choice in a general use case.
There was a problem hiding this comment.
Just add more commits here. Much easier that way. We can squash it when it’s ready.
|
|
||
| @test abs(one(Rational{UInt})) === one(Rational{UInt}) | ||
| @test abs(one(Rational{Int})) === one(Rational{Int}) | ||
| @test abs(-one(Rational{Int})) === one(Rational{Int}) |
There was a problem hiding this comment.
Would be good to add a comment to reference this PR just above the tests. The two last tests seems unrelated to the PR?
There was a problem hiding this comment.
You are perfectly right. Just the first test is related to the bug. However, I did not find other tests regarding the abs method, so I thought that checking that the signed version works too would be useful. I admit I have not much experience about writing testcases, so if you think the tests should be kept to a minimum I will remove the last two.
There was a problem hiding this comment.
These seem fine to me. The main thing is to test it and I don’t mind having a slightly unrelated test added as well.
|
Happy to help! Even if it's just a very small bug ;-) |
|
Not at all! It’s great to have it noticed and fixed. |
* Fix abs for Rational{<:Unsigned}
* Add tests for abs(x::Rational)
(cherry picked from commit 4eb1a93)
* Fix abs for Rational{<:Unsigned}
* Add tests for abs(x::Rational)
(cherry picked from commit 4eb1a93)
* Fix abs for Rational{<:Unsigned}
* Add tests for abs(x::Rational)
(cherry picked from commit 4eb1a93)
* Fix abs for Rational{<:Unsigned}
* Add tests for abs(x::Rational)
(cherry picked from commit 4eb1a93)
I noticed that calling
absfor values of typeRational{<:Unsigned}results in anOverflowErrorbeing raised. E.g., in Julia 1.0.1 I getSince
abs(x::Real)innumber.jlusesifelse, it tries to compute-xwhich is not possible forx::Rational{<:Unsigned}. A possible solution for this problem is specializingabslikeas done in this pull request.