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

Rename lgamma to log(abs)gamma #156

Merged
merged 14 commits into from
May 1, 2019
Merged

Rename lgamma to log(abs)gamma #156

merged 14 commits into from
May 1, 2019

Conversation

Sumegh-git
Copy link
Contributor

@Sumegh-git Sumegh-git commented Apr 10, 2019

Ping @simonbyrne .
If the changes are valid and right to do, I'll add another commit for doing the same for lbeta and lbinomial, or maybe I can make those changes in a separate PR.
Fixes #154

Copy link
Member

@simonbyrne simonbyrne left a comment

Choose a reason for hiding this comment

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

So I noticed that logabsdet actually returns a tuple of log(abs(det(x))), sign(det(x)). I'm not sure if we want to do the same thing here, or would it be better as a separate function?

src/gamma.jl Outdated Show resolved Hide resolved
src/gamma.jl Outdated Show resolved Hide resolved
src/gamma.jl Outdated Show resolved Hide resolved
src/gamma.jl Outdated Show resolved Hide resolved
src/gamma.jl Outdated Show resolved Hide resolved
@mschauer mschauer changed the title Renamed lgamma Rename lgamma to loggamma Apr 13, 2019
@mschauer mschauer changed the title Rename lgamma to loggamma Rename lgamma to log(abs)gamma Apr 13, 2019
src/gamma.jl Show resolved Hide resolved
src/deprecated.jl Outdated Show resolved Hide resolved
src/gamma.jl Outdated Show resolved Hide resolved
src/gamma.jl Outdated Show resolved Hide resolved
src/gamma.jl Outdated Show resolved Hide resolved
src/gamma.jl Show resolved Hide resolved
src/gamma.jl Outdated Show resolved Hide resolved
src/gamma.jl Outdated Show resolved Hide resolved
src/gamma.jl Outdated Show resolved Hide resolved
@Sumegh-git
Copy link
Contributor Author

Sumegh-git commented Apr 27, 2019

@simonbyrne made the changes!!

src/deprecated.jl Outdated Show resolved Hide resolved
src/gamma.jl Outdated Show resolved Hide resolved
src/gamma.jl Outdated Show resolved Hide resolved
src/gamma.jl Outdated Show resolved Hide resolved
src/gamma.jl Outdated Show resolved Hide resolved
src/gamma.jl Outdated Show resolved Hide resolved
@Sumegh-git
Copy link
Contributor Author

Sumegh-git commented Apr 28, 2019

@simonbyrne made the other changes except for removing logabsgamma(x::Number) since that will break lbeta and beta tests. They previously called lgamma_r which worked with Complex{Float} but now we deprecated it to logabsgamma which if I remove (x::Number) will break it.

Also I can't replace lbeta calls with loggamma instead of logabsgamma beacause it also requires the sign(gamma(x)) which is given as a tuple.

So, one possible fix can be to make loggamma return a tuple too. Any suggestions?
Or rather explicitly find the sign of gamma(x) and use that in computation of lbeta instead of tuple?

@Sumegh-git
Copy link
Contributor Author

@simonbyrne issue is resolved I guess. Renamed lbeta accordingly as well.

src/deprecated.jl Outdated Show resolved Hide resolved
src/gamma.jl Outdated Show resolved Hide resolved
src/gamma.jl Outdated Show resolved Hide resolved
Copy link
Member

@simonbyrne simonbyrne left a comment

Choose a reason for hiding this comment

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

Overall, this looks good to me. I'll give it a day or two to see if anyone else has comments.

@Sumegh-git
Copy link
Contributor Author

@simonbyrne this is fine now! Updated the docs too.

@simonbyrne simonbyrne merged commit 23ec9f1 into JuliaMath:master May 1, 2019
@simonbyrne
Copy link
Member

Thanks for your persistence here, looks great!

simonbyrne added a commit that referenced this pull request May 1, 2019
Ensure consistency with #156.
@Sumegh-git Sumegh-git deleted the gamma branch May 2, 2019 09:29
simonbyrne added a commit that referenced this pull request May 2, 2019
Ensure consistency with #156.
simonbyrne added a commit that referenced this pull request May 7, 2019
Ensure consistency with #156.
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.

lgamma() wrong complex result with real argument
3 participants