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.

Sign up for GitHub

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

size field: allow Integer values of singleton type as elements #107

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

size field: allow Integer values of singleton type as elements #107

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d7a3613
`size` field: allow `Integer` values of singleton type as elements
nsajko Jan 26, 2025
cee5106
`reshape`
nsajko Feb 12, 2025
77ee0ca
do `check_size_type` in `collect_as`, test error path
nsajko Feb 12, 2025
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
107 changes: 94 additions & 13 deletions src/FixedSizeArray.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,39 @@
FixedSizeArray, FixedSizeVector, FixedSizeMatrix,
FixedSizeArrayDefault, FixedSizeVectorDefault, FixedSizeMatrixDefault

struct FixedSizeArray{T,N,Mem<:DenseVector{T}} <: DenseArray{T,N}
function normalized_dim_int(x::Integer)
Int(x)::Int
end

function normalized_dim_integer(x::Integer)
if Base.issingletontype(typeof(x))
x

Check warning on line 11 in src/FixedSizeArray.jl

View check run for this annotation

Codecov / codecov/patch

src/FixedSizeArray.jl#L11 

Added line #L11 was not covered by tests
else
normalized_dim_int(x)
end
end

const NormalizedDim = Union{typeof(normalized_dim_int), typeof(normalized_dim_integer)}

function normalized_dims(::NormalizedDim, ::Tuple{})
()
end
function normalized_dims(normalized_dim::NormalizedDim, x::Tuple{Integer, Vararg{Integer}})
i_tail = Base.tail(x)
o_tail = normalized_dims(normalized_dim, i_tail)
f = normalized_dim(first(x))
(f, o_tail...)
end
function normalized_dims(::NormalizedDim, x::Tuple{Int, Vararg{Int}})
x

Check warning on line 29 in src/FixedSizeArray.jl

View check run for this annotation

Codecov / codecov/patch

src/FixedSizeArray.jl#L28-L29 

Added lines #L28 - L29 were not covered by tests
end

struct FixedSizeArray{T,N,Mem<:DenseVector{T},Size<:NTuple{N,Integer}} <: DenseArray{T,N}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this turning FixedSizeArray too close to StaticArray by having the shape in the type domain?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are some similarities with both HybridArrays.jl (allows specifying an arbitrary mix of statically and dynamically sized axes) and SizedArray from StaticArrays.jl (allows specifying statically sized axes for a wrapped dynamically-sized array). The main difference (both a benefit and limitation) over both HybridArrays.jl and StaticArrays.jl is that FixedSizeArray is dense (subypes DenseArray). And StaticArrays requires all axes to be statically sized, of course.

In case it's not clear, this PR doesn't remove any existing features, it just generalizes the current design.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wasn't concerned about removing features but about putting too much information in the type domain. That's a recipe for skyrocketing compile times.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For what it's worth I think the test suite takes about the same time to execute as before. So, as long as all axes are dynamically sized, I don't expect compile time regressions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure you'd see much of a difference in tests here where we mostly do different operations. The problem is more when you do same operations on arrays of different sizes: the generated code is very similar, but each of them triggers full recompilation since the type is different. Of course putting more information in the type domain is more flexible (you can even dispatch on the exact shape of the array which in certain cases can be very convenient) but that comes at a cost. The constant size field isn't as flexible, but it still enables some compile-time optimisations (e.g. when constant-propagation does its job).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The problem is more when you do same operations on arrays of different sizes: the generated code is very similar, but each of them triggers full recompilation since the type is different.

That's definitely true, with this PR it's possible to create lots of distinct types, causing lots of work for the compiler. However in my mind if that situation happens it is almost surely what the user wants, no? The potential to unintentionally construct a FixedSizeArray with a statically sized axis seems small, given that static integers are rarely used.

Do I understand correctly that you're worried about this unintentional construction/type proliferation? If that's it, it should be possible to further diminish the possibility of unintentionally constructing an array with a statically sized axis: we could, for example, require such construction to specify the Size type parameter explicitly. That is, a constructor type that explicitly features a Size type parameter with singleton type constituents would be the only way to construct a FixedSizeArray with a non-Int-sized axis.

