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

Avoid use of Uint128 in sampling from a range #10606

Merged
merged 1 commit into from
Mar 27, 2015

Conversation

mschauer
Copy link
Contributor

This is desired because of 2c327d3#commitcomment-10326954

Note that this also allows to use the full entropy of Uint128 random integers to sample a Uint128 range, so I included this immediately. This means for certain cases, rand(uint128(0:i)) will be more efficient (but produce different random numbers.)

cc @rfourquet

mschauer referenced this pull request Mar 23, 2015
Introduces the tryparse method:
- tryparse{T<:Integer}(::Type{T<:Integer},s::AbstractString)
- tryparse(::Type{Float..},s::AbstractString)
- a few variants of the above

And:
- tryparse(Float.., ...) call the corresponding C functions jl_try_strtof, jl_try_substrtof, jl_try_strtod and jl_try_substrtod.
- The parseint, parsefloat, float64_isvalid and float32_isvalid methods wrap the corresponding tryparse methods.
- The jl_strtod, jl_strtof, ... functions are wrappers over the jl_try_str... functions.

This should fix #10498 as well.

Ref: discussions at #9316, #3631, #5704
@JeffBezanson
Copy link
Member

+1
Thanks for this. Very nice improvement. Fewer definitions too!

@ivarne
Copy link
Member

ivarne commented Mar 23, 2015

I love the stackoverflow link in the comment. We should have more of those.

@tkelman
Copy link
Contributor

tkelman commented Mar 23, 2015

I love the stackoverflow link in the comment.

Internet hive mind, solve my problem for me!

Appveyor failure was an intermittent unrelated fluke (#9572), OSX Travis timed out. LGTM.

@mschauer
Copy link
Contributor Author

PS:

Master

 @time rand!(X,0:div(typemax(Uint128),2));
elapsed time: 0.035102936 seconds (272 bytes allocated)

PR

 @time rand!(X,0:div(typemax(Uint128),2));
elapsed time: 0.019820404 seconds (272 bytes allocated)

@StefanKarpinski
Copy link
Member

Woot. That's a lovely speedup.

maxmultiplemix(k::UInt64) = (div((k >> 32 != 0)*0x0000000000000000FFFFFFFF00000000 + 0x0000000100000000, k + (k == 0))*k - 1) % UInt64
# that is 0xFFFF...FFFF if k = typemax(T) - typemin(T) with intentional underflow
# see http://stackoverflow.com/questions/29182036/integer-arithmetic-add-1-to-uint-max-and-divide-by-n-without-overflow
maxmultiple{T<:Unsigned}(k::T) = (div(typemax(T) - k + one(k), k + (k == 0))*k + k - one(k))::T
Copy link
Member

Choose a reason for hiding this comment

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

is there another (slight) improvement to use zero(T) here? (and below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess k == 0 is a julia idiom, the result is of type Bool anyway adding a Boolean does not change the type of k.

Copy link
Member

Choose a reason for hiding this comment

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

Are the one(k) even needed here?

Copy link
Member

Choose a reason for hiding this comment

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

In most situations it's exactly equivalent, or better, because numeric types typically have a specialized methods for comparison with Int. (Eg BigInt)

one(k) is required to not widen the result for UInt8, UInt16 (and usually UInt32).

Copy link
Member

Choose a reason for hiding this comment

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

Ah, right, widening...

JeffBezanson added a commit that referenced this pull request Mar 27, 2015
Avoid use of Uint128 in sampling from a range
@JeffBezanson JeffBezanson merged commit a635856 into JuliaLang:master Mar 27, 2015
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.

7 participants