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

LilithHafner · 2024-11-02T20:36:16Z

An alternative to #56113 that does not include minor breaking changes to gcd or lcm(::AbstractArray{<:Integer}) .

nsajko · 2024-11-03T01:18:40Z

test/intfuncs.jl

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


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.

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.

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

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.

oscardssmith · 2024-11-03T14:55:20Z

test/intfuncs.jl

@@ -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


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

xref #56166

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.

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.

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.

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/

@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").

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)

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

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.

nsajko

Seems like there should be a decision on the desired semantics, no point in making PRs before that.

LilithHafner · 2024-11-04T17:07:52Z

We decided on semantics in #27039 and #33910. Were this a PR that changed existing non-buggy behavior, I would agree that we need to come to a decision about desired semantics before merging. As is, it's a simple bugfix:

julia> lcm(1//6, 1//3)
1//3 => 1//3

julia> lcm(1//6, 2//3)
2//3 => 2//3

julia> lcm([1//6, 1//3])
1//1 => 1//3

julia> lcm([1//6, 2//3])
2//1 => 2//3

julia> lcm(Rational[])
1//1 => 1//1

We should not wait for a decision on revising previously decided semantics before merging bugfixes for existing semantics.

nsajko · 2024-11-04T17:35:11Z

We decided on semantics in #27039 and #33910.

The decision there was to extend gcd and lcm to nonintegers according to the commensurability-based semantics. This isn't consistent with empty LCM being defined. Here you're trying to double-down on this design bug.

nsajko · 2024-11-04T17:36:12Z

The solution seems simple, make empty LCM throw, do a PkgEval.

adienes · 2024-11-04T18:57:23Z

just to be clear

math.lcm() == 1 --- Python (math stdlib)
lcm([]) == 1 --- Python (SymPy)
lcm([]) == 1 --- Python (SageMath)
lcm([]) == 1 --- PARI / GP
GPT tells me both Mathematica and Maple also follow this convention lcm() == 1, but I do not have access to run either of these languages; maybe someone else can verify?

so Julia would certainly not be alone in this convention and I think calling it "doubling down on a bug" is unnecessarily inflammatory.

There certainly are many languages which choose to error --- it seems that MATLAB (& Octave), R, GMP, numpy, Magma, GAP, probably many more error. but crucially as far as I can see none of these define gcd([]) either.

so I repeat: it's not a "bug" it's a convention we can choose. and if we choose to define the empty gcd then we should surely define also the empty lcm, otherwise Julia really will be completely alone in the design.

LilithHafner · 2024-11-04T19:16:33Z

This PR doesn't change the behavior of lcm(Rational[]) or lcm(Int[]) so IMO this discussion is off topic.

nsajko · 2024-11-04T19:23:45Z

This PR introduces dubious special casing to fix the behavior of LCM for nonintegers. Doing that before good semantics are decided upon could make it more difficult or impossible to fix these semantics in the future. Right now LCM for nonintegers is incorrect anyway, so behavioral changes are more easily possible.

cyanescent · 2024-11-07T16:22:01Z

* GPT tells me both Mathematica and Maple also follow this convention `lcm() == 1`, but I do not have access to run either of these languages; maybe someone else can verify?

For Mathematica, it is left undefined apparently:

LCM[]
LCM called with 0 arguments; 1 or more arguments are expected.

LCM[,]
LCM[Null, Null]

LCM[Null]
LCM[Null]

JuergenWiemers · 2024-11-07T19:42:28Z

GPT tells me both Mathematica and Maple also follow this convention lcm() == 1, but I do not have access to run either of these languages; maybe someone else can verify?

To add another data point, Maple accepts lcm with zero arguments and thinks the answer is

> lcm();
                                                           1

Regarding Mathematica, I'm not sure if LCM[Null] is the right question to ask. Wouldn't that be similar to lcm(nothing) in Julia? I think LCM[{}] == LCM[List[]] (i.e., lcm of the empty list) might be semantically closer to lcm(Integer[]). It makes a bit of a difference: While LCM[Null] is returned unevaluated (which is Mathematica's way to say "I have no idea what to do with this, so here you have it back."), LCM[List[]) returns the empty list:

In[1]:= LCM[List[]] 
Out[1]:= {}

LilithHafner · 2024-11-21T19:37:17Z

For two argument lcm, we should make the lcm of infinity and a finite number throw. Other than that two-arg lcm and gcd are free of bugs (that we know of):

julia> lcm(1//0, 3//2)
3//2 # Should throw

julia> lcm(1//0, 1//0)
1//0 # Correct

julia> lcm(0//1, 2//3)
0//1 # Correct

julia> lcm(1//0, -1//0)
1//0 # Correct

lcm has no identity for Rationals, so lcm(T[]) where T <: Real but not T <: Integer should throw. If someone defines lcm for a non-integer field, they will have to provide the identity.

Other than the non-integer empty case, this pull request correctly generalizes lcm to arrays.

Action steps:

Make two-arg lcm throw when one arg is infinite and the other is finite (separate PR)
Make lcm throw on empty real non-integer arrays (same PR)

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

fd5346d

LilithHafner added maths Mathematical functions bugfix This change fixes an existing bug labels Nov 2, 2024

nsajko approved these changes Nov 3, 2024

View reviewed changes

nsajko reviewed Nov 3, 2024

View reviewed changes

oscardssmith reviewed Nov 3, 2024

View reviewed changes

restore lcm(Rational{Int}[]) === 1//1

4f06ad0

adienes approved these changes Nov 4, 2024

View reviewed changes

nsajko suggested changes Nov 4, 2024

View reviewed changes

LilithHafner added 2 commits November 4, 2024 11:11

Fix lcm([-1])

232402d

Update comment

79d3f77

LilithHafner requested a review from oscardssmith November 4, 2024 19:55

LilithHafner added triage This should be discussed on a triage call backport 1.10 Change should be backported to the 1.10 release backport 1.11 Change should be backported to release-1.11 labels Nov 8, 2024

KristofferC mentioned this pull request Nov 11, 2024

Backports for Julia 1.10.7 #56381

Merged

47 tasks

This was referenced Nov 22, 2024

Backports for Julia 1.11.3 #56652

Merged

Backports for Julia 1.10.8 #56653

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

LilithHafner commented Nov 2, 2024

nsajko Nov 3, 2024 •

edited

Loading

LilithHafner Nov 3, 2024

nsajko Nov 3, 2024

LilithHafner Nov 4, 2024

oscardssmith Nov 3, 2024

nsajko Nov 3, 2024

LilithHafner Nov 4, 2024

adienes Nov 4, 2024

LilithHafner Nov 4, 2024

adienes Nov 4, 2024

nsajko Nov 4, 2024

adienes Nov 4, 2024

nsajko Nov 4, 2024

adienes Nov 4, 2024 •

edited

Loading

nsajko left a comment

LilithHafner commented Nov 4, 2024 •

edited

Loading

nsajko commented Nov 4, 2024

nsajko commented Nov 4, 2024

adienes commented Nov 4, 2024

LilithHafner commented Nov 4, 2024

nsajko commented Nov 4, 2024

cyanescent commented Nov 7, 2024

JuergenWiemers commented Nov 7, 2024

LilithHafner commented Nov 21, 2024

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

Are you sure you want to change the base?

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

Conversation

LilithHafner commented Nov 2, 2024

nsajko Nov 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adienes Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

nsajko left a comment

Choose a reason for hiding this comment

LilithHafner commented Nov 4, 2024 • edited Loading

nsajko commented Nov 4, 2024

nsajko commented Nov 4, 2024

adienes commented Nov 4, 2024

LilithHafner commented Nov 4, 2024

nsajko commented Nov 4, 2024

cyanescent commented Nov 7, 2024

JuergenWiemers commented Nov 7, 2024

LilithHafner commented Nov 21, 2024

nsajko Nov 3, 2024 •

edited

Loading

adienes Nov 4, 2024 •

edited

Loading

LilithHafner commented Nov 4, 2024 •

edited

Loading