Add a few missing methods for AbstractJuMPScalar to support e.g. Distances.jl

[LebedevRI]

Currently, AbstractJuMPScalar is somewhat partial, and e.g. Distances.jl does not want to play along:

using JuMP, Ipopt, Distances
model = Model(Ipopt.Optimizer)
@variable(model, a)
@variable(model, b)
euclidean(a, b)

Results in:

ERROR: MethodError: no method matching length(::VariableRef)

Closest candidates are:
  length(::Union{Base.KeySet, Base.ValueIterator})
   @ Base abstractdict.jl:58
  length(::Union{LinearAlgebra.Adjoint{T, S}, LinearAlgebra.Transpose{T, S}} where {T, S})
   @ LinearAlgebra /builddirs/julia/usr/share/julia/stdlib/v1.9/LinearAlgebra/src/adjtrans.jl:295
  length(::Union{SparseArrays.FixedSparseVector{Tv, Ti}, SparseArrays.SparseVector{Tv, Ti}} where {Tv, Ti})
   @ SparseArrays /builddirs/julia/usr/share/julia/stdlib/v1.9/SparseArrays/src/sparsevector.jl:95
  ...

That is solved by providing Base.length(::AbstractJuMPScalar) = 1

But fixing that we hit yet another issue,
with a call to oneunit() with bogus Type{Any}.
There, the fix appears to be providing Base.IteratorEltype() / Base.eltype(),
this looks a bit weird to me, but seems symmetrical with e.g. Int32?

And then we finally hit the missing Base.oneunit(a::GenericAffExpr),
and since i don't believe there are any types here,
it's identical to the one().

Afterwards, we finally get the right answer:

julia> euclidean(a, b)
sqrt(a² - 2 b*a + b²)

Thanks!

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (94ba2dd) 98.39% compared to head (0405c3c) 98.39%.
Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3585   +/-   ##
=======================================
  Coverage   98.39%   98.39%           
=======================================
  Files          38       38           
  Lines        5655     5664    +9     
=======================================
+ Hits         5564     5573    +9     
  Misses         91       91           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[odow]

I would just add tests for the new methods that you've added. I don't think we should add Distances.jl as a test dependency just for this.

[LebedevRI]

I would just add tests for the new methods that you've added. I don't think we should add Distances.jl as a test dependency just for this.

Aha, doing that now...

[odow]

This is another example where Julia's lack of a formal interface specification hurts us. It's really hard to know what third-party packages expect from the implementer of a set of methods.

[LebedevRI]

FWIW, i'm a bit surprised AbstractJuMPScalar does not derive from Real.
I will be really surprised if i manage to convince Measurements.jl to support this :(
(JuliaPhysics/Measurements.jl#142 / JuliaPhysics/Measurements.jl#144)

[odow]

FWIW, i'm a bit surprised AbstractJuMPScalar does not derive from Real.

Because they can be things other than Real, like complex-valued expressions.

In general, there is some need in Julia for struct Foo{T} <: T, but this isn't supported, so we cannot make a lot of things work (like your Measurements stuff).

As a general comment, the composability of Julia is beneficial, but it often leads to more problems that it is worth. JuMP has been very conservative in how we interact with other Julia packages. When we find things that are broken, we'll generally improve the error message rather than enable new features.

[LebedevRI]

@odow thank you!

[LebedevRI]

FWIW, i'm a bit surprised AbstractJuMPScalar does not derive from Real.

Because they can be things other than Real, like complex-valued expressions.

Right, but so what, so can the Symbolic.Num-based symbolic variables.
It's not like the code that receives symbolic variable
can detect/decide if it should handle it as Complex or not.

In general, there is some need in Julia for struct Foo{T} <: T, but this isn't supported, so we cannot make a lot of things work (like your Measurements stuff).

Ah, i guess i see what the bigger picture/problem is now:
AbstractJuMPScalar is derived from MutableArithmetics.AbstractMutable,
which would need said struct Foo{T} <: T.

As a general comment, the composability of Julia is beneficial, but it often leads to more problems that it is worth. JuMP has been very conservative in how we interact with other Julia packages. When we find things that are broken, we'll generally improve the error message rather than enable new features.

But i'm rather disappointed that JuMP has to invent it's own Symbolics.jl instead using the existing one.
Just 2c.

[odow]

Ah, i guess i see what the bigger picture/problem is now

Yes 😄. As a general comment, we've put a lot of thought into the design of JuMP. If something seems odd, it's probably that way for a reason.

But i'm rather disappointed that JuMP has to invent its own Symbolics.jl instead using the existing one.

Development of JuMP started in 2013, long before Symbolics existed. When we added GennericNonlinearExpr, we looked into Symbolics. Ultimately, we decided that Symbolics was not the right choice for JuMP. See #3106 for a slightly long conversation...