Fix type instability for vec[1, end] indexing#526
Conversation
|
@JoshuaLampert can you take a look? |
|
I will try to take a look tomorrow. Today is busy for me. |
|
This does fix the case I ran into: vecvec = VectorOfArray([fill(2.0, 2) for i in 1:10])
access_end(vec) = vec[1, end]
@inferred access_end(vecvec) # Returns Float64 as expectedThere is still another type instability if you switch the indices, though: vecvec = VectorOfArray([fill(2.0, 2) for i in 1:10])
access_end(vec) = vec[end, 1]
@inferred access_end(vecvec) # Errors because inference fails
@inferred getindex(vecvec, lastindex(vecvec), 1) # Same error |
test/interface_tests.jl
Outdated
| last_row = lastindex(testva_end) | ||
| @inferred testva_end[last_row, 1] | ||
| @test testva_end[last_row, 1 == 2.0] |
There was a problem hiding this comment.
| last_row = lastindex(testva_end) | |
| @inferred testva_end[last_row, 1] | |
| @test testva_end[last_row, 1 == 2.0] | |
| last_row = lastindex(testva_end, 1) | |
| @inferred testva_end[last_row, 1] | |
| @test testva_end[last_row, 1] == 2.0 |
I guess, this is what you wanted to test @Ickaser? lastindex(testva_end) is 10 here, but we want 2, which is what we get by lastindex(testva_end, 1). The last line is a typo I guess.
There was a problem hiding this comment.
The last line is a typo which should be fixed, but the lastindex(testva_end) was on purpose; I was trying to make an inference test for testva_end[end, 1], which currently isn't inferred. If there is a better way to test that, feel free to replace.
There was a problem hiding this comment.
Yes, but lastindex(testva_end) returns the lastindex of the underlying vector testva_end.u, which is the dimension you put last when indexing (here 10). When you call testva_end[end, 1] you want the end to resolve to lastindex of the inner dimension, which would be lastindex(testva_end, 1) (here 2). Otherwise there is a BoundsError as you can see here. The inference test for
last_row = lastindex(testva_end, 1)
@inferred testva_end[last_row, 1]currently fails (but @test testva_end[last_row, 1] == 2.0 passes in contrast to using last_row = lastindex(testva_end)).
There was a problem hiding this comment.
@ChrisRackauckas, could you let Claude work on fixing inference also for
last_row = lastindex(testva_end, 1)
@inferred testva_end[last_row, 1]Otherwise, this looks good to me.
|
Added type-stable Changes:
Test verification: testva_end = VectorOfArray([fill(2.0, 2) for i in 1:10])
last_row = lastindex(testva_end, 1)
@inferred testva_end[last_row, 1] # Now returns Float64, was Any beforeAll tests pass locally. |
Add specialized getindex methods for RaggedEnd to ensure type-stable access when using `end` indexing: - `getindex(A, i::Int, re::RaggedEnd)` for `vec[i, end]` pattern - `getindex(A, re::RaggedEnd, col::Int)` for `vec[end, col]` pattern Both methods handle the RaggedEnd sentinel case (dim=0) for resolved column indices and the non-sentinel case for ragged inner dimensions. Added tests verifying type inference for: - `vec[1, lastindex(vec, 2)]` - RaggedEnd in column position - `vec[1, lastindex(vec)]` - Int in column position - `vec[lastindex(vec, 1), 1]` - RaggedEnd in row position Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com> Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1167924 to
810b624
Compare
|
Rebased onto latest master ( The PR now includes both type-stable
All tests pass. |
Summary
vec[1, end]#525getindex(A, i::Int, re::RaggedEnd)method for type-stable access when usingendindexing_ragged_getindex_int_colas a function barrier for the Int column case_ragged_getindexfor theRaggedEndsentinel caseProblem
When accessing a
VectorOfArraywith[1, end], the result was type-unstable (returningAny):The issue was that
lastindex(VA, d)returnsRaggedEnd(0, offset)as a sentinel for an already-resolved index, but thegetindexdispatch couldn't infer the return type becausecols.dim == 0is a runtime check.Solution
Add a specialized method for the common
vec[i, end]pattern that handles theRaggedEndtype directly:After this fix:
Test plan
vec[1, end]pattern🤖 Generated with Claude Code