Skip to content

Conversation

odow
Copy link
Member

@odow odow commented Feb 14, 2021

In order to close jump-dev/JuMP.jl#1943, we need a way of checking whether it is possible to push the function constant into the set. The easiest way is just to define this fallback, so the JuMP code will become:

function build_constraint(
    ::Function,
    expr::Union{GenericAffExpr, GenericQuadExpr},
    set::MOI.AbstractScalarSet,
)
    offset = constant(expr)
    new_set = MOIU.shift_constant(set, -offset)
    if new_set === nothing
        return ScalarConstraint(expr, set)
    else
        add_to_expression!(expr, -offset)
        return ScalarConstraint(expr, new_set)
    end
end

This makes JuMP's heuristic of "I'll push the constant into the set" well-defined by "does it implement shift_constant".

As a follow-up question: should this be moved to MOI proper?

@blegat
Copy link
Member

blegat commented Feb 15, 2021

I'm hesitant between this has adding a supports_constant_shift(::Type{<:MOI.AbstractScalarSet}) = false. The advantage of this PR is that it's not possible to implement shift_constant but forget to implement supports_constant_shift. The advantage of supports_constant_shift is that it can be checked when we only have the type of the set. A disadvantage of this PR is that if the user pass a constant of incorrect type, it might use the fallback even if there is a shift implemented.

@odow
Copy link
Member Author

odow commented Feb 15, 2021

it's not possible to implement shift_constant but forget to implement supports_constant_shift.

I always get confused with out supports_xxx.

A disadvantage of this PR is that if the user pass a constant of incorrect type, it might use the fallback even if there is a shift implemented.

If they pass the incorrect type, then the shift implemented will not work anyway.

For example: shift_constant(MOI.EqualTo{Int}(1), 0.5). We shouldn't just check that MOI.EqualTo{Int} supports the shift.

@blegat
Copy link
Member

blegat commented Feb 17, 2021

If they pass the incorrect type, then the shift implemented will not work anyway.

Yes but shift_constant(MOI.EqualTo(1), 2.5) should probably error or return MOI.EqualTo(3.5), not nothing

@odow
Copy link
Member Author

odow commented Feb 17, 2021

Returning nothing says that it isn't okay to shift the constant into the set. Otherwise we need to define supports_shift_constant(::Type{Set}, ::Type{T}).

And EqualTo could define to implement shift_contant(set::MOI.EqualTo, shift::T) = MOI.EqualTo(set.value + shift) if it so chose.

It really, do you want to implement an extra supports_shift_constant method, or do you want a nothing fallback?

@blegat
Copy link
Member

blegat commented Feb 19, 2021

Some user might also not see the error when nothing is returned, for instance if they store the result in a Any[]. Also, we might need to check whether the set can be shifted only with the set type (e.g. in concrete_bridge_type of a bridge).

@odow
Copy link
Member Author

odow commented Feb 21, 2021

Some user might also not see the error when nothing is returned, for instance if they store the result in a Any[]

This is user-error.

Also, we might need to check whether the set can be shifted only with the set type

We haven't needed to yet?

@blegat
Copy link
Member

blegat commented Feb 23, 2021

This is user-error.

Why do you mean ? The user should never have Any[] vectors ?

We haven't needed to yet?

No, just speculative

@odow
Copy link
Member Author

odow commented Feb 23, 2021

Why do you mean?

If the user doesn't check for a return of nothing, they're using the function wrong. Or you mean this is breaking because currently it will error?

@odow odow changed the title [Utilities] add a default for shift_constant [Utilities] add supports_shift_constant Mar 2, 2021
@odow odow merged commit e9b03cb into master Mar 2, 2021
@odow odow deleted the od/shift_constant branch March 2, 2021 20:38
@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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

[Needs MOI 0.9.21] AffExpr in ZeroOne does not work

2 participants