Skip to content

Support for Unitful.Gain in axis recipes? #4801

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.

Sign up for GitHub

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

Jump to bottom
Open
icweaver opened this issue Feb 17, 2025 · 4 comments · May be fixed by #4853
Open

Support for Unitful.Gain in axis recipes? #4801

icweaver opened this issue Feb 17, 2025 · 4 comments · May be fixed by #4853
Labels
enhancement Feature requests and enhancements Makie Backend independent issues (Makie core) units aka dim converts

Comments

@icweaver
Copy link

Feature description

Would it be possible to add Unitful.Gain to the list of supported units in unitful-integrations.jl?

We're investigating transitioning some of our JuliaAstro plotting infra over to Makie.jl, but are currently hitting this issue when trying to work with units that are not Quantities, which comes up when dealing with magnitudes in UnitfulAstro or logarithmic units in base Unitful for example:

julia> using GLMakie, Unitful

julia> scatter((1:10)u"dB")
ERROR: ArgumentError:     Conversion failed for Scatter (With conversion trait PointBased()) with args: Tuple{Vector{Gain{Unitful.LogInfo{:Decibel, 10, 10}, :?, Int64}}} .
    Scatter requires to convert to argument types Tuple{AbstractVector{<:Union{Point2, Point3}}}, which convert_arguments didn't succeed in.
    To fix this overload convert_arguments(P, args...) for Scatter or PointBased() and return an object of type Tuple{AbstractVector{<:Union{Point2, Point3}}}.`

Stacktrace:
 [1] conversion_pipeline(P::Type{…}, used_attrs::Tuple{}, args::Tuple{…}, kw_obs::Observable{…}, args_obs::Tuple{…}, user_attributes::Dict{…}, deregister::Vector{…}, recursion::Int64)
   @ Makie ~/projects/Makie.jl/src/interfaces.jl:241
 [2] conversion_pipeline(P::Type{…}, used_attrs::Tuple{}, args::Tuple{…}, kw_obs::Observable{…}, args_obs::Tuple{…}, user_attributes::Dict{…}, deregister::Vector{…}, recursion::Int64)
   @ Makie ~/projects/Makie.jl/src/interfaces.jl:233
 [3] conversion_pipeline(P::Type{…}, used_attrs::Tuple{}, args::Tuple{…}, kw_obs::Observable{…}, args_obs::Tuple{…}, user_attributes::Dict{…}, deregister::Vector{…})
   @ Makie ~/projects/Makie.jl/src/interfaces.jl:213
 [4] (Scatter)(user_args::Tuple{Vector{Gain{Unitful.LogInfo{…}, :?, Int64}}}, user_attributes::Dict{Symbol, Any})
   @ Makie ~/projects/Makie.jl/src/interfaces.jl:273
 [5] _create_plot(F::Function, attributes::Dict{Symbol, Any}, args::Vector{Gain{Unitful.LogInfo{…}, :?, Int64}})
   @ Makie ~/projects/Makie.jl/src/figureplotting.jl:316
 [6] #scatter#54
   @ ~/.julia/packages/MakieCore/sHgwT/src/recipes.jl:510 [inlined]
 [7] scatter(args::Vector{Gain{Unitful.LogInfo{:Decibel, 10, 10}, :?, Int64}})
   @ MakieCore ~/.julia/packages/MakieCore/sHgwT/src/recipes.jl:508
 [8] top-level scope
   @ REPL[6]:1
Some type information was truncated. Use `show(err)` to see complete types.

At any rate, #3226 has been a game changer for working with Unitful.jl, thank you for adding this support!

For plot types, please add an image of how it should look like

julia> using GLMakie, Unitful

julia> scatter((1:10)u"m")
# but replace "m" with "dB" or "mag"

Image
@icweaver icweaver added the enhancement Feature requests and enhancements label Feb 17, 2025
@icweaver icweaver mentioned this issue Feb 17, 2025
Open
12 tasks
@ffreyer ffreyer added units aka dim converts Makie Backend independent issues (Makie core) labels Feb 26, 2025
@asinghvi17
Copy link
Member

asinghvi17 commented Mar 5, 2025 •
edited
Loading

It seems like that would be pretty simple to add along with some function overloads on our end. I'm not a Unitful expert, but if you'd like to have a go at this I'd be happy to support from our end.

@ffreyer
Copy link
Collaborator

ffreyer commented Mar 5, 2025

Maybe just needs to be added to

Makie.jl/src/dim-converts/unitful-integration.jl

Line 5 in 2fe1ac4

const SupportedUnits = Union{Period,Unitful.Quantity,Unitful.Units}
?

@icweaver
Copy link
Author

icweaver commented Mar 5, 2025 •
edited
Loading

Thanks all for taking a look and for the quick feedback!

oof, I was hoping that we could just update SupportedUnits too, and maybe add Gain/LogScale specific methods for base_unit, unit_string, and friends, but it looks like there are a fair number of other places in unitful-integration.jl that I think might need some special treatment for this type.

For example, it seems that I am running into PainterQubits/Unitful.jl#463 when best_unit tries to find the average of two decibel values:

Makie.jl/src/dim-converts/unitful-integration.jl

Lines 93 to 94 in 2fe1ac4

function best_unit(min, max)
middle = (min + max) / 2.0

i.e., it seems that things like

julia> using Unitful

julia> (1u"dB" + 5u"dB") / 2.0
ERROR: no automatic promotion of Gain{Unitful.LogInfo{:Decibel, 10, 10}, :?, Int64} and Float64.

are not currently supported in Unitful (although I guess this part could be worked around by just multiplying by 0.5 instead?) nvm, decided to just totally sidestep this in the PR

Additionally, it seems there are places where Makie tries to convert things to Quantities:

Makie.jl/src/dim-converts/unitful-integration.jl

Lines 158 to 161 in 2fe1ac4

function update_extrema!(conversion::UnitfulConversion, value_obs::Observable)
conversion.automatic_units || return
eltype, extrema = eltype_extrema(value_obs[])
conversion.extrema[value_obs.id] = promote(Quantity.(extrema)...)

which doesn't play nicely with Unitful.LogScaled subtypes.

Is this a blocker for trying to support these kinds of logarithmic objects in Makie?

This was referenced Mar 7, 2025
Open
Open
@icweaver
Copy link
Author

icweaver commented Mar 7, 2025 •
edited
Loading

At any rate, here's a suuuper rough first attempt. Will continue the convo over there if this still seems like a worthwhile pursuit 👍🏾

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement Feature requests and enhancements Makie Backend independent issues (Makie core) units aka dim converts
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add Unitful.jl support for logarithmic units icweaver/Makie.jl
3 participants
@ffreyer @icweaver @asinghvi17