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

Ternary operator inside macros #324

Closed
joaquimg opened this issue Jan 23, 2025 · 12 comments
Closed

Ternary operator inside macros #324

joaquimg opened this issue Jan 23, 2025 · 12 comments

Comments

@joaquimg
Copy link
Member

Many users use the ternary operator (? :) inside @expresion and @constraint.
A very common usage is optional slack variables.

The following example illustrates that usage.
JuMP has options to do it efficiently, writing different expressions in a loop or using add_to_expression!. But:

  • The downside of writing separate expressions is that there might be much more code replication if the constraint is more complicated.
  • The downside of add_to_expression! is readability.
  • ifelse is not an option as the variable frequently does not exist in this cases.

--

As an alternative to:
@expression(model, 2x[1] + (iseven(j) ? 3x[j] : 0))

Maybe we can to something more clever? such as:

@expression(model, 2x[1] + @zero_if_not(iseven(j), 3x[j]))

--

Here is the example:

using JuMP
function test1()
    model = Model()
    N = 1_000_000
    @variable(model, x[1:N])
    _zero_exp = JuMP.AffExpr(0.0)
    for i in 1:5
        @show i
        GC.gc()
        GC.gc()
        @time for j in 1:N
            if iseven(j)
                @expression(model, 2x[1] + 3x[j])
            else
                @expression(model, 2x[1])
            end
        end
        GC.gc()
        GC.gc()
        @time for j in 1:N
            e = @expression(model, 2x[1])
            if iseven(j)
                add_to_expression!(e, 3, x[j])
            end
        end
        GC.gc()
        GC.gc()
        @time for j in 1:N
            @expression(model, 2x[1] + (iseven(j) ? 3x[j] : 0))
        end
    end
end
test1()

Which results in:

i = 1
  0.415310 seconds (7.00 M allocations: 549.316 MiB)
  0.424566 seconds (7.00 M allocations: 549.316 MiB)
  0.584483 seconds (10.50 M allocations: 823.975 MiB, 19.22% gc time)
i = 2
  0.337471 seconds (7.00 M allocations: 549.316 MiB)
  0.446796 seconds (7.00 M allocations: 549.316 MiB)
  0.591993 seconds (10.50 M allocations: 823.975 MiB, 21.81% gc time)
i = 3
  0.417391 seconds (7.00 M allocations: 549.316 MiB)
  0.437248 seconds (7.00 M allocations: 549.316 MiB)
  0.595859 seconds (10.50 M allocations: 823.975 MiB, 18.18% gc time)
i = 4
  0.329041 seconds (7.00 M allocations: 549.316 MiB)
  0.345533 seconds (7.00 M allocations: 549.316 MiB)
  0.670093 seconds (10.50 M allocations: 823.975 MiB, 23.70% gc time)
i = 5
  0.424246 seconds (7.00 M allocations: 549.316 MiB)
  0.324499 seconds (7.00 M allocations: 549.316 MiB)
  0.749065 seconds (10.50 M allocations: 823.975 MiB, 15.93% gc time)
@odow
Copy link
Member

odow commented Jan 23, 2025

Check against @expression(model, 2x[1] + if iseven(j); 3x[j] else 0 end))

@odow odow transferred this issue from jump-dev/JuMP.jl Jan 23, 2025
@odow
Copy link
Member

odow commented Jan 24, 2025

So I think we already do the clever thing here, because ? : is just syntactic sugar for an Expr structure we already optimize:

julia> dump(:(a ? b : c))
Expr
  head: Symbol if
  args: Array{Any}((3,))
    1: Symbol a
    2: Symbol b
    3: Symbol c

julia> dump(:(if a; b else c end))
Expr
  head: Symbol if
  args: Array{Any}((3,))
    1: Symbol a
    2: Expr
      head: Symbol block
      args: Array{Any}((2,))
        1: LineNumberNode
          line: Int64 1
          file: Symbol REPL[54]
        2: Symbol b
    3: Expr
      head: Symbol block
      args: Array{Any}((2,))
        1: LineNumberNode
          line: Int64 1
          file: Symbol REPL[54]
        2: Symbol c

Check:

julia> model = Model();

julia> @variable(model, x[1:2])
2-element Vector{VariableRef}:
 x[1]
 x[2]

julia> j = 2
2

julia> @allocated @expression(model, (iseven(j) ? 3x[j] : 0) + 2x[1])
640

julia> @allocated @expression(model, 2x[1] + (iseven(j) ? 3x[j] : 0))
1216

julia> @allocated @expression(model, 2x[1] + 3x[j])
640

The issue is that if the if block is on the right-hand side, we first do 2x[1], but then we rewrite the inner if block, and then finally sum them together.

Trying to merge the outer expression with the inner part of the if block seems problematic. Especially because people write code like:

@expression(model, 2x[1] + (iseven(j) ? (a = 3; b = x[j]; a * b) : 0))

