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

Extend @SArray (nested cat, 1.7 syntax) #1009

Merged
merged 11 commits into from
Mar 25, 2022
Merged

Conversation

N5N3
Copy link
Contributor

@N5N3 N5N3 commented Mar 2, 2022

This PR tries to add support to nested cat, and 1.7 Multidimensional syntax.

julia> @SArray [1 2 ; 3 4 ;;; 5 6 ; 7 8]
2×2×2 SArray{Tuple{2, 2, 2}, Int64, 3, 8} with indices SOneTo(2)×SOneTo(2)×SOneTo(2):
[:, :, 1] =
 1  2
 3  4

[:, :, 2] =
 5  6
 7  8

julia> @SArray [[[1 ; 2] ;; [3 ; 4]] ;;; [[5 ; 6] ;; [7 ; 8]]]
2×2×2 SArray{Tuple{2, 2, 2}, Int64, 3, 8} with indices SOneTo(2)×SOneTo(2)×SOneTo(2):
[:, :, 1] =
 1  3
 2  4

[:, :, 2] =
 5  7
 6  8

Close #974.
Edit: commit histroy has been splitted for easier review.

@N5N3 N5N3 force-pushed the NestedCat branch 7 times, most recently from eb542c7 to 870bb14 Compare March 3, 2022 09:07
@N5N3
Copy link
Contributor Author

N5N3 commented Mar 3, 2022

Beside the macro extension, most of the convert ambiguities are resolved in this PR. Now with a fresh REPL

julia> using StaticArrays, Test

