Skip to content

Conversation

@PaulXiCao
Copy link
Contributor

@PaulXiCao PaulXiCao commented Jan 20, 2021

Base.factorial is extended in SpecialFunction.jl.

This is type piracy and doesn't follow Julia's style guide.

There is at least one problem with it for which we did not find a solution so far:
The method Base.factorial(x::BigFloat) assumes x::UInt and therefor, we cannot overwrite it using the gamma function for arbitrary BigFloats. There is also an issue #232 for it.

As stated by stevengj at issue #296 one should much rather just call gamma for non-integer arguments directly. Thus, there is no need for an extended factorial function.

@codecov
Copy link

codecov bot commented Jan 20, 2021

Codecov Report

Merging #297 (6b4dfff) into master (281292f) will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #297      +/-   ##
==========================================
+ Coverage   88.17%   88.20%   +0.02%     
==========================================
  Files          11       11              
  Lines        2630     2628       -2     
==========================================
- Hits         2319     2318       -1     
+ Misses        311      310       -1     
Flag Coverage Δ
unittests 88.20% <ø> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/gamma.jl 93.79% <ø> (+0.22%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 281292f...6b4dfff. Read the comment docs.

@simonbyrne
Copy link
Member

The gamma function is also not the only function that interpolates the factorials, e.g. https://en.wikipedia.org/wiki/Hadamard%27s_gamma_function

@stevengj
Copy link
Member

I'm generally fine with this. However, since it is a breaking change, it would be good to survey packages using SpecialFunctions to get a sense for whether they rely on this…

@PaulXiCao
Copy link
Contributor Author

factorial(x::BigFloat)

The following search on github.com
"org:JuliaMath factorial(big"
yields only 5 results and all use BigInt.

Another search
"language:Julia factorial(BigFloat"
yields no results at all.

A more general search
"language:Julia factorial(big"
yields more than 250 results but as I scanned the first couple of them I only found usage of BigInts.


factorial(x::Float)

The search "language:Julia factorial(Float" yields 6 results.
This reposity will actually need to change their code because of this pr.

The search "https://github.com/JulHoltzDevelopers/WavesAndEigenvalues.jl" yields no result.


Those were just the first searches that popped to my mind. But it seems to me like SpecialFunctions.factorial is not used very widely.

@ViralBShah
Copy link
Member

Can we merge this and bump the minor version number to signal the API change?

@stevengj stevengj merged commit 82ec8e2 into JuliaMath:master Nov 17, 2021
stevengj added a commit to stevengj/DFTK.jl that referenced this pull request Nov 23, 2021
The only breaking change in SpecialFunctions 2.0 [was deleting](JuliaMath/SpecialFunctions.jl#297) the `factorial(x::Real) = gamma(x+1)` function, and since you apparently only use `factorial` for integer arguments this shouldn't affect you.
stevengj added a commit to JuliaMath/DecFP.jl that referenced this pull request Nov 23, 2021
The only breaking change in SpecialFunctions 2.0 [was deleting](JuliaMath/SpecialFunctions.jl#297) the `factorial(x::Real) = gamma(x+1)` function, and since we don't use `factorial` this shouldn't affect us.
stevengj added a commit to stevengj/ModifiedHankelFunctionsOfOrderOneThird.jl that referenced this pull request Nov 23, 2021
The only breaking change in SpecialFunctions 2.0 [was deleting](JuliaMath/SpecialFunctions.jl#297) the `factorial(x::Real) = gamma(x+1)` function, and since you don't use `factorial` for non-integer arguments this shouldn't affect you.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants