Skip to content

Conversation

@blegat
Copy link
Member

@blegat blegat commented May 9, 2021

As this is taking a non-negligeable time in #1287, it is worth optimizing.
Consider the benchmark:

function bench(n)
    x = MOI.VariableIndex.(1:n)
    f = MOI.ScalarAffineFunction(MOI.ScalarAffineTerm.(ones(length(x)), x), 0.0)
    @btime MOI.Utilities.is_canonical($f)
end

Before:

julia> bench(10000)
  8.769 μs (0 allocations: 0 bytes)

Initial suggestion:

julia> bench(10000)
  6.477 μs (0 allocations: 0 bytes)

@odow improved implementation:

julia> bench(10000)
  5.808 μs (0 allocations: 0 bytes)

Copy link
Member

@odow odow left a comment

Choose a reason for hiding this comment

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

Does it really make that much of a difference for #1287? It's not often we have functions with 10k terms.

@joaquimg
Copy link
Member

joaquimg commented May 9, 2021

This seems to be proportional to the overall number of non-zeros, not just in one function.

@odow
Copy link
Member

odow commented May 9, 2021

Before

julia> bench(10_000)
  10.609 μs (0 allocations: 0 bytes)

After

julia> bench(10_000)
  5.383 μs (0 allocations: 0 bytes)
function MathOptInterface.Utilities.is_strictly_sorted(x::Vector, by, filter)
    if isempty(x)
        return true
    end
    current_x = first(x)
    if !filter(current_x)
        return false
    end
    current_fx = by(current_x)
    @inbounds for i in eachindex(x)[2:end]
        next_x = x[i]
        if !filter(next_x)
            return false
        end
        next_fx = by(next_x)
        if next_fx <= current_fx
            return false
        end
        current_x, current_fx = next_x, next_fx
    end
    return true
end

@odow
Copy link
Member

odow commented May 9, 2021

Actually this was better

function MathOptInterface.Utilities.is_strictly_sorted(x::Vector, by, filter)
    if isempty(x)
        return true
    end
    @inbounds current_x = first(x)
    if !filter(current_x)
        return false
    end
    current_fx = by(current_x)
    for i in eachindex(x)[2:end]
        @inbounds next_x = x[i]
        if !filter(next_x)
            return false
        end
        next_fx = by(next_x)
        if next_fx <= current_fx
            return false
        end
        current_x, current_fx = next_x, next_fx
    end
    return true
end

There's too much noise with only 10k terms.

@blegat
Copy link
Member Author

blegat commented May 9, 2021

This is the part of copying to Clp.Model for the benchmark of #1287:
strictly_sorted

@odow odow merged commit 22240f0 into master May 9, 2021
@odow odow deleted the bl/is_strictly_inc branch May 9, 2021 21:50
@blegat blegat added this to the v0.10 milestone May 22, 2021
blegat added a commit that referenced this pull request May 22, 2021
blegat added a commit that referenced this pull request May 22, 2021
blegat added a commit that referenced this pull request May 23, 2021
@blegat blegat modified the milestones: v0.10, v0.9.22 May 25, 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.

4 participants