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

Multiplying JuMP expression with UniformScaling absorbs the matrix #3553

Closed
blegat opened this issue Oct 31, 2023 · 7 comments · Fixed by #3557
Closed

Multiplying JuMP expression with UniformScaling absorbs the matrix #3553

blegat opened this issue Oct 31, 2023 · 7 comments · Fixed by #3557
Labels

Comments

@blegat
Copy link
Member

blegat commented Oct 31, 2023

julia> @variable(model, x)

julia> rand(2, 2) - x * I
ERROR: MethodError: no method matching -(::Matrix{Float64}, ::AffExpr)

Closest candidates are:
  -(::RationalPoly, ::Any)
   @ MultivariatePolynomials ~/.julia/packages/MultivariatePolynomials/6TYc8/src/rational.jl:73
  -(::V, ::GenericAffExpr{C, V}) where {C, V<:AbstractVariableRef}
   @ JuMP ~/.julia/packages/JuMP/D44Aq/src/operators.jl:146
  -(::GenericQuadExpr, ::GenericAffExpr)
   @ JuMP ~/.julia/packages/JuMP/D44Aq/src/operators.jl:361
  ...

Stacktrace:
 [1] top-level scope
   @ REPL[39]:1

x * I cannot be a UniformScaling since the scaling should be a Number but and we cannot create a Diagonal([x, x, ...]) since we don't know yet what is the dimension we'll need.

@odow
Copy link
Member

odow commented Oct 31, 2023

I don't know what we can or should do here. The answer is to force people to use I(2).

@odow
Copy link
Member

odow commented Oct 31, 2023

I don't really want to add MutableArithmetics.UniformScaling{<:AbstractMutable} and have to deal with all the consequences?

@blegat
Copy link
Member Author

blegat commented Oct 31, 2023

Yes it seems like a rabbit hole. Maybe we could try to convince LinearAlgebra to accept Any instead of Number. I think they thought that they are only implementing * with Number. This is fine but they can still allow Any in the constructor when it's called explicitly. In a Matrix they didn't force the entries to be Number so why do it in UniformScaling ?

@odow
Copy link
Member

odow commented Oct 31, 2023

Perhaps open an issue? This will be easier with upgradable stdlibs.

@odow odow added the upstream label Nov 1, 2023
@odow
Copy link
Member

odow commented Nov 1, 2023

Closing as too hard to fix. It can't be a common issue, because I don't remember it coming up on Discourse.

@odow odow closed this as completed Nov 1, 2023
@jd-foster
Copy link
Collaborator

jd-foster commented Nov 2, 2023

Could this give a better error message? For example,

Base.:-(a::Matrix, b::Union{JuMP.GenericVariableRef, JuMP.GenericAffExpr}) = error(
   "Subtraction between a Matrix and a JuMP variable is not supported: instead of A - x, " *
   "do A .- x for element-wise subtraction, or if you are modifying the diagonal entries of the matrix " *
   " do A - x * LinearAlgebra.I(n), where n is the diagonal length."
)

@odow odow reopened this Nov 2, 2023
@blegat
Copy link
Member Author

blegat commented Nov 2, 2023

Yes, I think such error message for :+(::Matrix, ::AbstractJuMPScalar) would be useful

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

Successfully merging a pull request may close this issue.

3 participants