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

Build faster paths for SAF constructions #305

Closed
matbesancon opened this issue Oct 10, 2024 · 5 comments · Fixed by #306
Closed

Build faster paths for SAF constructions #305

matbesancon opened this issue Oct 10, 2024 · 5 comments · Fixed by #306

Comments

@matbesancon
Copy link

I was repeatedly solving large LPs that were fast enough and the bottleneck ended up being the SAF constructions.
Specifically, for λ a vector of variable indices:

dummy_objective = sum(λ, init=0.0)

was noticeably slow when profiling, and:

nv = length(atoms)
for i in 1:nv
    lhs[i] = 2 * sum(λ[j] * dot(atoms[i], atoms[j]) for j in 1:nv)
    rhs[i] = -dot(atoms[i], b)
end

was also extremely slow (taking more time than a solve. The length of atoms was in the hundreds.

For both, the bulk of the time was spent in the SAF construction, MA.operate, etc

@matbesancon
Copy link
Author

Some potential improvements:

  1. a MOI manual section on performance gotchas with deceptively slow operations
  2. specializing at least common ones? sum and dot for instance
  3. ?

@blegat
Copy link
Member

blegat commented Oct 10, 2024

We have specialized sum, but you don't hit it because of the keyword argument, that should be fixed in #306
You can also use the following, that's what MA will do once that PR is merged:

λ = MOI.VariableIndex.(1:n)
mapreduce(identity, MA.add!!, λ; init = 0.0)

For the second example, use dot, it will use an optimized method in MA:

nv = length(atoms)
for i in 1:nv
    lhs[i] = dot(λ, [2dot(atoms[i], atoms[j]) for j in 1:nv])
    rhs[i] = -dot(atoms[i], b)
end

@blegat blegat transferred this issue from jump-dev/MathOptInterface.jl Oct 10, 2024
@matbesancon
Copy link
Author

you do need the initial argument though, otherwise it doesn't work for summing variable indices:

julia> sum(l)
ERROR: MethodError: no method matching zero(::Type{MathOptInterface.VariableIndex})
The function `zero` exists, but no method is defined for this combination of argument types.

@matbesancon
Copy link
Author

For the second example, use dot, it will use an optimized method in MA:

the only issue is that you need to allocate a whole vector just to construct another SAF which itself will also allocate

@blegat
Copy link
Member

blegat commented Oct 10, 2024

Yes, you need the init, that's why we need #306

You don't need to allocate anything. dot calls fused_map_reduce which needs getindex to be defined so you cannot use Base.Generator but you can use MOI.Utilities.VectorLazyMap. You can also simply do:

mapreduce(identity, (acc, j) -> add_mul!!(acc, λ[j], 2dot(atoms[i], atoms[j])), 1:nv; init = 0.0)

Or just write the for loop calling add_mul!!

@odow odow closed this as completed in #306 Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants