Unitful and Intevals causes trouble for Makie

[ptiede]

Makie version: 0.21.7

It looks like to_endpoints is missing a dispatch for uniful intervals. MWE

using Unitful
using CairoMakie

x = 1..3
y = 10u"km"..100u"km"
image(x, y, rand(3, 10))
ERROR: MethodError: no method matching to_endpoints(::Tuple{Quantity{Int64, 𝐋, Unitful.FreeUnits{(km,), 𝐋, nothing}}, Quantity{Int64, 𝐋, Unitful.FreeUnits{(km,), 𝐋, nothing}}})

Closest candidates are:
  to_endpoints(::Any, ::Any)
   @ Makie ~/.julia/packages/Makie/aG6xp/src/conversions.jl:372
  to_endpoints(::IntervalSets.ClosedInterval)
   @ Makie ~/.julia/packages/Makie/aG6xp/src/conversions.jl:368
  to_endpoints(::Union{IntervalSets.Interval, AbstractVector})
   @ Makie ~/.julia/packages/Makie/aG6xp/src/conversions.jl:369
  ...

Stacktrace:
  [1] to_endpoints(x::IntervalSets.ClosedInterval{Quantity{Int64, 𝐋, Unitful.FreeUnits{(km,), 𝐋, nothing}}})
    @ Makie ~/.julia/packages/Makie/aG6xp/src/conversions.jl:368
  [2] to_endpoints(x::IntervalSets.ClosedInterval{Quantity{Int64, 𝐋, Unitful.FreeUnits{(km,), 𝐋, nothing}}}, dim::String)
    @ Makie ~/.julia/packages/Makie/aG6xp/src/conversions.jl:375
  [3] convert_arguments(::ImageLike, xs::IntervalSets.ClosedInterval{Int64}, ys::IntervalSets.ClosedInterval{Quantity{…}}, data::Matrix{Float64})
    @ Makie ~/.julia/packages/Makie/aG6xp/src/conversions.jl:422
  [4] convert_arguments(::Type{Image}, ::IntervalSets.ClosedInterval{Int64}, ::Vararg{Any}; kw::@Kwargs{})
    @ Makie ~/.julia/packages/Makie/aG6xp/src/conversions.jl:26
  [5] convert_arguments(::Type{Image}, ::IntervalSets.ClosedInterval{Int64}, ::IntervalSets.ClosedInterval{Quantity{…}}, ::Matrix{Float64})
    @ Makie ~/.julia/packages/Makie/aG6xp/src/conversions.jl:14
  [6] conversion_pipeline(P::Type{…}, used_attrs::Tuple{}, args::Tuple{…}, args_obs::Tuple{…}, user_attributes::Dict{…}, deregister::Vector{…}, recursion::Int64)
    @ Makie ~/.julia/packages/Makie/aG6xp/src/interfaces.jl:223
  [7] conversion_pipeline(P::Type{…}, used_attrs::Tuple{}, args::Tuple{…}, args_obs::Tuple{…}, user_attributes::Dict{…}, deregister::Vector{…})
    @ Makie ~/.julia/packages/Makie/aG6xp/src/interfaces.jl:214
  [8] (Image)(user_args::Tuple{IntervalSets.ClosedInterval{…}, IntervalSets.ClosedInterval{…}, Matrix{…}}, user_attributes::Dict{Symbol, Any})
    @ Makie ~/.julia/packages/Makie/aG6xp/src/interfaces.jl:272
  [9] _create_plot(::Function, ::Dict{Symbol, Any}, ::IntervalSets.ClosedInterval{Int64}, ::Vararg{Any})
    @ Makie ~/.julia/packages/Makie/aG6xp/src/figureplotting.jl:316
 [10] image(::IntervalSets.ClosedInterval{Int64}, ::Vararg{Any}; kw::@Kwargs{})
    @ MakieCore ~/.julia/packages/MakieCore/fkSJS/src/recipes.jl:436
 [11] image(::IntervalSets.ClosedInterval{Int64}, ::Vararg{Any})
    @ MakieCore ~/.julia/packages/MakieCore/fkSJS/src/recipes.jl:434
 [12] top-level scope
    @ REPL[15]:1

