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.

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

Correction of lcm() for Array arguments #56113

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

Conversation

cyanescent
Copy link

@cyanescent cyanescent commented Oct 11, 2024

This is a draft (1st PR attempt!) to try to correct the wrong implementation of the lcm() function with Array argument, cf. bug #55379.

The init argument in the reduce() function was incorrect. It is sufficient to initialize with any value in the Array remove the init argument.
I have also added examples in the doc string, but I don't know if I did it the right way (Array argument may need a separate entry). Also, I don't know yet how to write a test.

Fixes #55379
Fixes #56166

The init argument in the reduce() function was incorrect. It is sufficient to initialize with any value in the Array.
Learn to do a PR: Correction of the gcd() and lcm() of Array.
@nsajko nsajko added the maths Mathematical functions label Oct 11, 2024
@nsajko

This comment was marked as resolved.

@giordano
Copy link
Contributor

@cyanescent thanks for your contribution! When you propose a change which includes a bugfix, could you please always add a relevant test, to demonstrate the bug is indeed fixed and there won't be regressions in the future? Thanks!

@giordano giordano added the needs tests Unit tests are required for this change label Oct 11, 2024
@cyanescent
Copy link
Author

cyanescent commented Oct 11, 2024

Why do you want to make lcm and gcd throw for empty arrays? I think the current behavior for empty input is fine. It agrees with the Fricas CAS, at least. EDIT: it agrees with Giac, too.

Interesting, I guess they have not implemented it for Rational. Or am I missing something?

In my understanding, the lcm and the gcd are functions defined on sets of rational numbers, and related by an inversion of their arguments (with neutral element 1). With this in mind, it makes no sense to define the degenerate cases for one of them as 1 and the other one as 0. If no error is thrown and that the value is not NaN (could be a nice solution too), then there are not many possibilities: either 1 for both or Inf and 0.

In practice, we have just seen that lcm(Int[])=1 as implemented here with the reduce() function yields unacceptable results for the type Array{Rational}. This would make us lean towards lcm(Int[])=Inf. Problem: there is no Inf in the type Int.
1//0 could be a strange solution, returning a Rational. But since gcd() and lcm() throw an error already, why not gcd([]) and lcm([])?

EDIT: it could be a cleaner error though. The proposed patch throws trying to evaluate an empty array at index 1.

@nsajko

This comment was marked as resolved.

@nsajko

This comment was marked as resolved.

@cyanescent
Copy link
Author

The multiplicative identity is the lcm identity, but not the gcd identity.

That is true for integers, but not for Rationals:

julia> lcm(1//2,1)
1//1

julia> gcd(1//2,1)
1//2

I have just tried FriCAS, and I am very puzzled by it's seemingly wrong behaviour for rationals:

(1) -> lcm(1/2,1)

        1
   (1)  -
        2
                                                      Type: Fraction(Integer)
(2) -> gcd(1/2,1)

   (2)  1
                                                      Type: Fraction(Integer)

@nsajko nsajko added needs news A NEWS entry is required for this change minor change Marginal behavior change acceptable for a minor release needs pkgeval Tests for all registered packages should be run with this change bugfix This change fixes an existing bug labels Oct 14, 2024
Copy link
Contributor

@nsajko nsajko left a comment

Choose a reason for hiding this comment

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

Can you also add these lines to the end of the PR message, to make Github link to these issues properly:

Fixes #55379

Fixes #56166

You'll also need to add some tests, I'll try to help you with that soon.

base/intfuncs.jl Outdated Show resolved Hide resolved
@nsajko
Copy link
Contributor

nsajko commented Oct 14, 2024

This test needs to be adjusted or removed:

@test lcm(T[]) T(1)

Co-authored-by: Neven Sajko <s@purelymail.com>
@nsajko
Copy link
Contributor

nsajko commented Oct 15, 2024

@cyanescent: I just realized you made this PR from your master branch. The correct procedure is to create a new branch for each PR (feature branches). Could you do either of these:

  • Fix the branch for this PR (not sure if this is possible TBH)
  • Create a new PR with the same commits but a new branch. The required git commands would be something like this, for example:
    • git checkout -b lcm_rationals_fix  # create new branch
      git checkout master  # switch back to master
      git rebase -i f3a36d74eeb1f8  # delete the PR commits from master interactively
      git checkout lcm_rationals_fix  # switch back to PR branch
      git push origin lcm_rationals_fix  # push the PR branch to your fork

@@ -149,8 +155,8 @@ lcm(a::Real, b::Real, c::Real...) = lcm(a, lcm(b, c...))
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)))
gcd(abc::AbstractArray{<:Real}) = reduce(gcd, abc)
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand the change for lcm, but why also change this in gcd, it works fine, doesn't it?

Suggested change
gcd(abc::AbstractArray{<:Real}) = reduce(gcd, abc)
gcd(abc::AbstractArray{<:Real}) = reduce(gcd, abc; init=zero(eltype(abc)))

Copy link
Author

@cyanescent cyanescent Oct 18, 2024

Choose a reason for hiding this comment

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

Right, but does it make sense?

EDIT: it makes sense if we also define:

