Skip to content

Introduce StaticArrayLike #565

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

Jump to bottom
Merged
merged 2 commits into from
Dec 13, 2018
Merged

Introduce StaticArrayLike #565

merged 2 commits into from
Dec 13, 2018

Conversation

tkoolen
Copy link
Contributor

@tkoolen tkoolen commented Dec 10, 2018

Now that we have wrapper types in Base that can be used in conjunction
with StaticArrays, we should discern between actual StaticArrays and
non-StaticArray subtypes for which we still know a static Size. To
this effect, this PR adds the type aliases StaticallySizedMatrix,
StaticallySizedVecOrMat, and StaticallySizedArray, and uses them to
widen various type signatures.

Fixes #561.

@tkoolen
Introduce StaticallySizedArray
c8e2fa7 
Now that we have wrapper types in Base that can be used in conjunction
with StaticArrays, we should discern between actual `StaticArray`s and
non-`StaticArray` subtypes for which we still know a static `Size`. To
this effect, this commit adds the type aliases `StaticallySizedMatrix`,
`StaticallySizedVecOrMat`, and `StaticallySizedArray`, and uses them to
widen various type signatures.

Fixes #561.
@coveralls
Copy link

coveralls commented Dec 11, 2018 •
edited
Loading

Coverage Status

Coverage increased (+0.07%) to 66.3% when pulling 7a4bc2e on tk/statically-sized-array into 3ccbd41 on master.

c42f
c42f approved these changes Dec 11, 2018
View reviewed changes
Copy link
Member

@c42f c42f 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.

Obviously not a general fix as people can define their own wrappers... but short of a statically sized trait system in Base to support us, there's probably not much we can do about that (?)

src/StaticArrays.jl Outdated
@@ -78,6 +78,19 @@ const StaticVector{N, T} = StaticArray{Tuple{N}, T, 1}
const StaticMatrix{N, M, T} = StaticArray{Tuple{N, M}, T, 2}
const StaticVecOrMat{T} = Union{StaticVector{<:Any, T}, StaticMatrix{<:Any, <:Any, T}}

# Being a member of StaticallySizedMatrix, StaticallySizedVecOrMat, or StaticallySizedArray implies that Size(A)
# returns a static Size instance. The converse may not be true.
const StaticallySizedMatrix{T} = Union{
Copy link
Member

Choose a reason for hiding this comment

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

There's an interesting (possibly less confusing) alternative name StaticArrayLike (/ StaticMatrixLike) at #537 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fine by me. Anybody else have opinions on the naming?

@tkoolen
Copy link
Contributor Author

tkoolen commented Dec 11, 2018 •
edited
Loading

short of a statically sized trait system in Base to support us, there's probably not much we can do about that?

Yeah. Another case that's not handled is nested wrappers from LinearAlgebra, like Symmetric{Diagonal{X}}. This PR at least makes it so that the Size for such types is no longer dynamic, but method dispatch for such types is just not possible without a hook in Base, I don't think.

@andyferris
Copy link
Member

Well, I guess this may be an improvement on the status quo but I have to admit that I do find it a bit unsatisfying - I find things like StridedArray to be a bit of an anti-pattern.

like Symmetric{Diagonal{X}}

Does this ever get constructed? It seems a little unnecessary.

@tkoolen
Copy link
Contributor Author

tkoolen commented Dec 11, 2018

I find things like StridedArray to be a bit of an anti-pattern.

Fully agree. But I think it's just necessary given the current approach in Base.

Does this ever get constructed? It seems a little unnecessary.

No, no. Just a made-up example.

andyferris
andyferris approved these changes Dec 11, 2018
View reviewed changes
Copy link
Member

@andyferris andyferris left a comment

Choose a reason for hiding this comment

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

Well, if it is helpful, I guess we should do it. :)

@c42f
Copy link
Member

c42f commented Dec 11, 2018

Well, if it is helpful, I guess we should do it

My thoughts exactly.

@tkoolen tkoolen changed the title Introduce StaticallySizedArray Introduce StaticArrayLike Dec 11, 2018
@tkoolen
Copy link
Contributor Author

tkoolen commented Dec 11, 2018

Did the rename and added a comment referring to StridedArray.

@timholy
Copy link
Member

timholy commented Dec 11, 2018

Dang this is just crying out for a new version of Traitor.jl. Maybe a Christmas-break project...

@tkoolen tkoolen merged commit ae14694 into master Dec 13, 2018
@tkoolen tkoolen deleted the tk/statically-sized-array branch December 13, 2018 00:12
@c42f c42f mentioned this pull request Dec 21, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@c42f c42f c42f approved these changes

@andyferris andyferris andyferris approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tkoolen @coveralls @andyferris @c42f @timholy