Skip to content

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

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

Merged
merged 7 commits into from
Mar 4, 2021
Merged

Conversation

odow
Copy link
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
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
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
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
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
Member

blegat commented Mar 4, 2021

This assumes that the function is canonicalized?

No

@odow
Copy link
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