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

Add CanonicalConstraintFunction attribute #1118

Merged
merged 6 commits into from
Jul 24, 2020

Conversation

blegat
Copy link
Member

@blegat blegat commented Jul 10, 2020

For models that knows that the constraint function is already canonical they can just

function MOI.get(model::ModelType, ::Union{MOI.ConstraintFunction, MOI.CanonicalConstraintFunction}, ci)
    ...
end

For MOI.Utilities.Model, the function are not canonicalized when added (should we ?) so we canonicalize them then CanonicalConstraintFunction is called to spare the copy that is done in MOI.Utilities.canonical as it can slows things down dramatically (see jump-dev/SCS.jl#186).
Alternatively, we could say that the functions added to MOI.Utilities.Model are automatically canonicalized in addition of being copied by replacing this copy(f) by canonical(f):

push!(model.constrmap, _add_constraint(model, ci, copy(f), copy(s)))

src/attributes.jl Show resolved Hide resolved
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.

I thought we agreed that solvers should just check if the function was canonicalized. What is the benefit of having ConstraintFunction and CanonicalizedConstraintFunction?

Couldn't we just make it that all solvers return canonicalized functions?

@joaquimg
Copy link
Member

Many times we dont need cannonicalized functions.
I think that forcing that all the time is a overkill.
Better to let one use is_cannonical if needed.

@blegat
Copy link
Member Author

blegat commented Jul 16, 2020

Any objection to merge ?

struct CanonicalConstraintFunction <: AbstractConstraintAttribute end

function get_fallback(model::ModelLike, ::CanonicalConstraintFunction, ci::ConstraintIndex)
return Utilities.canonical(get(model, ConstraintFunction(), ci))
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the is_canonical check go in this fallback? It would save the checks in Model and UniversalFallback.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done :)

@blegat blegat merged commit cc68d63 into master Jul 24, 2020
@blegat blegat changed the title [RFC] Add CanonicalConstraintFunction attribute Add CanonicalConstraintFunction attribute Jul 24, 2020
@blegat blegat added this to the v0.9.15 milestone Jul 24, 2020
@odow odow deleted the bl/canonical_constraint_function branch September 2, 2020 21:23
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.

3 participants