Skip to content

Fix complexity of getindex(::ScalarFunctionIterator, i)#1257

Merged
odow merged 7 commits intomasterfrom
od/vaf_iterator
Mar 4, 2021
Merged

Fix complexity of getindex(::ScalarFunctionIterator, i)#1257
odow merged 7 commits intomasterfrom
od/vaf_iterator

Conversation

@odow
Copy link
Copy Markdown
Member

@odow odow commented Mar 2, 2021

Updated times for most recent commit
Before

julia> @time main(300)
 10.678643 seconds (632.16 k allocations: 132.960 MiB, 1.65% gc time)

After

julia> @time main(300);
  0.167274 seconds (182.72 k allocations: 137.331 MiB, 17.05% gc time)

Code

function main(n)
    model = MOIU.Model{Float64}()
    x = MOI.add_variables(model, n^2)
    MOI.set(model, MOI.ObjectiveSense(), MOI.MIN_SENSE)
    f = MOI.ScalarQuadraticFunction{Float64}(
        MOI.ScalarAffineTerm{Float64}.(rand(n^2), x),
        MOI.ScalarQuadraticTerm{Float64}.(1.0, x, x),
        0.0,
    )
    MOI.set(
        model,
        MOI.ObjectiveFunction{MOI.ScalarQuadraticFunction{Float64}}(),
        f,
    )
    MOI.add_constraint(model, MOI.VectorOfVariables(x), MOI.Nonnegatives(n))
    bridged = MOI.Bridges.full_bridge_optimizer(
        MOI.Utilities.CachingOptimizer(
            MOI.Utilities.Model{Float64}(),
            SCS.Optimizer(),
        ),
        Float64,
    )
    MOI.copy_to(bridged, model)
    return bridged
end

Closes #1137
Closes #418

@blegat
Copy link
Copy Markdown
Member

blegat commented Mar 2, 2021

This does not fix #418 completely.
If you use getindex several time which is what happens if you do collect the it's still slow.
We should precompute an array next::Vector{Int} that has the same size than the number of terms and next[i] is the index of the smallest j > i such that terms[i].output_index == terms[i].output_index.
This can be computed by going through the vector of terms just once.
Then every getindex is efficient

@odow
Copy link
Copy Markdown
Member Author

odow commented Mar 2, 2021

Okay. I've rewritten all the getindex functions to use a cache of the output index to the index of the term in the array. This should be fast now, at the expense of some more memory.

@odow odow changed the title Fix complexity of getindex(::ScalarFunctionIterator{VAF} Fix complexity of getindex(::ScalarFunctionIterator, i) Mar 2, 2021
@blegat
Copy link
Copy Markdown
Member

blegat commented Mar 3, 2021

Creating a vector of vector will be much slower than a single vector as suggested in #1257 (comment).
You can do it with a backward lookup

next = Vector{Int}(undef, length(terms))
last = zeros(Int, output_dimension)
for i in eachindex(terms)
    j = term[i].output_index
    if !iszero(last[j])
        next[last[j]] = i
    end
    last[j] = i
end
for j in eachindex(last)
    if !iszero(last[j])
        next[last[j]] = 0
    end
end

then you store next in the struct and last, you can just drop it.

@odow
Copy link
Copy Markdown
Member Author

odow commented Mar 3, 2021

This assumes that the function is canonicalized?

This fixes the existing performance problem. I don't know if it's worth checking for canonical and adding both look-up ways. It gets rid of the current bottleneck, we can revisit if it becomes a problem in future.

@odow odow requested a review from blegat March 4, 2021 03:09
@blegat
Copy link
Copy Markdown
Member

blegat commented Mar 4, 2021

This assumes that the function is canonicalized?

No

@odow
Copy link
Copy Markdown
Member Author

odow commented Mar 4, 2021

@blegat's way is faster:

julia> @time main(300);
  0.129429 seconds (2.69 k allocations: 123.049 MiB, 19.75% gc time)

@odow odow merged commit 02083f6 into master Mar 4, 2021
@odow odow deleted the od/vaf_iterator branch March 4, 2021 20:37
@blegat blegat added this to the v0.9.21 milestone May 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

Performance issue with Quadratic objective to SOC constraint ScalarFunctionIterator has quadratic complexity

2 participants