Does that seem like an improvement, and perhaps acceptable?

Copy link

@PatrickHaecker PatrickHaecker Feb 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Update: Sorry, you can ignore this comment. I understood that Size can really be used to store the size information and I need to think this through.

Isn't this turning FixedSizeArray too close to StaticArray by having the shape in the type domain?

I have the feeling, that there is a misunderstanding, because I misread the code change in the beginning. Even after this change Size does not contain the size information, i.e. FixedSizeArray does not contain the shape in the type domain. This is in contrast to StaticArrays, although StaticArrays uses the same identifier Size for the shape information in its documentation.

What Size in this PR contains is the type of the size field for each dimension, e.g. only (a Tuple containing) Int8 for a short vector if the user specifies this. And while I now see the logic in the PR between the identifiers size and Size, I suggest to rename Size into SizeType or SizeT or something similar to avoid this confusion.

And while lifting size in the type domain could indeed trigger a lot of additional compilations (and would be best done in a new type in my opinion if anyone planned this), I think putting its type information into the type domain is reasonable. As default Int will be used as before, so there are no additional method compilations for existing users.

I expect only few different types which will be used in a typical program, i.e. a lot of combinations like {Int128, UInt8, UInt32} are unlikely to be ever seen in reality. But it's great that they are available if need arises without any cost when not used. As an NTuple is used, this type is not supported, so it would only be, e.g., {Int8, Int8, Int8}.

Copy link
Collaborator

@giordano giordano Feb 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, I just realised I did misunderstand the new type parameter: it isn't the size tuple itself, but its type:

julia> FixedSizeArray{Float64}(undef, 0, 0)
0×0 FixedSizeArray{Float64, 2, Memory{Float64}, Tuple{Int64, Int64}}

