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

Add Support for BitVector and BitMatrix Indexing #708

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add Support for BitVector and BitMatrix Indexing #708

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ab44000
Add Support for `BitVector` and `BitMatrix` Indexing
RoyiAvital Oct 27, 2024
0d5b3aa
Add Support for `BitVector` and `BitMatrix` Indexing
RoyiAvital Oct 27, 2024
e84d666
Add Support for `BitVector` and `BitMatrix` Indexing
RoyiAvital Oct 27, 2024
3ade074
Update IndexAtom.jl
odow Oct 27, 2024
52fc1f9
Update test_atoms.jl
odow Oct 27, 2024
3af9f28
Update test_atoms.jl
odow Oct 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/atoms/IndexAtom.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,14 @@ function Base.getindex(x::AbstractExpr, I::AbstractVector{Bool})
return [xi for (xi, ii) in zip(x, I) if ii]
end

function Base.getindex(x::AbstractExpr, I::BitMatrix)
return [xi for (xi, ii) in zip(x, I) if ii]
end

function Base.getindex(x::AbstractExpr, I::BitVector)
return [xi for (xi, ii) in zip(x, I) if ii]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need some boundschecking on these, since we should get an error if I is too large or has the wrong shape, but zip won't do that

Copy link
Author

Choose a reason for hiding this comment

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

I copied the code from the Boolean matrix / vector case.
Fixing it here won't change there.

So maybe get this in and add an issue for bound checking for the Boolean / Bit cases later?

Copy link
Collaborator

@ericphanson ericphanson Oct 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

ah ok. Maybe we can just change the signature of those to

Base.getindex(x::AbstractExpr, I::Union{<:AbstractVector{Bool}, <:BitVector})

? Instead of adding two new methods. Then we would only need to add the checking in one place later.

Copy link
Author

Choose a reason for hiding this comment

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

I'm OK with anything :-).

Copy link
Author

Choose a reason for hiding this comment

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

@ericphanson , Done.

Copy link
Author

Choose a reason for hiding this comment

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

By the way, didn't contribute anything real here. All credit is you and @odow which I basically copied his text.
So the credit should be yours.

end

# All rows and columns
Base.getindex(x::AbstractExpr, ::Colon, ::Colon) = x

Expand Down
9 changes: 9 additions & 0 deletions test/test_atoms.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -520,6 +520,15 @@ function test_IndexAtom()
y = [true, false, true]
x = Variable(3)
@test string(x[y]) == string([x[1], x[3]])
target = """
variables: x1, x2, x3
minobjective: [1.0 * x1, 1.0 * x3]
"""
_test_atom(target) do context
x = Variable(3)
y = BitVector([true, false, true])
return x[y]
end
return
end

Expand Down