Define nextind/prevind for ChainedVectorIndex #75
Conversation
The `BitVector(::AbstractVector{Bool})` method uses the `Base.nextind`
method to increment the inds from `eachindex` that I didn't know existed.
I'm not exactly sure why they use that instead of just iterating `eachindex`
but it turns out we can define `nextind` to be pretty efficient for
`ChainedVectorIndex` since we basically already have that logic in
`eachindex(::ChainedVector)`.
Fixes the issue reported in #74.
Codecov ReportBase: 95.42% // Head: 95.38% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #75 +/- ##
==========================================
- Coverage 95.42% 95.38% -0.05%
==========================================
Files 4 4
Lines 897 996 +99
==========================================
+ Hits 856 950 +94
- Misses 41 46 +5
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| chunk_i = 1 | ||
| i += 1 | ||
| end | ||
| return ChainedVectorIndex(chunkidx, chunk, chunk_i, i) |
There was a problem hiding this comment.
Two questions:
- first is something we discussed some time ago, but can you please confirm that chunks cannot be empty?
- the function returns the same value when last element of the array is visited - is this intended? Other definitions behave as follows:
julia> nextind([1, 2], 0)
1
julia> nextind([1, 2], 1)
2
julia> nextind([1, 2], 2)
3
julia> nextind([1, 2], 3)
4
julia> nextind([1,2,3], CartesianIndex(0))
CartesianIndex(1,)
julia> nextind([1,2,3], CartesianIndex(1))
CartesianIndex(2,)
julia> nextind([1,2,3], CartesianIndex(2))
CartesianIndex(3,)
julia> nextind([1,2,3], CartesianIndex(3))
CartesianIndex(4,)
There was a problem hiding this comment.
- Correct. We have a
cleanup!function that is called before operations where we require all non-empty chunks (and it is called byeachindex) - I assumed the intended behavior was that you eventually get an
indwhich will throw aBoundsError, and so it doesn't matter if you keep incrementing the ind after that since you'll always get the sameBoundsErrorbehavior.
There was a problem hiding this comment.
Ad 2.
Yes, but your condition is:
if chunk_i < length(chunk)
...
elseif chunkidx < length(A.arrays)
...
end
so if chunk_i == length(chunk) && chunkidx == length(A.arrays) (so essentially it is a last element of an array) you do not move it to BoundsError, but leave it as is. This was my issue
There was a problem hiding this comment.
Ah shoot; good catch. I'll come up with a fix.
| @@ -212,11 +213,15 @@ Base.hash(x::ChainedVectorIndex, h::UInt) = hash(x.i, h) | |||
|
|
|||
| @inline Base.getindex(x::ChainedVectorIndex) = @inbounds x.array[x.array_i] | |||
|
|
|||
| @inline function Base.getindex(A::ChainedVector, x::ChainedVectorIndex) | |||
| Base.checkbounds(::Type{Bool}, A::ChainedVector, ind::ChainedVectorIndex) = 1 <= ind.array_i <= length(ind.array) | |||
There was a problem hiding this comment.
checkbounds probably should also check i?
There was a problem hiding this comment.
ah - maybe it is ignored everywhere.
There was a problem hiding this comment.
we don't rely on i at all except for "display"-like purposes.
| @@ -196,6 +196,7 @@ end | |||
|
|
|||
| # custom index type used in eachindex | |||
| struct ChainedVectorIndex{A} <: Integer | |||
| arrays_i::Int | |||
There was a problem hiding this comment.
do we check somewhere that A type uses 1-based indexing?
| chunk_i = 1 | ||
| i += 1 | ||
| else | ||
| chunk_i += 1 # make sure this goes out of bounds |
There was a problem hiding this comment.
increment of i is missing. Maybe it should just be moved out of the if part and just i = x.i + 1 be written. Same for prevind.
The
BitVector(::AbstractVector{Bool})method uses theBase.nextindmethod to increment the inds fromeachindexthat I didn't know existed. I'm not exactly sure why they use that instead of just iteratingeachindexbut it turns out we can definenextindto be pretty efficient forChainedVectorIndexsince we basically already have that logic ineachindex(::ChainedVector).Fixes the issue reported in #74.