-
-
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
changed lcm and gcd implementations for when just one arg is infinite #57067
base: master
Are you sure you want to change the base?
Conversation
Thanks! Looking at this I'm realizing there is another way to define Currently, we define
If we change that to
Then the following are correct: julia> lcm(1//0, 1//1)
1//0 # currently 1//1
julia> lcm(1//0, 0//1)
0//1 # current behavior, only correct with #57068, otherwise error is appropriate
julia> gcd(1//0, 1//1)
1//1 # currently 1//0
julia> gcd(1//0, 0//1)
1//0 # current behavior, only correct with #57068, otherwise error is appropriate EDITED to incorporate #57067 (comment) |
since the discussion was done, this is the new implementation. Minor change -
|
Triage wants to add the definition of multiple to gcd to match lcm (but it should be the definition of divides). Triage does not like Lilith's idea of extending integers to include 1//0, -1//0, and false in this case. |
I’ve updated the implementation to throw an error when one of the arguments is infinite in gcd/lcm and fixed the related tests to reflect this change. Additionally, I’ve added a definition for 'multiple' in the docs :) |
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.
Thanks! I've left some inline comments and this looks good; we should be able to get it merged.
``a`` is a multiple of ``b`` if there exists an integer ``m`` such | ||
that ``b*m=a``. | ||
``a`` is a multiple of ``b`` if there exists an integer ``m`` such | ||
that ``a=mb``. |
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.
This note is already present; it previously looked like it wasn't because the base commit was out of date.
gcd(x::Rational, y::Rational) = unsafe_rational(gcd(x.num, y.num), lcm(x.den, y.den)) | ||
lcm(x::Rational, y::Rational) = unsafe_rational(lcm(x.num, y.num), gcd(x.den, y.den)) | ||
function gcd(x::Rational, y::Rational) | ||
if (isinf(x) && !isinf(y)) || (!isinf(x) && isinf(y)) |
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.
I think this is more clear:
if (isinf(x) && !isinf(y)) || (!isinf(x) && isinf(y)) | |
isinf(x) != isinf(y) |
lcm(x::Rational, y::Rational) = unsafe_rational(lcm(x.num, y.num), gcd(x.den, y.den)) | ||
function gcd(x::Rational, y::Rational) | ||
if (isinf(x) && !isinf(y)) || (!isinf(x) && isinf(y)) | ||
throw(ArgumentError("lcm is not defined")) |
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.
throw(ArgumentError("lcm is not defined")) | |
throw(ArgumentError("lcm is not defined between infinite and finite numbers")) |
if (isinf(x) && !isinf(y)) || (!isinf(x) && isinf(y)) | ||
throw(ArgumentError("lcm is not defined")) | ||
end | ||
return unsafe_rational(gcd(x.num, y.num), lcm(x.den, y.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.
return unsafe_rational(gcd(x.num, y.num), lcm(x.den, y.den)) | |
unsafe_rational(gcd(x.num, y.num), lcm(x.den, y.den)) |
Most of this file uses implicit return stlye
end | ||
end | ||
|
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.
Let's remove this unrelated whitespace change
|
||
@test_throws ArgumentError gcd(T(1)//T(1), T(1)//T(0)) | ||
@test_throws ArgumentError gcd(T(1)//T(0), T(0)//T(1)) | ||
|
||
@test_throws ArgumentError lcm(T(1)//T(1), T(1)//T(0)) | ||
@test_throws ArgumentError lcm(T(1)//T(0), T(1)//T(1)) |
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.
For tests, let's just change all the existing tests to @test_throws; that way we know we're not losing coverage. It will also make the blame/diff easier to review.
Fixes #56991 by adding conditions for both gcd and lcm cases. Also fixes tests for the same i.e. lot of tests regarding gcdx and lcm are removed. Two relevant tests have been added as well.