My concern was unwarranted (I read the code wrong and thought it'd move the tuple itself to the type domain), but I'd like @oscardssmith opinion on this. The type parameters are becoming a little unwieldy here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we probably want to keep the size as Int. the overhead should be low.

mem::Mem
size::NTuple{N,Int}
global function new_fixed_size_array(mem::DenseVector{T}, size::NTuple{N,Int}) where {T,N}
new{T,N,typeof(mem)}(mem, size)
size::Size
global function new_fixed_size_array(mem::DenseVector{T}, size::NTuple{N,Integer}) where {T,N}
s = normalized_dims(normalized_dim_integer, size)
new{T,N,typeof(mem),typeof(s)}(mem, s)
end
end

Expand All @@ -23,25 +51,38 @@
Union{
# 0 fixed parameters
Type{FixedSizeArray{T, N, Mem{T}} where {T, N}},
Type{FixedSizeArray{T, N, Mem{T}, NTuple{N, Int}} where {T, N}},
Type{FixedSizeArray{T, N, Mem{T}, Size} where {T, N, Size <: NTuple{N, Integer}}},
# 1 fixed parameter
Type{FixedSizeArray{T, N, Mem{T}} where {N}} where {T},
Type{FixedSizeArray{T, N, Mem{T}} where {T}} where {N},
Type{FixedSizeArray{T, N, Mem{T}, NTuple{N, Int}} where {N}} where {T},
# 2 fixed parameters
Type{FixedSizeArray{T, N, Mem{T}}} where {T, N},
Type{FixedSizeArray{T, N, Mem{T}, Size} where {T}} where {N, Size <: NTuple{N, Integer}},
# 3 fixed parameters
Type{FixedSizeArray{T, N, Mem{T}, Size}} where {T, N, Size <: NTuple{N, Integer}},
}
end
special_storage_types = (Vector, optional_memory...)
Union{
# 0 fixed parameters
Type{FixedSizeArray},
Type{FixedSizeArray{T, N, Mem, NTuple{N, Int}} where {T, N, Mem <: DenseVector{T}}},
# 1 fixed parameter
Type{FixedSizeArray{T}} where {T},
Type{FixedSizeArray{T, N} where {T}} where {N},
Type{FixedSizeArray{T, N, Mem, NTuple{N, Int}} where {N, Mem <: DenseVector{T}}} where {T},
# 2 fixed parameters
Type{FixedSizeArray{T, N}} where {T, N},
Type{FixedSizeArray{T, N, Mem} where {N}} where {T, Mem <: DenseVector{T}},
Type{FixedSizeArray{T, N, Mem, NTuple{N, Int}} where {N}} where {T, Mem <: DenseVector{T}},
Type{FixedSizeArray{T, N, Mem, Size} where {T, Mem <: DenseVector{T}}} where {N, Size <: NTuple{N, Integer}},
# 3 fixed parameters
Type{FixedSizeArray{T, N, Mem}} where {T, N, Mem <: DenseVector{T}},
Type{FixedSizeArray{T, N, Mem, Size} where {Mem <: DenseVector{T}}} where {T, N, Size <: NTuple{N, Integer}},
# 4 fixed parameters
Type{FixedSizeArray{T, N, Mem, Size}} where {T, N, Mem <: DenseVector{T}, Size <: NTuple{N, Integer}},
# special cases depending on the underlying storage type
map(f, special_storage_types)...,
}
Expand All @@ -68,10 +109,12 @@
default_underlying_storage_type
end
for Mem ∈ (Vector, optional_memory...)
FSA = FixedSizeArray{T, N, Mem{T}} where {T, N}
FSA = FixedSizeArray{T, N, Mem{T}, Size} where {T, N, Size <: NTuple{N, Integer}}
t_fsa = Union{
Type{FSA},
Type{FSA{T, N} where {T}} where {N},
Type{FSA{T, N, Size} where {T}} where {N, Size <: NTuple{N, Integer}},
Type{FSA{T, N, NTuple{N, Int}} where {T, N}},
}
@eval begin
function parent_type_with_default(::$t_fsa)
Expand All @@ -91,13 +134,43 @@
size
end

function check_size_type(::Type{FSA}) where {
N,
Size <: NTuple{N, Integer},
FSA <: (FixedSizeArray{T, N, Mem, Size} where {T, Mem <: DenseVector{T}}),
}
for t ∈ fieldtypes(Size)
if !Base.issingletontype(t) && !(t <: Int)
throw(ArgumentError("size tuple fields must be either of singleton type or `Int`"))
end
end
FSA

Check warning on line 147 in src/FixedSizeArray.jl

View check run for this annotation

Codecov / codecov/patch

src/FixedSizeArray.jl#L147 

Added line #L147 was not covered by tests
end
function check_size_type(::Type{FSA}) where {FSA <: FixedSizeArray}
FSA

Check warning on line 150 in src/FixedSizeArray.jl

View check run for this annotation

Codecov / codecov/patch

src/FixedSizeArray.jl#L149-L150 

Added lines #L149 - L150 were not covered by tests
end

function convert_array_size_tuple(
::Type{<:(FixedSizeArray{T, N, Mem, Size} where {T, Mem <: DenseVector{T}})},
size::Tuple{Vararg{Integer}},
) where {N, Size <: NTuple{N, Integer}}
convert(Size, size)
end
function convert_array_size_tuple(::Type{<:FixedSizeArray}, size::Tuple{Vararg{Integer}})
size

Check warning on line 160 in src/FixedSizeArray.jl

View check run for this annotation

Codecov / codecov/patch

src/FixedSizeArray.jl#L159-L160 

Added lines #L159 - L160 were not covered by tests
end

function undef_constructor(::Type{FSA}, size::Tuple{Vararg{Integer}}) where {T, FSA <: FixedSizeArray{T}}
size = check_ndims(FSA, size)
s = map(Int, size)
Mem = parent_type_with_default(FSA)
len = checked_dims(s)
fsa_type = check_constructor_is_allowed(FSA)
fsa_type = check_size_type(fsa_type)
size = check_ndims(fsa_type, size)
siz = convert_array_size_tuple(fsa_type, size)
siz_integer = normalized_dims(normalized_dim_integer, siz)
siz_int = normalized_dims(normalized_dim_int, siz_integer)
Mem = parent_type_with_default(fsa_type)
len = checked_dims(siz_int)
mem = Mem(undef, len)
new_fixed_size_array(mem, s)
new_fixed_size_array(mem, siz_integer)
end

function (::Type{FSA})(::UndefInitializer, size::Tuple{Vararg{Integer}}) where {T, FSA <: FixedSizeArray{T}}
Expand Down Expand Up @@ -330,14 +403,22 @@

# `reshape`: specializing it to ensure it returns a `FixedSizeArray`

function Base.reshape(a::FixedSizeArray, size::(NTuple{N,Int} where {N}))
len = checked_dims(size)
function reshape_fsa(a::FixedSizeArray, size::Tuple{Vararg{Integer}})
size_int = normalized_dims(normalized_dim_int, size)
len = checked_dims(size_int)
if length(a) != len
throw(DimensionMismatch("new shape not consistent with existing array length"))
end
new_fixed_size_array(a.mem, size)
end

function Base.reshape(a::FixedSizeArray, size::Tuple{Vararg{Int}})
reshape_fsa(a, size)
end
function Base.reshape(a::FixedSizeArray, size::Tuple{Integer, Vararg{Integer}})
reshape_fsa(a, size)
end

# `iterate`: the `AbstractArray` fallback doesn't perform well, so add our own methods

function Base.iterate(a::FixedSizeArray)
Expand All @@ -347,6 +428,6 @@
iterate(a.mem, state)
end

const FixedSizeArrayDefault = FixedSizeArray{T, N, default_underlying_storage_type{T}} where {T, N}
const FixedSizeArrayDefault = FixedSizeArray{T, N, default_underlying_storage_type{T}, NTuple{N, Int}} where {T, N}
const FixedSizeVectorDefault = FixedSizeArrayDefault{T, 1} where {T}
const FixedSizeMatrixDefault = FixedSizeArrayDefault{T, 2} where {T}
1 change: 1 addition & 0 deletions src/collect_as.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ function collect_as(::Type{FSA}, iterator) where {FSA<:FixedSizeArray}
throw(ArgumentError("iterator is infinite, can't fit infinitely many elements into a `FixedSizeArray`"))
end
T = check_constructor_is_allowed(FSA)
T = check_size_type(T)
mem = parent_type_with_default(T)
output_dimension_count = checked_dimension_count_of(T, size_class)
fsv = if (
Expand Down
68 changes: 62 additions & 6 deletions test/runtests.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,21 @@ using FixedSizeArrays
using OffsetArrays: OffsetArray
import Aqua

struct Three <: Integer end
struct Seven <: Integer end
Base.Int(::Three) = 3
Base.Int(::Seven) = 7
Base.promote_rule(::Type{Int}, ::Type{Three}) = Int
Base.promote_rule(::Type{Int}, ::Type{Seven}) = Int
Base.promote_rule(::Type{Three}, ::Type{Int}) = Int
Base.promote_rule(::Type{Seven}, ::Type{Int}) = Int
Base.zero(::Union{Three, Seven}) = false
Base.zero(::Type{<:Union{Three, Seven}}) = false
Base.one(::Union{Three, Seven}) = true
Base.one(::Type{<:Union{Three, Seven}}) = true
Base.iszero(::Union{Three, Seven}) = false
Base.isone(::Union{Three, Seven}) = false

# Check if the compilation options allow maximum performance.
const build_is_production_build_env_name = "BUILD_IS_PRODUCTION_BUILD"
const build_is_production_build = let v = get(ENV, build_is_production_build_env_name, "true")
Expand Down Expand Up @@ -49,7 +64,7 @@ end
# helpers for constructing the type constructors

function fsa(vec_type::Type{<:DenseVector})
FixedSizeArray{T,N,vec_type{T}} where {T,N}
FixedSizeArray{T,N,vec_type{T},NTuple{N,Int}} where {T,N}
end
function fsm(vec_type::Type{<:DenseVector})
fsa(vec_type){T,2} where {T}
Expand Down Expand Up @@ -80,6 +95,51 @@ end
end
end

@testset "various dim size types" begin
for Mem ∈ (Vector, ((@isdefined Memory) ? (Memory,) : ())...)
for siz ∈ ((3, 7), (3, Seven()), (Three(), 7), (Three(), Seven()))
elt = Float32
Mem_elt = Mem{elt}
requested_type = FixedSizeArray{elt, <:Any, Mem_elt}
return_type = FixedSizeArray{elt, length(siz), Mem_elt, typeof(siz)}
for args ∈ ((undef, siz), (undef, siz...))
test_inferred(requested_type, return_type, args)
a = FixedSizeArray{elt}(args...)
@test siz === @inferred size(a)
end
let a = requested_type(undef, 3 * 7)
for args ∈ ((a, siz), (a, siz...))
test_inferred(reshape, return_type, args)
end
end
end
let a = FixedSizeArray{Float32}(undef, (big(3), big(7)))
@test (3, 7) === @inferred size(a)
end
let a = FixedSizeArray{Float32}(undef, (big(3), Seven()))
@test (3, Seven()) === @inferred size(a)
end
for WrongInt ∈ (UInt, BigInt, Bool)
for m ∈ (1, UInt(1), big(1), true)
let elt = Float32
requested_type = FixedSizeVector{elt, Mem{elt}, Tuple{WrongInt}}
@test_throws ArgumentError requested_type(undef, m)
@test_throws ArgumentError requested_type(undef, (m,))
@test_throws ArgumentError requested_type([7])
@test_throws ArgumentError collect_as(requested_type, [7])
end
end
end
end
end

@testset "ndims type safety" begin
@test_throws TypeError FixedSizeArray{<:Any, Int}
@test_throws TypeError FixedSizeArray{<:Any, Union{}}
@test_throws TypeError FixedSizeArray{<:Any, Int32(0)}
@test_throws Exception FixedSizeArray{<:Any, -1}
end

@testset "type aliases" begin
@test FixedSizeArrayDefault <: FixedSizeArray
@test FixedSizeVectorDefault <: FixedSizeVector
Expand Down Expand Up @@ -135,7 +195,7 @@ end
@testset "default underlying storage type" begin
default = FixedSizeArrays.default_underlying_storage_type
@test default === (@isdefined(Memory) ? Memory : Vector)
return_type = FixedSizeVector{Int,default{Int}}
return_type = FixedSizeVector{Int,default{Int},Tuple{Int}}
@test return_type === FixedSizeVectorDefault{Int}
test_inferred(FixedSizeArray{Int}, return_type, (undef, 3))
test_inferred(FixedSizeArray{Int}, return_type, (undef, (3,)))
Expand Down Expand Up @@ -507,16 +567,12 @@ end
end
@testset "`Union{}`" begin
@test_throws Exception collect_as(Union{}, ())
@test_throws Exception collect_as(FixedSizeVector{Union{}, Union{}}, ())
@test_throws Exception collect_as(FixedSizeVector{<:Any, Union{}}, ())
end
for T ∈ (FSA{Int}, FSV{Int})
for iterator ∈ (Iterators.repeated(7), Iterators.cycle(7))
@test_throws ArgumentError collect_as(T, iterator)
end
end
@test_throws ArgumentError collect_as(FSA{Int, -1}, 7:8)
@test_throws TypeError collect_as(FSA{Int, 3.1}, 7:8)
for T ∈ (FSA{3}, FSV{3})
iterator = (7:8, (7, 8))
@test_throws TypeError collect_as(T, iterator)
Expand Down
Loading