versioninfo()
Julia Version 1.10.4
Commit 48d4fd48430 (2024-06-04 10:41 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: 32 × AMD Ryzen 9 7950X 16-Core Processor
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-15.0.7 (ORCJIT, znver3)
Threads: 1 default, 0 interactive, 1 GC (on 32 virtual cores)

[ptiede]

Adding the method does seem to correct this

Makie.to_endpoints(x::Tuple{Unitful.AbstractQuantity,Unitful.AbstractQuantity}) = (ustrip(x[1]), ustrip(x[2]))

[asinghvi17]

Hmm, this might need more dimension expanding. Unitful quantities should never be hitting convert_arguments in the first place

[ptiede]

Is it a expand_dimension method that is missing? I noticed that expand_dimensions only seems to be defined for abstract vectors and an interval isn't a subtype of that

https://github.com/MakieOrg/Makie.jl/blob/2b5931ee92c00603a669d02b8103bff49315024b/src/dim-converts/unitful-integration.jl#L7

I'm really new to the Makie codebase (but long time user) so I don't understand the expand_dimension and conversion pipeline yet.

[asinghvi17]

The expand dimensions pipeline is also super new 😄 but the idea is that all unit information is stripped by expand_dimensions, and then the only values passed into convert_arguments are unitless. This also allows us to ensure that all unitful quantities of the same dimension share the same unit / magnitude (so you don't have combinations of m and km, which don't make sense).

There are two main methods to target AFAIK - should_dim_convert and expand_dimensions. I have to step out now but will dive into the codebase tomorrow and send a better explanation.

The solution here is probably to support unitful intervals for x and y on ImageLike and GridBased conversion traits.

[ptiede]

Looking through the code one thing I noticed is that AbstractIntervals aren't being treated the same as AbstractVectors, especially w.r.t. get_element_type. Currently get_element_type will just return the interval and I would guess it should be

get_element_type(int::AbstractInterval{T}) where {T} = T

so that whatever conversion pipeline can see that the element type is not real and run a convert on it.

EDIT or maybe if eltype is Any you can do

function get_element_type(int::AbstractInterval{T}) where {T}
    if T  === Any
        return mapreduce(typeof, promote_type, endpoints(int))
    else
         return T
    end 
end

[ptiede]

So if you add the get_element_type method you run into more errors, although they all live int unitful_integration.jl. My guess is that's a good thing! The main problem in this file is that a lot of the dim_convert machinery assumes that the dimensions are iterable, which isn't true for Intervals. To fix this you can add the following definitions

function eltype_extrema(values::ClosedInterval)
    return eltype_extrema(endpoints(values))
end

function unit_convert(unit::T, x::ClosedInterval) where T <: Union{Type{<:Unitful.AbstractQuantity}, Unitful.FreeUnits, Unitful.Unit}
    int = endpoints(x)
    return ClosedInterval(unit_convert.(Ref(unit), int)...)
end

_check_convert(unit, values) = unit_convert(unit, values)
_check_convert(unit, value::ClosedInterval) = unit_convert(unit, minimum(value))

function convert_dim_observable(conversion::UnitfulConversion, value_obs::Observable, deregister)
    result = map(conversion.unit, value_obs; ignore_equal_values=true) do unit, values
        if !isempty(values)
            # try if conversion works, to through error if not!
            # Is there a function for this to check in Unitful?

            _check_convert(unit, values)
        end
        update_extrema!(conversion, value_obs)
        return unit_convert(conversion.unit[], values)
    end
    append!(deregister, result.inputs)
    return result
end

These changes fix the MWE, but I am not sure if this is correct Makie code.