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 pure kwarg to map #71

Merged
merged 45 commits into from
Sep 1, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
6056598
add pure kwarg to map
bkamins Aug 13, 2021
62dcdb8
Update Project.toml
bkamins Aug 13, 2021
6745eef
Update runtests.jl
bkamins Aug 13, 2021
2de3717
Apply suggestions from code review
bkamins Aug 13, 2021
a3de2c3
Apply suggestions from code review
bkamins Aug 13, 2021
6be6bde
Update src/PooledArrays.jl
bkamins Aug 13, 2021
065629d
more efficient map for not-pure case
bkamins Aug 13, 2021
73ff1ed
Apply suggestions from code review
bkamins Aug 13, 2021
819bddf
Update src/PooledArrays.jl
bkamins Aug 13, 2021
4aef8d9
Update src/PooledArrays.jl
bkamins Aug 13, 2021
b3dc12c
Update test/runtests.jl
quinnj Aug 14, 2021
eb5f5ee
Update test/runtests.jl
quinnj Aug 14, 2021
9845443
Apply suggestions from code review
bkamins Aug 14, 2021
ef7b491
Update src/PooledArrays.jl
bkamins Aug 14, 2021
de11e9e
Update src/PooledArrays.jl
bkamins Aug 14, 2021
5d80486
Apply suggestions from code review
bkamins Aug 14, 2021
14bc92b
Update test/runtests.jl
bkamins Aug 14, 2021
75aacf9
Update test/runtests.jl
bkamins Aug 14, 2021
ef02b27
fix promotion
bkamins Aug 14, 2021
ebc4d0a
fix type chceck
bkamins Aug 14, 2021
f1b60b1
Apply suggestions from code review
bkamins Aug 14, 2021
d9a7e4a
Update src/PooledArrays.jl
bkamins Aug 14, 2021
60fdbfc
Update PooledArrays.jl
bkamins Aug 14, 2021
1716888
Apply suggestions from code review
bkamins Aug 14, 2021
8aab260
small changes
bkamins Aug 17, 2021
a6c6e65
Merge branch 'bkamins-patch-2' of https://github.com/JuliaData/Pooled…
bkamins Aug 17, 2021
a59e22a
remove type assertion
bkamins Aug 17, 2021
2f80224
Apply suggestions from code review
bkamins Aug 18, 2021
3a0b7e8
Update src/PooledArrays.jl
bkamins Aug 18, 2021
3a97346
Update src/PooledArrays.jl
bkamins Aug 18, 2021
72a6089
fix signature
bkamins Aug 18, 2021
09c2f20
Apply suggestions from code review
bkamins Aug 20, 2021
ade7029
Apply suggestions from code review
bkamins Aug 20, 2021
6acf2e8
Update src/PooledArrays.jl
bkamins Aug 20, 2021
a059bef
Update src/PooledArrays.jl
bkamins Aug 29, 2021
6a4bfa5
apply suggestions from code review
bkamins Aug 29, 2021
950d914
Apply suggestions from code review
bkamins Aug 29, 2021
5c13102
amend tests
bkamins Aug 29, 2021
4204992
fix Ti issues
bkamins Aug 31, 2021
aba380c
add Base.require_one_based_indexing test in inner constructor
bkamins Aug 31, 2021
60efc6f
fix offest checks for old Julia versions
bkamins Aug 31, 2021
b98ee8f
another fix to Julia 1.0
bkamins Aug 31, 2021
a146fa6
fix typo
bkamins Aug 31, 2021
97fe088
Apply suggestions from code review
bkamins Aug 31, 2021
0b21c63
apply review comments
bkamins Aug 31, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 10 additions & 11 deletions src/PooledArrays.jl
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ const PooledArrOrSub = Union{SubArray{T, N, <:PooledArray{T, R}},
##############################################################################

# Echo inner constructor as an outer constructor
PooledArray(refs::RefArray{RA}, invpool::Dict{T,R}, pool::Vector{T}=_invert(invpool),
@inline PooledArray(refs::RefArray{RA}, invpool::Dict{T,R}, pool::Vector{T}=_invert(invpool),
refcount::Threads.Atomic{Int}=Threads.Atomic{Int}(1)) where {T,R,RA<:AbstractArray{R}} =
PooledArray{T,R,ndims(RA),RA}(refs, invpool, pool, refcount)

Expand All @@ -89,7 +89,7 @@ function _our_copy(x::SubArray{<:Any, 0})
return y
end

function PooledArray(d::PooledArrOrSub)
@inline function PooledArray(d::PooledArrOrSub)
Threads.atomic_add!(refcount(d), 1)
return PooledArray(RefArray(_our_copy(DataAPI.refarray(d))),
DataAPI.invrefpool(d), DataAPI.refpool(d), refcount(d))
Expand Down Expand Up @@ -140,9 +140,8 @@ _widen(::Type{Int32}) = Int64
Freshly allocate `PooledArray` using the given array as a source where each
element will be referenced as an integer of the given type.

`PooledArray` constructor is not type stable.

If `reftype` is not specified, Boolean keyword arguments `signed` and `compress`
If `reftype` is not specified then `PooledArray` constructor is not type stable.
In this case boolean keyword arguments `signed` and `compress`
bkamins marked this conversation as resolved.
Show resolved Hide resolved
determine the type of integer references. By default (`signed=false`), unsigned integers
are used, as they have a greater range.
However, the Arrow standard at https://arrow.apache.org/, as implemented in
Expand All @@ -165,7 +164,7 @@ if all values already exist in the pool.
"""
PooledArray

function PooledArray{T}(d::AbstractArray, r::Type{R}) where {T,R<:Integer}
@inline function PooledArray{T}(d::AbstractArray, r::Type{R}) where {T,R<:Integer}
refs, invpool, pool = _label(d, T, R)

if length(invpool) > typemax(R)
Expand All @@ -176,19 +175,19 @@ function PooledArray{T}(d::AbstractArray, r::Type{R}) where {T,R<:Integer}
return PooledArray(RefArray(refs::Vector{R}), invpool::Dict{T,R}, pool)
end

function PooledArray{T}(d::AbstractArray; signed::Bool=false, compress::Bool=false) where {T}
@inline function PooledArray{T}(d::AbstractArray; signed::Bool=false, compress::Bool=false) where {T}
R = signed ? (compress ? Int8 : DEFAULT_SIGNED_REF_TYPE) : (compress ? UInt8 : DEFAULT_POOLED_REF_TYPE)
refs, invpool, pool = _label(d, T, R)
return PooledArray(RefArray(refs), invpool, pool)
end

PooledArray(d::AbstractArray{T}, r::Type) where {T} = PooledArray{T}(d, r)
PooledArray(d::AbstractArray{T}; signed::Bool=false, compress::Bool=false) where {T} =
@inline PooledArray(d::AbstractArray{T}, r::Type) where {T} = PooledArray{T}(d, r)
@inline PooledArray(d::AbstractArray{T}; signed::Bool=false, compress::Bool=false) where {T} =
PooledArray{T}(d, signed=signed, compress=compress)

# Construct an empty PooledVector of a specific type
PooledArray(t::Type) = PooledArray(Array(t,0))
PooledArray(t::Type, r::Type) = PooledArray(Array(t,0), r)
@inline PooledArray(t::Type) = PooledArray(Array(t,0))
@inline PooledArray(t::Type, r::Type) = PooledArray(Array(t,0), r)

##############################################################################
##
Expand Down
12 changes: 12 additions & 0 deletions test/runtests.jl
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,13 @@ end
@test PooledArrays.fast_sortable(v3) == PooledArray([1, 3, 2, 4])
@test isbitstype(eltype(PooledArrays.fast_sortable(v3)))
Base.Order.Perm(Base.Order.Forward, v3).data == PooledArray([1, 3, 2, 4])

for T in (Int8, Int16, Int32, Int64, UInt8, UInt16, UInt32, UInt64)
@inferred PooledArray([1, 2, 3], T)
end
for signed in (true, false), compress in (true, false)
@test_throws ErrorException @inferred PooledArray([1, 2, 3])
end
Copy link
Member

Choose a reason for hiding this comment

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

Checking this is a bit extreme: would it be a problem if inference managed to get this right? :-D

Copy link
Member Author

Choose a reason for hiding this comment

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

The objective is different - the moment the inference will get it right we will know it.
OTOH - advanced users reading the tests will know that we have a problem here (BTW - I have forgotten to add kwargs in the call I will fix this)

end

@testset "pool non-copying constructor and copy tests" begin
Expand Down Expand Up @@ -552,4 +559,9 @@ end
y = map(f(), x)
@test y == fill(-1)
@test typeof(y) === PooledArray{Int, Int8, 0, Array{Int8, 0}}

for signed in (true, false), compress in (true, false), len in (1, 100, 1000)
x = PooledArray(fill(1, len), signed=true, compress=true);
@inferred PooledVector{Int, Int, Vector{Int}} map(identity, x)
bkamins marked this conversation as resolved.
Show resolved Hide resolved
end
end