isapprox bug, and doubts regarding container methods

[hustf]

On Julia 1.8.1 (and also on 1.7.3)

using Unitful; using Unitful:kg

julia> A = [1 2kg; 3/kg 4]
2×2 Matrix{Quantity{Int64}}:
       1  2 kg
 3 kg^-1     4

julia> A == A
true

julia> A ≈ A
false

julia> A ≈ 2A
ERROR: DimensionError:  and kg^-1 are not dimensionally compatible.
...

julia> A .≈ A
2×2 BitMatrix:
 1  1
 1  1

I'm not able to figure out the reason for quantity.jl:327 at all. Is it a temporary placeholder introduced with AbstractQuantity? Is it a fallback which should never be fallen back on?

 isapprox(x::AbstractArray{S}, y::AbstractArray{T};
    kwargs...) where {S <: AbstractQuantity,T <: AbstractQuantity} = false

Generally on methods for containers: Defining methods on containers opens for a large number of potential errors. E.g. are two containers similar when the contents are similar and container types differ? (note that typeof(2A) ≠ typeof(A) ? There's no definite and expected behaviour to questions like that.

When dealing with quantities and dimensionality, strictly speaking, this (or something like a potential UnitfulCore.jl) is the authorative package. But containers of quantities have zillions of different meanings depending on context and preferences, and should behave differently according to context. Sometimes, there's no obvious definition covering all use cases, like for 'norm', 'zero' or 'approx'.

[sostock]

That method is definitely wrong and should be removed. If I had to guess, I would say it was added for cases like [1m, 2m] ≈ [1s, 2s] and the author of that method just didn’t consider arrays with non-concrete eltype, or containers with different types in general.

The A ≈ A in the above example should error (and it does if one defines A as a Matrix{Any}), since neither zero(eltype(A)) nor norm(A) do exist (unless one uses the norm keyword argument to specify a norm that can handle these kinds of heterogeneous arrays consistently).

The first part of this is a duplicate of #310, but the last two paragraphs are more general. In addition to deleting the isapprox method in question, we should audit all container methods (cf. https://github.com/PainterQubits/Unitful.jl/pull/544#discussion_r969231326 for another method that should be removed).

[hustf]

I'll submit a minimal PR for isapprox. The container stuff would perhaps be nice to have insights from more people on first?

[hustf]

See the pull request to my existing fork Unitfu, linked above. Tests are still running. I was not allowed to create more fresh forks of Unitful.jl, so pulling this into Unitful master, if accepted, would require a merge.

The error we would get with this change are not very friendly, but at least it is obvious to the determined reader that Unitful doesn't dispatch on the container type:

julia> [1 2kg] ≈ [1 2kg]
ERROR: DimensionError: 0.0kg and 0.0 are not dimensionally compatible.
Stacktrace:
  [1] _isless
    @ C:\Users\frohu_h4g8g6y\.julia\dev\Unitfu\src\quantities.jl:288 [inlined]
....
 [16] isapprox(x::Matrix{Quantity{Int64}}, y::Matrix{Quantity{Int64}})
    @ LinearAlgebra C:\Users\frohu_h4g8g6y\AppData\Local\Programs\Julia-1.8.1\share\julia\stdlib\v1.8\LinearAlgebra\src\generic.jl:1696
 [17] top-level scope
    @ REPL[3]:1

Perhaps the general documentation should point out that norms and distances with units are context dependant and left to downstream packages?

[oschulz]

Here's another case for this:

julia> A = [0.0u"s":0.1u"s":1.0u"s"]
1-element Vector{StepRangeLen{Quantity{Float64, 𝐓, Unitful.FreeUnits{(s,), 𝐓, nothing}}, Base.TwicePrecision{Quantity{Float64, 𝐓, Unitful.FreeUnits{(s,), 𝐓, nothing}}}, Base.TwicePrecision{Quantity{Float64, 𝐓, Unitful.FreeUnits{(s,), 𝐓, nothing}}}, Int64}}:
 (0.0:0.1:1.0) s

julia> isapprox(A, A)
ERROR: DimensionError: 2.9238189563674606e-8 s and 0.0 are not dimensionally compatible.