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.

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

Add Support for BitVector and BitMatrix Indexing #708

Merged
merged 6 commits into from
Nov 13, 2024

Conversation

RoyiAvital
Copy link
Contributor

Fixes #707 .

src/atoms/IndexAtom.jl Outdated Show resolved Hide resolved
function Base.getindex(
x::AbstractExpr,
I::Union{AbstractMatrix{Bool},<:BitMatrix},
)
return [xi for (xi, ii) in zip(x, I) if ii]
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. So these methods have been there forever:
92c93fd
but they don't really with the vibe of Convex because they return vector/matrices instead of a new atom.

Copy link

codecov bot commented Oct 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.24%. Comparing base (cc71b51) to head (3af9f28).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #708      +/-   ##
==========================================
+ Coverage   98.21%   98.24%   +0.02%     
==========================================
  Files          87       87              
  Lines        5214     5186      -28     
==========================================
- Hits         5121     5095      -26     
+ Misses         93       91       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@odow
Copy link
Member

odow commented Oct 27, 2024

So @RoyiAvital's example now works (with a small tweak).

It's a bit annoying that BitVector requires different syntax to Vector{Int}:

julia> using Convex

julia> using ECOS

julia> N = 100
100

julia> x = Variable(N)
Variable
size: (100, 1)
sign: real
vexity: affine
id: 178163

julia> y = rand(N);

julia> threshold = sort(y; rev = true)[5]
       # BitVector
0.9244665817970124

julia> p = y .>= threshold;

julia> constraints = x[p] .== y[p]
5-element Vector{Constraint{MathOptInterface.Zeros}}:
 == constraint (affine)
└─ + (affine; real)
   ├─ index (affine; real)
   │  └─ 100-element real variable (id: 178163)
   └─ [-0.951834;;]
 == constraint (affine)
└─ + (affine; real)
   ├─ index (affine; real)
   │  └─ 100-element real variable (id: 178163)
   └─ [-0.993115;;]
 == constraint (affine)
└─ + (affine; real)
   ├─ index (affine; real)
   │  └─ 100-element real variable (id: 178163)
   └─ [-0.977036;;]
 == constraint (affine)
└─ + (affine; real)
   ├─ index (affine; real)
   │  └─ 100-element real variable (id: 178163)
   └─ [-0.983307;;]
 == constraint (affine)
└─ + (affine; real)
   ├─ index (affine; real)
   │  └─ 100-element real variable (id: 178163)
   └─ [-0.924467;;]

julia> model = minimize(sumsquares(x - vY), constraints)
Problem statistics
  problem is DCP         : true
  number of variables    : 1 (100 scalar elements)
  number of constraints  : 5 (5 scalar elements)
  number of coefficients : 106
  number of atoms        : 12

Solution summary
  termination status : OPTIMIZE_NOT_CALLED
  primal status      : NO_SOLUTION
  dual status        : NO_SOLUTION

Expression graph
  minimize
   └─ qol (convex; positive)
      ├─ + (affine; real)
      │  ├─ 100-element real variable (id: 178163)
      │  └─ 100×1 Matrix{Float64}
      └─ [1;;]
  subject to
   ├─ == constraint (affine)
   │  └─ + (affine; real)
   │     ├─ index (affine; real)
   │     │  └─ 
   │     └─ [-0.951834;;]
   ├─ == constraint (affine)
   │  └─ + (affine; real)
   │     ├─ index (affine; real)
   │     │  └─ 
   │     └─ [-0.993115;;]
   ├─ == constraint (affine)
   │  └─ + (affine; real)
   │     ├─ index (affine; real)
   │     │  └─ 
   │     └─ [-0.977036;;]
   


julia> solve!(model, ECOS.Optimizer)
       # findall
