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

Change fallback unsafe_gamma for x::Real to x<:AbstractFloat. Add #28

Merged
merged 4 commits into from
Aug 9, 2020
Merged

Conversation

cgeoga
Copy link
Contributor

@cgeoga cgeoga commented Aug 8, 2020

This PR tweaks the behavior of unsafe_gamma to avoid stack overflow errors with ForwardDiff.jl. The method unsafe_gamma(x::Real) = unsafe_gamma(float(x)) has been given different type bounds as that particular dispatch method seems prone to these issues (ref JuliaLang/julia#26552). Further adds a generic fallback to gamma from SpecialFunctions.jl.

This design decision may not be coherent with other choices here, like manually implementing unsafe_gamma(x::Dual), so please let me know if this is not acceptable. There is some discussion available on the issue here. This method avoids needing to make ForwardDiff or DiffRules a new dependency.

fallback for generic argument to default gamma function.
@codecov
Copy link

codecov bot commented Aug 8, 2020

Codecov Report

Merging #28 into master will decrease coverage by 2.35%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #28      +/-   ##
==========================================
- Coverage   79.88%   77.53%   -2.36%     
==========================================
  Files           6        6              
  Lines         900      899       -1     
==========================================
- Hits          719      697      -22     
- Misses        181      202      +21     
Impacted Files Coverage Δ
src/specialfunctions.jl 53.99% <0.00%> (-6.52%) ⬇️
src/gauss.jl 53.00% <0.00%> (-1.00%) ⬇️

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 720e95d...9456171. Read the comment docs.

@MikaelSlevinsky
Copy link
Collaborator

Hey thanks!

If you could bump the version when you're happy, then I could register a new release upon merging.

@cgeoga
Copy link
Contributor Author

cgeoga commented Aug 9, 2020

Thanks so much! I just pushed up a new tag. That is what it means to bump the version, right? Sorry---first time doing this.

@MikaelSlevinsky
Copy link
Collaborator

I mean change this line

version = "0.3.1"

to version = "v0.3.2".

Then, I can use the @JuliaRegistrator GitHub bot to register the new version in the General registry (and then a tag/release will also be made to this repository).

@JuliaMath JuliaMath deleted a comment from JuliaRegistrator Aug 9, 2020
@cgeoga
Copy link
Contributor Author

cgeoga commented Aug 9, 2020

Oof, apologies. Just pushed up a commit that does that.

Also, I played with the dispatch one last time, removing unsafe_gamma(z<:AbstractFloat) = unsafe_gamma(z) and unsafe_gamma(z<:Complex) = gamma(z), as those should both be covered by the fallback unsafe_gamma(z) = gamma(z). Assuming that is agreeable, I think that should do it.

EDIT: Very sorry for so many tiny commits. I could have sworn I removed the commented line before, but I didn't. I fixed that in yet another commit.

@MikaelSlevinsky MikaelSlevinsky merged commit f4461b4 into JuliaMath:master Aug 9, 2020
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.

2 participants