we'd need to track that the outer operator is + and we need to rewrite the last expression of the if block with that in mind.

@odow
Copy link
Member

odow commented Jan 24, 2025

I think we can just close this as won't fix.

@joaquimg
Copy link
Member Author

Since conditionals are already optimized, I would like to bring back the alternative of the macro kind of solution such as @zero_else

@odow
Copy link
Member

odow commented Jan 25, 2025

Strongly against. The macro is more characters:

@zero_else(iseven(j), 3x[j])
(iseven(j) ? 3x[j] : 0)

For the common case of out-of-bounds access, you can also do:

julia> model = Model();

julia> @variable(model, x[1:2]);

julia> @expression(model, [i in 1:2], (i > 1 ? x[i-1] : 0) + x[i])
2-element Vector{AffExpr}:
 x[1]
 x[1] + x[2]

julia> @expression(model, [i in 1:2], get(x, i-1, 0) + x[i])
2-element Vector{AffExpr}:
 x[1]
 x[1] + x[2]

@blegat
Copy link
Member

blegat commented Jan 27, 2025

The issue is that users might accidentally write the slow version. If we add another macro, since the users won't know about it, they will still be able to write the slow version.
I'm not sure there is much we can do here.

@odow
Copy link
Member

odow commented Jan 27, 2025

It's also the case that the most common use-case is something like a variable or 0.0, so there's no penalty for writing either order. It matters only if there is a large expression inside the if statement.

@blegat
Copy link
Member

blegat commented Jan 28, 2025

If it's a variable or zero then you should do 3 * (iseven(j) ? x[j] : 0) 😆
We could have a special case for that that factors out the common factor if that's so common

@odow
Copy link
Member

odow commented Jan 28, 2025

What's the point of all this? It's up to the user to write nice code. This is, at best, a small edge case that does not have performance implications. I don't want a special case.

The if statement issue was GenX writing code like this: https://github.com/GenXProject/GenX.jl/blob/0efaff1b9de5529b001facd4f15aaeee960e33e2/src/model/core/discharge/investment_discharge.jl#L71-L106

    @expression(EP, eTotalCap[y in 1:G],
        if y in intersect(NEW_CAP, RET_CAP, RETROFIT_CAP) # Resources eligible for new capacity, retirements and being retrofitted
            if y in COMMIT
                eExistingCap[y] +
                cap_size(gen[y]) * (EP[:vCAP][y] - EP[:vRETCAP][y] - EP[:vRETROFITCAP][y])
            else
                eExistingCap[y] + EP[:vCAP][y] - EP[:vRETCAP][y] - EP[:vRETROFITCAP][y]
            end
        elseif y in intersect(setdiff(RET_CAP, NEW_CAP), setdiff(RET_CAP, RETROFIT_CAP)) # Resources eligible for only capacity retirements
            if y in COMMIT
                eExistingCap[y] - cap_size(gen[y]) * EP[:vRETCAP][y]
            else
                eExistingCap[y] - EP[:vRETCAP][y]
            end
        elseif y in setdiff(intersect(RET_CAP, NEW_CAP), RETROFIT_CAP) # Resources eligible for retirement and new capacity
            if y in COMMIT
                eExistingCap[y] + cap_size(gen[y]) * (EP[:vCAP][y] - EP[:vRETCAP][y])
            else
                eExistingCap[y] + EP[:vCAP][y] - EP[:vRETCAP][y]
            end
        elseif y in setdiff(intersect(RET_CAP, RETROFIT_CAP), NEW_CAP) # Resources eligible for retirement and retrofitting
            if y in COMMIT
                eExistingCap[y] -
                cap_size(gen[y]) * (EP[:vRETROFITCAP][y] + EP[:vRETCAP][y])
            else
                eExistingCap[y] - (EP[:vRETROFITCAP][y] + EP[:vRETCAP][y])
            end
        elseif y in intersect(setdiff(NEW_CAP, RET_CAP), setdiff(NEW_CAP, RETROFIT_CAP))  # Resources eligible for only new capacity
            if y in COMMIT
                eExistingCap[y] + cap_size(gen[y]) * EP[:vCAP][y]
            else
                eExistingCap[y] + EP[:vCAP][y]
            end
        else # Resources not eligible for new capacity or retirement
            eExistingCap[y]
        end)

That's a much more impactful case than a few flag ? x : 0.

@joaquimg
Copy link
Member Author

The most common are indeed + (flag ? variable : 0.0) and - (flag ? variable : 0.0).
They can be very frequent.

@odow
Copy link
Member

odow commented Jan 28, 2025

In which case, rewriting has no benefit. And IMO, adding zero_else just make the code harder to read because it introduces more special syntax.

@blegat
Copy link
Member

blegat commented Jan 29, 2025

Yes, it also make it sounds like there is something sophisticated behind while the simplest thing (ternary) already works so I don't think we should do anything

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

No branches or pull requests

3 participants