[ Info: [Convex.jl] Compilation finished: 0.0 seconds, 265.953 KiB of memory allocated

ECOS 2.0.8 - (C) embotech GmbH, Zurich Switzerland, 2012-15. Web: www.embotech.com/ECOS

It     pcost       dcost      gap   pres   dres    k/t    mu     step   sigma     IR    |   BT
 0  +0.000e+00  -3.537e-03  +7e+01  2e-01  8e-04  1e+00  4e+01    ---    ---    1  1  - |  -  - 
 1  -3.848e-03  -4.005e-04  +1e+00  6e-03  2e-05  2e-02  7e-01  0.9816  1e-04   1  1  1 |  0  0
 2  +7.394e-02  +1.124e-01  +5e-01  3e-03  7e-06  5e-02  3e-01  0.6382  8e-02   1  2  2 |  0  0
 3  -5.985e-01  +4.737e-01  +5e-01  5e-02  5e-05  1e+00  3e-01  0.2932  8e-01   2  2  2 |  0  0
 4  +7.193e-01  +7.258e-01  +4e-02  3e-03  3e-06  1e-02  2e-02  0.9329  3e-04   2  2  2 |  0  0
 5  +1.275e+00  +1.315e+00  +1e-02  3e-04  5e-07  4e-02  7e-03  0.7002  1e-01   2  2  2 |  0  0
 6  +1.318e+00  +1.323e+00  +9e-04  3e-05  5e-08  5e-03  7e-04  0.9101  9e-04   1  1  1 |  0  0
 7  +1.329e+00  +1.331e+00  +3e-04  1e-05  2e-08  3e-03  2e-04  0.8508  2e-01   1  1  1 |  0  0
 8  +1.332e+00  +1.332e+00  +2e-05  1e-06  2e-09  2e-04  2e-05  0.9118  1e-03   1  1  1 |  0  0
 9  +1.332e+00  +1.332e+00  +5e-06  3e-07  4e-10  5e-05  5e-06  0.8998  1e-01   1  1  1 |  0  0
10  +1.332e+00  +1.332e+00  +6e-07  3e-08  5e-11  7e-06  6e-07  0.8800  3e-03   1  1  1 |  0  0
11  +1.332e+00  +1.332e+00  +2e-07  3e-08  1e-11  2e-06  1e-07  0.8801  1e-01   1  1  1 |  0  0
12  +1.332e+00  +1.332e+00  +2e-08  2e-09  2e-12  3e-07  2e-08  0.8488  8e-03   1  1  1 |  0  0
13  +1.332e+00  +1.332e+00  +6e-09  5e-09  1e-12  8e-08  6e-09  0.8578  1e-01   1  1  1 |  0  0

OPTIMAL (within feastol=4.9e-09, reltol=4.8e-09, abstol=6.4e-09).
Runtime: 0.000621 seconds.

Problem statistics
  problem is DCP         : true
  number of variables    : 1 (100 scalar elements)
  number of constraints  : 5 (5 scalar elements)
  number of coefficients : 106
  number of atoms        : 12

Solution summary
  termination status : OPTIMAL
  primal status      : FEASIBLE_POINT
  dual status        : FEASIBLE_POINT
  objective value    : 1.3321

Expression graph
  minimize
   └─ qol (convex; positive)
      ├─ + (affine; real)
      │  ├─ 100-element real variable (id: 178163)
      │  └─ 100×1 Matrix{Float64}
      └─ [1;;]
  subject to
   ├─ == constraint (affine)
   │  └─ + (affine; real)
   │     ├─ index (affine; real)
   │     │  └─ 
   │     └─ [-0.951834;;]
   ├─ == constraint (affine)
   │  └─ + (affine; real)
   │     ├─ index (affine; real)
   │     │  └─ 
   │     └─ [-0.993115;;]
   ├─ == constraint (affine)
   │  └─ + (affine; real)
   │     ├─ index (affine; real)
   │     │  └─ 
   │     └─ [-0.977036;;]
   


julia> p = findall(y .>= threshold);

julia> constraints = x[p] == y[p]
== constraint (affine)
└─ + (affine; real)
   ├─ index (affine; real)
   │  └─ 100-element real variable (id: 178163)
   └─ 5×1 Matrix{Float64}

julia> model = minimize(sumsquares(x - vY), [constraints])
Problem statistics
  problem is DCP         : true
  number of variables    : 1 (100 scalar elements)
  number of constraints  : 1 (5 scalar elements)
  number of coefficients : 106
  number of atoms        : 4

Solution summary
  termination status : OPTIMIZE_NOT_CALLED
  primal status      : NO_SOLUTION
  dual status        : NO_SOLUTION

Expression graph
  minimize
   └─ qol (convex; positive)
      ├─ + (affine; real)
      │  ├─ 100-element real variable (id: 178163)
      │  └─ 100×1 Matrix{Float64}
      └─ [1;;]
  subject to
   └─ == constraint (affine)
      └─ + (affine; real)
         ├─ index (affine; real)
         │  └─ 
         └─ 5×1 Matrix{Float64}


julia> solve!(model, ECOS.Optimizer)
[ Info: [Convex.jl] Compilation finished: 0.0 seconds, 252.016 KiB of memory allocated

ECOS 2.0.8 - (C) embotech GmbH, Zurich Switzerland, 2012-15. Web: www.embotech.com/ECOS

It     pcost       dcost      gap   pres   dres    k/t    mu     step   sigma     IR    |   BT
 0  +0.000e+00  -3.537e-03  +7e+01  2e-01  8e-04  1e+00  4e+01    ---    ---    1  1  - |  -  - 
 1  -3.848e-03  -4.005e-04  +1e+00  6e-03  2e-05  2e-02  7e-01  0.9816  1e-04   1  1  1 |  0  0
 2  +7.394e-02  +1.124e-01  +5e-01  3e-03  7e-06  5e-02  3e-01  0.6382  8e-02   1  2  2 |  0  0
 3  -5.985e-01  +4.737e-01  +5e-01  5e-02  5e-05  1e+00  3e-01  0.2932  8e-01   2  2  2 |  0  0
 4  +7.193e-01  +7.258e-01  +4e-02  3e-03  3e-06  1e-02  2e-02  0.9329  3e-04   2  2  2 |  0  0
 5  +1.275e+00  +1.315e+00  +1e-02  3e-04  5e-07  4e-02  7e-03  0.7002  1e-01   2  2  2 |  0  0
 6  +1.318e+00  +1.323e+00  +9e-04  3e-05  5e-08  5e-03  7e-04  0.9101  9e-04   1  1  1 |  0  0
 7  +1.329e+00  +1.331e+00  +3e-04  1e-05  2e-08  3e-03  2e-04  0.8508  2e-01   1  1  1 |  0  0
 8  +1.332e+00  +1.332e+00  +2e-05  1e-06  2e-09  2e-04  2e-05  0.9118  1e-03   1  1  1 |  0  0
 9  +1.332e+00  +1.332e+00  +5e-06  3e-07  4e-10  5e-05  5e-06  0.8998  1e-01   1  1  1 |  0  0
10  +1.332e+00  +1.332e+00  +6e-07  3e-08  5e-11  7e-06  6e-07  0.8800  3e-03   1  1  1 |  0  0
11  +1.332e+00  +1.332e+00  +2e-07  3e-08  1e-11  2e-06  1e-07  0.8801  1e-01   1  1  1 |  0  0
12  +1.332e+00  +1.332e+00  +2e-08  2e-09  2e-12  3e-07  2e-08  0.8488  8e-03   1  1  1 |  0  0
13  +1.332e+00  +1.332e+00  +6e-09  5e-09  1e-12  8e-08  6e-09  0.8578  1e-01   1  1  1 |  0  0

OPTIMAL (within feastol=4.9e-09, reltol=4.8e-09, abstol=6.4e-09).
Runtime: 0.000871 seconds.

Problem statistics
  problem is DCP         : true
  number of variables    : 1 (100 scalar elements)
  number of constraints  : 1 (5 scalar elements)
  number of coefficients : 106
  number of atoms        : 4

Solution summary
  termination status : OPTIMAL
  primal status      : FEASIBLE_POINT
  dual status        : FEASIBLE_POINT
  objective value    : 1.3321

Expression graph
  minimize
   └─ qol (convex; positive)
      ├─ + (affine; real)
      │  ├─ 100-element real variable (id: 178163)
      │  └─ 100×1 Matrix{Float64}
      └─ [1;;]
  subject to
   └─ == constraint (affine)
      └─ + (affine; real)
         ├─ index (affine; real)
         │  └─ 
         └─ 5×1 Matrix{Float64}

@odow odow requested a review from ericphanson October 27, 2024 22:23
@RoyiAvital
Copy link
Contributor Author

@odow , I thought @ericphanson has already reviewed it.
Or are you after a review of your changes?

@odow
Copy link
Member

odow commented Oct 29, 2024

I'm after a new review because of the different syntax required for indexing with BitVector and Vector{Int}.

@odow
Copy link
Member

odow commented Nov 4, 2024

Thoughts @ericphanson?

@RoyiAvital
Copy link
Contributor Author

Any chance to move forward with this?

@odow
Copy link
Member

odow commented Nov 12, 2024

@RoyiAvital are you okay with the current behaviour?

@RoyiAvital
Copy link
Contributor Author

RoyiAvital commented Nov 12, 2024

@odow , When you say current, you mean without supporting BitVector / BitMatrix?
Or do you mean the fact that it requires .==?

@odow
Copy link
Member

odow commented Nov 12, 2024

Or do you mean the fact that it requires .==?

This. It currently returns a vector of expressions, instead of an expression that is a vector.

@odow
Copy link
Member

odow commented Nov 12, 2024

Its matches the existing indexing behaviour of Vector{Bool}, but not the indexing behaviour of Vector{Int}.

@RoyiAvital
Copy link
Contributor Author

I think as long as it is documented it is OK.
So maybe just add a comment in https://jump.dev/Convex.jl/stable/examples/supplemental_material/Convex.jl_intro_ISMP2015/#Constraints?

@odow
Copy link
Member

odow commented Nov 13, 2024

Merging because, while I don't particularly like this, there is precedent with the existing method.

We could always revisit it as part of a breaking [email protected] release.

@odow odow merged commit b5923f9 into jump-dev:master Nov 13, 2024
9 checks passed
@odow
Copy link
Member

odow commented Nov 13, 2024

I don't know that a tutorial from 2015 in the supplemental material section is the right place to document. But there isn't really a better place. This probably works as expected for people who try to use it, which is about all that can be said for a lot of Julia.

@RoyiAvital
Copy link
Contributor Author

I agree with your pragmatic approach.

Is there an easy way to create the proper atom out of it?
Maybe something I can try?

@odow
Copy link
Member

odow commented Nov 14, 2024

Yeah maybe it was a mistake copying the Bool methods.

It might actually be sufficient to do

getindex(x::AbstractExpr, y::BitVector) = getindex(x, findall(y))

and something similar for the BitMatrix case. Maybe

getindex(x::AbstractExpr, y::BitMatrix) = getindex(x, LinearIndices(y)[y])

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Support Boolean Indexing for Constraints
3 participants