Make it easier to extend lcm/gcd/gcdx to custom types #34129
Conversation
|
The failure is unrelated to the changes in this PR. |
|
Can this still be in 1.4? |
|
I guess this got overlooked because there is no issue for it, only the comments on the older PR. @StefanKarpinski I would really like to see this in 1.4. Can it be backported? I don’t see it as a feature, but rather a small correction of #33910, so I would argue it can be included in 1.4 regardless of the feature freeze. |
| lcm(a::Real, b::Real) = lcm(promote(a,b)...) | ||
| gcd(a::Real, b::Real...) = gcd(a, gcd(b...)) | ||
| lcm(a::Real, b::Real...) = lcm(a, lcm(b...)) | ||
| gcd(a::T, b::T) where T<:Real = throw(MethodError(gcd, (a,b))) |
There was a problem hiding this comment.
Manually throwing method errors tend to often be a code smell from my experience. What is the reason for it? Is it to avoid an infinite recursion?
There was a problem hiding this comment.
Yes, the reason is to avoid infinite recursion, since some Real types don’t implement gcd/lcm/gcdx (AbstractFloats, for example).
Yes, I just added tests for the promotion of types that are not |
base/intfuncs.jl
Outdated
| lcm(a::Union{Integer,Rational}, b::Union{Integer,Rational}...) = lcm(a, lcm(b...)) | ||
| gcd(a::Real, b::Real) = gcd(promote(a,b)...) | ||
| lcm(a::Real, b::Real) = lcm(promote(a,b)...) | ||
| gcd(a::Real, b::Real...) = gcd(a, gcd(b...)) |
There was a problem hiding this comment.
This should probably be gcd(a::Real, b::Real, c::Real...) to avoid trying to call gcd() --- it would be an error either way but clearer to get a method error for gcd(x::T) where the implementation for T is missing.
There was a problem hiding this comment.
I agree, will change it.
Edit: I changed it (a15c0bb).
|
👍 Ok by me and I'm ok with backporting it as well. |
|
I just noticed that the julia> gcd(5, 2, 1//2)
1//2
julia> gcd([5, 2, 1//2]) # incorrectly returns 1, because gcd(5, 2) == 1
1//1The wrong behavior was introduced in #33910 (by widening the argument type of the method) and is now also fixed as part of this PR (d46752e). I have added a test for it as well. |
|
Would it be possible to separate the bug fix in one commit and the API change in another? As it is, some of these commits need to be squashed, but it would be better to have those changes separate if possible. |
I have added a PR that includes only the bugfix: #34417. What would be the best way to realize the API-change PR? Should I rebase this PR on top of #34417? Now that I typed this, I realized that you only mentioned two different commits, not different PRs. Should I make a PR that includes two commits, first the bugfix and then the API change? |
|
Up to you: you can make a PR that builds on that one or just add a second API commit in that PR. |
This PR now builds on #34417 and implements the API change on top of it. |
As discussed in #33910 (comment), this changes the implementation of
lcm,gcd, andgcdxto use promotion for allRealarguments, thus making it easier to extend it to customRealtypes.