Skip to content

Conversation

odow
Copy link
Member

@odow odow commented Oct 18, 2022

This PR adds a fallback for ListOfConstraintIndices and NumberOfConstraints to return default values in the case that the solver does't support the particular function-set combination.

This has come up a few times:

Closes #2011
Closes #2010
Follow-up to #2008

We already had something similar in two places:

function MOI.get(
model::StructOfConstraints,
attr::MOI.ListOfConstraintIndices{F,S},
) where {F,S}
if !MOI.supports_constraint(model, F, S)
return MOI.ConstraintIndex{F,S}[]
end
return MOI.get(constraints(model, F, S), attr)
end

function MOI.get(
model::StructOfConstraints,
attr::MOI.NumberOfConstraints{F,S},
)::Int64 where {F,S}
if !MOI.supports_constraint(model, F, S)
return 0
end
return MOI.get(constraints(model, F, S), attr)
end

#2008 added it to two more:

function MOI.get(
model::AbstractModel,
attr::MOI.NumberOfConstraints{MOI.VariableIndex,S},
)::Int64 where {S}
if !MOI.supports_constraint(model, MOI.VariableIndex, S)
return 0
end
return MOI.get(model.variables, attr)
end

function MOI.get(
model::AbstractModel,
attr::MOI.ListOfConstraintIndices{MOI.VariableIndex,S},
) where {S}
if !MOI.supports_constraint(model, MOI.VariableIndex, S)
return MOI.ConstraintIndex{MOI.VariableIndex,S}[]
end
return MOI.get(model.variables, attr)
end

So this PR is essentially just making some implied behavior concrete.

I've tested with Gurobi:

julia> model = Gurobi.Optimizer();

julia> MOI.Test.test_attribute_unsupported_list_of_constraint_indices(model, config)

But once this PR is open, I'll run SolverTests to see the damage: jump-dev/SolverTests#19

@odow
Copy link
Member Author

odow commented Oct 18, 2022

No issues identified by SolverTests, jump-dev/SolverTests#19, so I'd say this is okay.

@blegat
Copy link
Member

blegat commented Oct 19, 2022

Since all MOI wrappers tested by SolverTests are correctly implemented, this is not different from #2011.
However, as detailed in #2011 (comment), I don't see in which situation, it is a good thing to add a fallback for supported constraint types.
If a solver supports a constraint type and does not implement the getter, returning zero is never a good implementation, a clear error saying that this is not implemented by the MOI implementation so a cache should be used if that feature is needed is what is most suited in that situation

@odow
Copy link
Member Author

odow commented Oct 19, 2022

Added a GetAttributeNotAllowedError

@blegat
Copy link
Member

blegat commented Oct 20, 2022

supports_constraint is not enough, consider for instance CSDP with VectorOfVariables in PSD, we need some condition like in #2011

@odow
Copy link
Member Author

odow commented Oct 21, 2022

Done.

Copy link
Member

@blegat blegat left a comment

Choose a reason for hiding this comment

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

Looks good, maybe add a comment in the code since it is not so obvious why we are doing what we are doing ?

@odow odow merged commit e9044c5 into master Oct 24, 2022
@odow odow deleted the od/unsupported-constraint branch October 24, 2022 01:41
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.

2 participants