Skip to content

fix: fix n-dimensional view into n-dimensional SArray with Any eltype #1310

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

Merged

Conversation

AayushSabharwal
Copy link
Contributor

As of the latest release, trying to view into an SArray with an eltype of Any such that the view also returns an SArray would wrap the returned array in a scalar. _maybewrapscalar assumes that if the eltype of the array matches the type of the view, it must be a scalar view. This assumption breaks down when the eltype is Any. MWE:

julia> A = SVector{4, Any}(1,2,3,4)

julia> I = SVector{2}(3, 1)

julia> view(A, I)
Scalar{Any}((Any[3, 1],))

julia> view(A, [3, 1])
2-element view(::SVector{4, Any}, [3, 1]) with eltype Any:
 3
 1

I've verified that all the added tests fail on the latest release and pass with this PR.

@AayushSabharwal
Copy link
Contributor Author

I'm not sure what to make of the CI failure or how it relates to the changes in this PR.

@mateuszbaran
Copy link
Collaborator

This looks reasonable.

Regarding the failure, IIRC Julia 1.6 doesn't support the new [1;;;] syntax but you can use something like @SArray fill(1, 1, 1) instead in the test.

@AayushSabharwal AayushSabharwal force-pushed the as/any-eltype-sarray-view branch from 45b0016 to 5ec5a81 Compare July 15, 2025 18:51
@AayushSabharwal
Copy link
Contributor Author

Makes sense, thanks. Pushed the fix.

test/indexing.jl Outdated
v = @inferred view(A, SA[3, 1])
@test v == SVector{2, Any}(3, 1)
A = SMatrix{2, 2, Any}(1, 2, 3, 4)
v = @inferred view(A, @SArray(fill(1, 1)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
v = @inferred view(A, @SArray(fill(1, 1)))
v = @inferred view(A, @SArray(fill(1, 1, 1)))

I think this needs one more 1: two for dimensions and one for for actual element? (similarly in the next test)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like I confused myself - thanks for catching that.

@AayushSabharwal AayushSabharwal force-pushed the as/any-eltype-sarray-view branch from 5ec5a81 to 8a35ec2 Compare July 16, 2025 05:40
@mateuszbaran mateuszbaran merged commit 41353d2 into JuliaArrays:master Jul 16, 2025
17 of 24 checks passed
@AayushSabharwal AayushSabharwal deleted the as/any-eltype-sarray-view branch July 16, 2025 11:02
@AayushSabharwal
Copy link
Contributor Author

Thanks for merging! Would it be possible to cut a release soon 😅 this is blocking some work in ModelingToolkit.jl.

@mateuszbaran
Copy link
Collaborator

Sure, I will make a release soon.

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.

2 participants