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

binomial no longer uses Float64 internally. #6163

Merged
merged 1 commit into from
Mar 15, 2014
Merged

Conversation

jiahao
Copy link
Member

@jiahao jiahao commented Mar 14, 2014

Replaces the old accumulation step, using x=*(nn::Float64, rr::Float64),
with x::T=div(widemul(x,nn), rr) as suggested by @JeffBezanson.

Previously, binomial(54,23) computed the wrong answer. The new thresholds are:

type arguments failure mode
Int32 (34,16) integer overflow (throws OverflowError())
Int64 (67,29) integer overflow (throws OverflowError())
Int128 (131,63) integer overflow (throws OverflowError())

Fix #6154.

rr += 1
nn += 1
end
sgn*iround(T,x)
x < 0 && throw(OverflowError())
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure x will always be negative if there's an overflow? That doesn't seem plausible.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is sufficient but not necessary. Maybe this should be in the inner loop? x always starts positive and is multiplied and divided by positive numbers, so the only way to make it negative is due to overflow. I don't think it is possible in the inner loop to overflow and remain positive because the factors aren't that large.

@JeffBezanson
Copy link
Member

I still don't think x<0 is a correct test for overflow. The only thing I can think of is

temp = div(widemul(x, nn), rr)
x = temp
x != temp && throw(OverflowError())

@lbenet
Copy link
Contributor

lbenet commented Mar 15, 2014

Using your code above I still get InexactError for binomial(67,30).

Forcing x::T as you suggested #6154, so the code becomes:

temp = div(widemul(x,nn), rr)
x::T = temp
x != temp && throw(OverflowError())

renders the OverflowError as wanted.

@lbenet
Copy link
Contributor

lbenet commented Mar 15, 2014

The code seems ok, though I don't think you need/use oldx

Replaces the old accumulation step, using x=*(nn::Float64, rr::Float64)
with x::T=xt=div(widemul(x,nn), rr). Also tests for integer overflow.

Fix #6154.
@jiahao
Copy link
Member Author

jiahao commented Mar 15, 2014

Thanks for catching that.

JeffBezanson added a commit that referenced this pull request Mar 15, 2014
binomial no longer uses Float64 internally.
@JeffBezanson JeffBezanson merged commit 28c70e9 into master Mar 15, 2014
@jiahao jiahao deleted the cjh/fix-6154 branch March 15, 2014 20:27
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.

Inaccurate (buggy) results with binomial
3 participants