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

Use standard binomial for Integer arguments #54307

Merged
merged 1 commit into from
Apr 29, 2024

Conversation

barucden
Copy link
Contributor

Before this commit, binomial(Int64(10), Int32(5)) meant the generalized binomial coefficient due to the arguments being of different types, which was in contradiction with the documentation.

Fixes #54296

Before this commit, `binomial(Int64(10), Int32(5))` meant the
generalized binomial coefficient due to the arguments being of different
types, which was in contradiction with the documentation.

Fixes JuliaLang#54296
@tecosaur tecosaur added the maths Mathematical functions label Apr 29, 2024
@oscardssmith oscardssmith added bugfix This change fixes an existing bug merge me PR is reviewed. Merge when all tests are passing labels Apr 29, 2024
@jishnub jishnub merged commit 10db278 into JuliaLang:master Apr 29, 2024
10 checks passed
@jishnub jishnub removed the merge me PR is reviewed. Merge when all tests are passing label Apr 29, 2024
@barucden barucden deleted the int-binomial branch April 29, 2024 19:44
@stevengj
Copy link
Member

stevengj commented Nov 25, 2024

Thanks, I just noticed this. Actually, before the introduction of the generalized binomial coefficient in 1.10 (#48124), the lack of promotion meant that different integer types simply gave an error:

julia1.9> binomial(Int32(5), 3)
ERROR: MethodError: no method matching binomial(::Int32, ::Int64)

so apparently this has always been broken (going back to Julia 1.0 and earlier).

@@ -1205,6 +1205,7 @@ Base.@assume_effects :terminates_locally function binomial(n::T, k::T) where T<:
end
copysign(x, sgn)
end
binomial(n::Integer, k::Integer) = binomial(promote(n, k)...)
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: I would have moved this general definition up, to attach it to the docstring.

Copy link
Member

Choose a reason for hiding this comment

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

Fixed in #56679

dkarrasch pushed a commit that referenced this pull request Nov 26, 2024
Slight tweak to #54307: attach the `binomial(n::Integer, k::Integer)`
method to the corresponding docstring, rather than the narrower
`binomial(n::T, k::T) where {T<:Integer}` method.
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Base.binomial returns Float64 for Integer inputs of different types.
5 participants