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

Add next_k_array! and k_array_rank. #199

Merged
merged 3 commits into from
Jan 10, 2018

Conversation

shizejin
Copy link
Member

@shizejin shizejin commented Jan 6, 2018

Add next_k_array! and k_array_rank utilities, mimicking the counterpart Python version implemented by @oyamad. It is mainly copying and pasting, and modifying a little part (let index start from 1 rather than 0).

@oyamad
Copy link
Member

oyamad commented Jan 6, 2018

Thanks @shizejin

k_array_rank is not type stable (the return type changes according to the input value). Run it with @code_warntype.

One possibility is:

function k_array_rank(T::Type{<:Integer}, a::Vector{<:Integer})
    k = length(a)
    idx = one(T)
    for i = 1:k
        idx += binomial(T(a[i])-one(T), T(i))
    end
    return idx
end

k_array_rank(a::Vector{<:Integer}) = k_array_rank(Int, a)
n, k = 100, 50
a = collect((n-k+1):n)
k_array_rank(BigInt, a)
100891344545564193334812497256
k_array_rank(a)
InexactError()

I don't know if this is "Julian": @sglyon ?

@sglyon
Copy link
Member

sglyon commented Jan 6, 2018

That proposal is quite Julian. It would be great if there was a way to make it "just work" without throwing an error, but unfortunately I don't think there is.

As long as you don't expect the InexactError to arise often in practice, the proposal by @oyamad has my vote.

@oyamad
Copy link
Member

oyamad commented Jan 6, 2018

@sglyon Thanks.

@shizejin We may add a note about the possibility of an error as well as a sufficient condition for it not to occur, as in the Python version.

@shizejin
Copy link
Member Author

shizejin commented Jan 6, 2018

@oyamad Sure, I will modify this.

In this case, in the tournament game, should we set a threshold of the pair of n and k to determine whether use k_array_rank(Int, a) or k_array_rank(BigInt, a)? Or do we just use k_array_rank(Int, a) as the InexactError case happens almost impossibly?

@oyamad
Copy link
Member

oyamad commented Jan 7, 2018

@shizejin See this.

The size of NumPy arrays is limited by the range of np.intp. Maybe Julia arrays are limited by the range of Csize_t (= UInt)?

@shizejin
Copy link
Member Author

shizejin commented Jan 7, 2018

@oyamad It seems that the InexactError of binomial happens when the function tries to convert the calculated result back to the type T of the arguments n and k. Therefore, follow the style of Python code, I think we can add these to Julia version of tournament game generator.

if binomial(BigInt(n), BigInt(k)) > typemax(typeof(n))
    error("Overflow! Maximum allowed size of $(typeof(n)) exceeded")
end

What do you think?

@oyamad
Copy link
Member

oyamad commented Jan 7, 2018

Are we talking about this PR, or the tournament game? About the latter let's discuss at Games.jl.

@codecov-io
Copy link

codecov-io commented Jan 7, 2018

Codecov Report

Merging #199 into master will increase coverage by 0.09%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #199      +/-   ##
==========================================
+ Coverage   91.02%   91.12%   +0.09%     
==========================================
  Files          24       24              
  Lines        1693     1712      +19     
==========================================
+ Hits         1541     1560      +19     
  Misses        152      152
Impacted Files Coverage Δ
src/util.jl 96.59% <100%> (+0.93%) ⬆️

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 84b961b...a645c2d. Read the comment docs.

Copy link
Member

@oyamad oyamad left a comment

Choose a reason for hiding this comment

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

Looks good to me.

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