lcm(abc::AbstractArray{<:Real}) = reduce(lcm, abc; init=1//zero(eltype(abc)))

or just lcm(T[]) = 1//zero(T), to prevent returning Vector{Rational} when the input is Vector{Int} for non-empty arrays.
cf. #55379 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

gcd(T[]) = zero(T) is correct.

From the docstring,

Greatest common (positive) divisor (or zero if all arguments are zero).

In this case, all arguments are zero (vacuously) so the answer is explicitly documented to be zero.

gcd(abc::AbstractArray{<:Real}) = reduce(gcd, abc; init=zero(eltype(abc)))
lcm(abc::AbstractArray{<:Real}) = reduce(lcm, abc; init=one(eltype(abc)))
gcd(abc::AbstractArray{<:Real}) = reduce(gcd, abc)
lcm(abc::AbstractArray{<:Real}) = reduce(lcm, abc)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
lcm(abc::AbstractArray{<:Real}) = reduce(lcm, abc)
lcm(abc::AbstractArray{<:Integer}) = reduce(lcm, abc, init=one(eltype(abc)))
lcm(abc::AbstractArray{<:Real}) = reduce(lcm, abc)

For integers, 1 is an identity of LCM (because all integers are a multiple of 1) and is also the correct answer for the empty case (see #55379 (comment)).

Copy link
Member

@LilithHafner LilithHafner left a comment

Choose a reason for hiding this comment

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

Thanks for finding and fixing this bug, @cyanescent!

Right now, according to my understanding of the documented behavior, this PR makes lcm & gcd throw more than they need to which is breaking, so we'll need to address that before merging.

Specifically, this makes gcd(Rational{Int}[]) and lcm(Int[]) throw when the previously returned answers (0//1 and 1) were correct.

@nsajko
Copy link
Contributor

nsajko commented Oct 22, 2024

@cyanescent are you still interested in this? Have you tried creating a new PR with a proper "feature" branch?

@LilithHafner
Copy link
Member

It's not standard but it is perfectly fine to make a PR from a master branch. See, for example, #38823, #39002, #39147, #39312, #39331, #39654, #39710, #39737, #39850, #39897, #40253, #40391, #40456, #40633, #40635, #40668, #40806, #40830, #40942, #41045, #41276, #41622, #41758, #41847, #41924, #42140, #42261, #42385, #42500, #42501, #42543, #42661, #43111, #43346, #43427, #43523, #43674, #43699, #43966, #43986, #44376, #44412, #44566, #44709, #44760, #44805, #44999, #45228, #45392, #45793, #46033, #46183, #46410, #47070, #47390, #47564, #47872, #47875, #48073, #48289, #48358, #48536, #48730, #48814, #48973, #49158, #49192, #49343, #49808, #50123, #50126, #50688, #50774, #50843, #51055, #51547, #52252, #52441, #53378, #53592, #53767, #53807, #53861, #53873, #53954, #53981, #54149, #54206, #54283, #54358, #54751, #54955, #54983, #55173, #55315, #56000, #56008, #56122, #56202.

Three's no need to close this PR and restart with a "proper feature branch" or do any git or github fancy operations.

@nsajko
Copy link
Contributor

nsajko commented Oct 23, 2024

OK, I hope this won't confuse NanoSoldier, then.

Here are the tests I wrote:

@testset "`gcd`, `lcm` binary operation properties" begin
    function test_commutativity(f, a, b)
        @test f(a, b) == f(b, a)
    end
    function test_product_property(f, g, a, b)
        @test f(a, b) * g(a, b) == abs(a * b)
    end
    function test_thomaes_function_gcd(f, a, b)
        @test f(a, b) == (abs(a) // denominator(b // a))
    end
    function gcd_array(a, b)
        gcd([a, b])
    end
    function lcm_array(a, b)
        lcm([a, b])
    end
    tested_integers = setdiff(-7:7, 0)
    for (f, g)  ((gcd, lcm), (gcd_array, lcm_array))
        for a  tested_integers
            for b  tested_integers
                test_commutativity(f, a, b)
                test_commutativity(g, a, b)
                test_product_property(f, g, a, b)
                test_thomaes_function_gcd(f, a, b)
            end
        end
        for an  tested_integers
            for ad  tested_integers
                a = an // ad
                for bn  tested_integers
                    for bd  tested_integers
                        b = bn // bd
                        test_commutativity(f, a, b)
                        test_commutativity(g, a, b)
                        test_product_property(f, g, a, b)
                        test_thomaes_function_gcd(f, a, b)
                    end
                end
            end
        end
    end
end

Probably also something like:

@testset "`lcm` of empty" begin
    T = Rational{Int}
    for emp  (Vector{T}(undef, 0), Matrix{T}(undef, 0, 0))
        @test_throws ArgumentError lcm(emp)
    end
end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug maths Mathematical functions minor change Marginal behavior change acceptable for a minor release needs news A NEWS entry is required for this change needs pkgeval Tests for all registered packages should be run with this change needs tests Unit tests are required for this change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

inconsistent semantics of gcd, lcm lcm(::Vector{<:Rational}) gives the wrong answer.
5 participants