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.

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

Bug in ChainedVector indexing API #74

Closed
bkamins opened this issue Sep 16, 2022 · 1 comment
Closed

Bug in ChainedVector indexing API #74

bkamins opened this issue Sep 16, 2022 · 1 comment
Labels
bug Something isn't working

Comments

@bkamins
Copy link
Member

bkamins commented Sep 16, 2022

An example showing the problem:

julia> x = ChainedVector([[true], [false], [true]])
3-element SentinelArrays.ChainedVector{Bool, Vector{Bool}}:
 1
 0
 1

julia> BitVector(x)
ERROR: MethodError: no method matching Int64(::SentinelArrays.ChainedVectorIndex{Vector{Bool}})

original problem reported by the user: JuliaData/DataFrames.jl#3154
@bkamins bkamins added the bug Something isn't working label Sep 16, 2022
quinnj added a commit that referenced this issue Sep 18, 2022
@quinnj
Define nextind/prevind for ChainedVectorIndex
2ae8830 
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.
@quinnj quinnj mentioned this issue Sep 18, 2022
Merged
@quinnj
Copy link
Member

quinnj commented Sep 18, 2022

Thanks for the report! Fix is up: #75

quinnj added a commit that referenced this issue Sep 20, 2022
@quinnj
Define nextind/prevind for ChainedVectorIndex (#75)
d3bd9d3 
* Define nextind/prevind for ChainedVectorIndex

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.

* fix nightly test

* fix out of bounds cases
@quinnj quinnj closed this as completed Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@quinnj @bkamins