jump-dev / JuMP.jl

Modeling language for Mathematical Optimization (linear, mixed-integer, conic, semidefinite, nonlinear)
http://jump.dev/JuMP.jl/
Other
2.17k stars 390 forks source link

[breaking] throw error if Containers.SparseAxisArray used in constraint function #3680

Closed odow closed 4 months ago

odow commented 4 months ago

Closes https://github.com/jump-dev/JuMP.jl/issues/3678

This PR is the nuclear option of throwing an error. Technically, this is breaking change, but it blocks a high risk of incorrect usage.

We should look at the tests and extension-tests.yml before deciding to merge this: https://github.com/jump-dev/JuMP.jl/actions/runs/7924586714

We could make this less breaking by adding methods like:

function build_constraint(
    ::Function,
    f::Containers.SparseAxisArray{<:Union{Number,AbstractJuMPScalar},1},
    s::MOI.Nonnegatives,
)
    return VectorConstraint(f, s)
end

function build_constraint(
    ::Function,
    f::Containers.SparseAxisArray{<:Union{Number,AbstractJuMPScalar},1},
    s::MOI.Nonpositives,
)
    return VectorConstraint(f, s)
end

function build_constraint(
    ::Function,
    f::Containers.SparseAxisArray{<:Union{Number,AbstractJuMPScalar},1},
    s::MOI.Zeros,
)
    return VectorConstraint(f, s)
end

but even then, things like the constraint dual might be in the "wrong" order.

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (a21e616) 98.33% compared to head (ae03657) 98.33%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #3680 +/- ## ======================================= Coverage 98.33% 98.33% ======================================= Files 43 43 Lines 5696 5704 +8 ======================================= + Hits 5601 5609 +8 Misses 95 95 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

odow commented 4 months ago

So this is a tricky decision. We didn't use it in any tests, and it was never used by any extension that we test.

I wonder how common it is in user-land.

odow commented 4 months ago

An alternative is to fix the iteration ordering with an OrderedDict.

odow commented 4 months ago

Closing in favor of #3681