julia> detect_ambiguities(#=LinearAlgebra, =#StaticArrays)
5-element Vector{Tuple{Method, Method}}:
 ((Scalar)(a::AbstractArray) in StaticArrays at C:\Users\MYJ\.julia\dev\StaticArrays\src\Scalar.jl:10, (::Type{SA})(a::StaticArray) where SA<:StaticArray 
in StaticArrays at C:\Users\MYJ\.julia\dev\StaticArrays\src\convert.jl:16)
 (SHermitianCompact(a::StaticMatrix{N, N, T}) where {N, T} in StaticArrays at C:\Users\MYJ\.julia\dev\StaticArrays\src\SHermitianCompact.jl:82, (::Type{SSC})(a::SSC) where SSC<:SHermitianCompact in StaticArrays at C:\Users\MYJ\.julia\dev\StaticArrays\src\SHermitianCompact.jl:85)
 ((::Type{SSC})(a::AbstractVector) where SSC<:SHermitianCompact in StaticArrays at C:\Users\MYJ\.julia\dev\StaticArrays\src\SHermitianCompact.jl:87, (::Type{SA})(a::StaticArray) where SA<:StaticArray in StaticArrays at C:\Users\MYJ\.julia\dev\StaticArrays\src\convert.jl:16)
 (SHermitianCompact(a::StaticMatrix{N, N, T}) where {N, T} in StaticArrays at C:\Users\MYJ\.julia\dev\StaticArrays\src\SHermitianCompact.jl:82, (::Type{SSC})(a::SHermitianCompact) where SSC<:SHermitianCompact in StaticArrays at C:\Users\MYJ\.julia\dev\StaticArrays\src\SHermitianCompact.jl:84)
 ((Scalar)(a::AbstractArray{T, 0} where T) in StaticArrays at C:\Users\MYJ\.julia\dev\StaticArrays\src\Scalar.jl:11, (::Type{SA})(a::StaticArray) where SA<:StaticArray in StaticArrays at C:\Users\MYJ\.julia\dev\StaticArrays\src\convert.jl:16)

Scalar's ambiguilty could be resolved with

@inline Scalar(a::StaticArray) = ndims(a) === 0 ? Scalar{eltype(a)}((a[],)) : Scalar{typeof(a)}((a,))

But I'm not sure is this the expected behavior.

All test passed locally, so I guess this is ready for review.

Edit: seperated to #1016

@mateuszbaran
Copy link
Collaborator

Thank you, this is really great. This will take some time to review though.

@N5N3
Copy link
Contributor Author

N5N3 commented Mar 3, 2022

Error seems reproduable on 32bit system. I'll take a look.

@N5N3
Copy link
Contributor Author

N5N3 commented Mar 3, 2022

Well, win32 test passed locally with official 1.7.2 (twice). Not sure what happened here.

@mateuszbaran
Copy link
Collaborator

Well, win32 test passed locally with official 1.7.2 (twice). Not sure what happened here.

I think the Julia process is just running out of memory.

Use `cat_any` to "cat" all arguments. (No promotion)
And better performance (3x faster)

Code clean for macro.
Just check the output's shape:
1. Alow missing dimension (Vector isa `n*1` Matrix)
2. And addition size-1 dimension (`m*n*1` Array isa `m*n` Matrix)
Code reuse.
1. [1;2] isa Vector
2. [f(...) for ...] has no dim limit.
The constructor is missing for empty `SVector`.
Would be fixed in future PRs. Thus just mark it as broken.
@N5N3
Copy link
Contributor Author

N5N3 commented Mar 22, 2022

Commits for constructor clean has been removed to focus this PR on syntax extension. (SA[1;;1] support has been added.)

Copy link
Collaborator

@mateuszbaran mateuszbaran left a comment

Choose a reason for hiding this comment

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

Thanks for this work. I didn't check all details but the code looks good and I only have a few small comments.

src/SArray.jl Outdated Show resolved Hide resolved
src/SArray.jl Outdated Show resolved Hide resolved
src/SArray.jl Outdated Show resolved Hide resolved
src/SArray.jl Outdated Show resolved Hide resolved
src/SArray.jl Outdated Show resolved Hide resolved
src/SArray.jl Outdated Show resolved Hide resolved
test/MVector.jl Show resolved Hide resolved
1. Only test `@SArray [;;]` on master.
2. code clean.
3. support `@SArray fill(1)` `@SArray zeros()`

Co-Authored-By: Mateusz Baran <[email protected]>
@mateuszbaran
Copy link
Collaborator

Close #911.

Did you add some tests related to this issue? I can't find it.

@N5N3
Copy link
Contributor Author

N5N3 commented Mar 23, 2022

Sorry for the careless. As #1016 has been seperated out, #911 wont be fixed in this PR, as the root cause is a constructor missing:

julia> MMatrix{2,2}((1.0,1,1,1))
ERROR: ArgumentError: Size mismatch in Static Array parameters. Got size Tuple{2, 2}, dimension 2 and length 1.

@mateuszbaran
Copy link
Collaborator

This may not be the simplest case but it works for normal arrays but not here:

a = @SArray fill(1, 2,3,2,4,5)
b = @SArray fill(2, 1,1,2,4,5)
c = @SArray fill(3, 1,2,2,4,5)
d = @SArray fill(4, 1,1,1,4,5)
e = @SArray fill(5, 1,1,1,4,5)
f = @SArray fill(6, 1,1,1,4,5)
g = @SArray fill(7, 2,3,1,4,5)
h = @SArray fill(8, 3,3,3,1,2)
i = @SArray fill(9, 3,2,3,3,2)
j = @SArray fill(10, 3,1,3,3,2)

result = @SArray [a; b c ;;; d e f ; g ;;;;; h ;;;; i j]
julia> [a; b c ;;; d e f ; g ;;;;; h ;;;; i j]
3×3×3×4×7 Array{Int64, 5}:
 < content >

julia> @SArray [a; b c ;;; d e f ; g ;;;;; h ;;;; i j]
ERROR: LoadError: DimensionMismatch("mismatch in dimension 2 (expected 1 got 2)")
Stacktrace:
 [1] cat_mismatch(j::Int64, sz::Int64, nsz::Int64)
   @ StaticArrays ~/.julia/dev/StaticArrays/src/SArray.jl:159
 [2] check_cat_size(szs::Vector{Tuple{Int64, Int64}}, catdim::Int64)
   @ StaticArrays ~/.julia/dev/StaticArrays/src/SArray.jl:170
 [3] cat_any(#unused#::Val{2}, #unused#::Val{1}, args::Vector{Any})
   @ StaticArrays ~/.julia/dev/Static

@mateuszbaran
Copy link
Collaborator

Ah, right, it can't work without a lot of trouble -- it might be worth documenting somewhere that @SArray and similar assume scalar elements.

@mateuszbaran
Copy link
Collaborator

I've tested this and it works well, so I think this can be merged after updating documentation and bumping version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants