Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix and test lcm([1//2, 1//2]) == 1//1 #56423

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix and test lcm([1//2, 1//2]) == 1//1 #56423

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fd5346d
Fix and test lcm([1//2, 1//2]) == 1//1
LilithHafner Nov 2, 2024
4f06ad0
restore `lcm(Rational{Int}[]) === 1//1`
LilithHafner Nov 4, 2024
232402d
Fix `lcm([-1])`
LilithHafner Nov 4, 2024
79d3f77
Update comment
LilithHafner Nov 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion base/intfuncs.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,13 @@ gcd(a::T, b::T) where T<:Real = throw(MethodError(gcd, (a,b)))
lcm(a::T, b::T) where T<:Real = throw(MethodError(lcm, (a,b)))

gcd(abc::AbstractArray{<:Real}) = reduce(gcd, abc; init=zero(eltype(abc)))
lcm(abc::AbstractArray{<:Real}) = reduce(lcm, abc; init=one(eltype(abc)))
function lcm(abc::AbstractArray{<:Real})
# Using reduce with init=one(eltype(abc)) is buggy for Rationals.
l = length(abc)
l == 0 && return one(eltype(abc))
l == 1 && return abs(only(abc))
return reduce(lcm, abc)
end

function gcd(abc::AbstractArray{<:Integer})
a = zero(eltype(abc))
Expand Down
7 changes: 7 additions & 0 deletions test/intfuncs.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,13 @@ end

@test lcm(T[2, 4, 6]) ⟷ T(12)
end

# Issue #55379
@test lcm([1//2; 1//2]) === lcm([1//2, 1//2]) === lcm(1//2, 1//2) === 1//2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, this is wrong. Rational numbers are a field, and as such to not have an LCM

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xref #56166

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is perfectly sensible to define lcm on rationals, provided we choose a definition for "multiple" that is based on integers. A definition that is mathematically sensible and in line with current behavior (except for #55379) is

For a signed ring R, m in R is an lcm of a_1, a_2, ... a_n in R iff m is positive and is a multiple of a_1, a_2, ... and a_n and for all other m' in R that are multiples of a_1, a_2, ... and a_n, m' is a multiple of m.

And we define "multiple" as

a is a multiple of b iff there exists an integer n such that b * n = a where * refers to multiplication in the ring of integers, not in the ring of integers mod 2^64.

While there is certainly a case to be made that lcm should throw on rationals, now is not the time to make that case. That time was #33910. lcm has supported rationals under the definition above since 1.4.

I am not aware of any definitions of lcm over the integers or the rationals that would result in returning different answers; though some definitions should throw errors:

Using "m <= m'" instead of "m' is a multiple of m" produces the same results on rationals and integers.

Defining "multiple" based on the existence of a ring element that takes b to a instead of based on an integer that takes b to a is equivalent for the ring of integers. For the rationals, every positive rational is the lcm of every set of rationals which doesn't seem like a particularly useful definition. Folks sometimes (e.g. #27039 (comment), #56166 (comment)) say that lcm(a,b) == 1 for a field which, if we are defining multiple as "all field elements are multiples of all other field elements", is not wrong but we could just as correctly say that lcm(a,b) == 17 for all field elements a and b under that definition which means the right answer for a programming language is an error, not the number 1 nor the number 17.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a different route to obtain the same answer here #55379 (comment)

with the FTA based definition, lcm(1//2, 1//2) == 1//2 . in my opinion it is perfectly clean & consistent this way.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, the FTA definition is nice. AFAICT these two definitions agree in all cases.

For the empty case over the rationals the definition I listed indicates there is no LCM. Using the FTA definition extended to rationals requires computing max over an empty set of integers which also does not exist.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as counterpoint to my interpretation, it would lead to gcd([]) = 1 as well, which is normatively worse than gcd([]) = 0

to be honest though, I think all these choices are basically just convention and I don't think it matters that much that there is some underlying purity determining the convention as long as what we end up with makes sense.

I think the current state of this PR makes good choices for each convention, and afaict it looks like the court of public opinion --- which seems to arise from the recursive definitions --- agrees https://math.stackexchange.com/questions/1755266/gcd-of-an-empty-set https://www.reddit.com/r/learnmath/comments/v9vmfm/whats_the_empty_lcm/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adienes, please take a look at #56166

all these choices are basically just convention and I don't think it matters that much that there is some underlying purity determining the convention as long as what we end up with makes sense

The only way to end up with something that makes sense is to respect the math ("underlying purity").

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

respectfully, I don't think "the math" makes any particularly consistent demands about what to do in these edge cases. and any choice made is essentially just convention (which can and does vary among authors)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As described in the linked issue, there indeed are multiple possible choices here. That doesn't imply that anything goes, though.

Copy link
Contributor

@adienes adienes Nov 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a more charitable interpretation of my comments (rather than "anything" goes) is "a lot of things could go, and I think the choices made here are good ones"

but anyway I guess let's just leave to to triage. I don't typically attend triage but if the discussion comes up, I would upvote the implementation in this PR.

@test gcd(Int[]) === 0
@test lcm(Int[]) === 1
@test gcd(Rational{Int}[]) === 0//1
Comment on lines +197 to +199
Copy link
Contributor

@nsajko nsajko Nov 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These three tests are unnecessarily strict.

Suggested change
@test gcd(Int[]) === 0
@test lcm(Int[]) === 1
@test gcd(Rational{Int}[]) === 0//1
@test gcd(Int[]) == 0
@test gcd(Rational{Int}[]) == 0

Relaxing them should help avoid spurious test failures in the future.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If someone changes the return type of gcd(Int[]) or gcd(Rational{Int}[]), I want a test to fail. Similarly, if someone changes the return type or value of lcm(Int[]), I want a test to fail. Those would be breaking changes that should not be made accidentally.

I'm adding them because I noticed that #56113 would break all three of these new tests but doesn't break any existing tests; I want to make sure that behavior change is only made if folks are aware that we are changing existing behavior.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the return type isn't breaking, as long as the value is correct and the type subtypes the correct abstract type.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making a mathematical operation on Ints that used to return Ints instead return a different type could introduce errors or correctness bugs based on overflow behavior changes or failed conversions and could also introduce significant performance issues through the introduction of type instability in user code.

Changing 1+1 to return an Int8 is breaking even through Int8 has all the same abstract supertypes as Int. A similar change to lcm or gcd would be much less impactful because + is so much more commonly used but would still be breaking.

@test lcm(Rational{Int}[]) === 1//1
end

⟷(a::Tuple{T, T, T}, b::Tuple{T, T, T}) where T <: Union{Int8, UInt8, Int16, UInt16, Int32, UInt32, Int64, UInt64, Int128, UInt128} = a === b
Expand Down