Units from 3rd axis applied to 4th plot

dmoored4 commented 3 years ago

I have a surface that is drawn in 3D space (length x, y, and z) with a 4th axis, the colorbar, which could be unitless, time, pressure, or some other dimension. The issue is that the colorbar title keeps the units from the 3rd axis (m). If I try to color based on anything other than a unitless number or a length, I get an error. If I could rename the colorbar_title = "Real Title ($real_units)" I would just strip out those units myself, but the other unit stays in so the title would be: "Real Title (kPa) (m)".

begin
    X = [1u"m", 1u"m"; 1u"m", 1u"m"]
    Y = [1u"m", 1u"m"; 1u"m", 1u"m"]
    Z = [1u"m", 1u"m"; 1u"m", 1u"m"]
    C = rand(2,  2)
    surface!(X, Y, Z, surfacecolor=C, color=:turbo, colorbar=true, colorbar_title="this should change (kPa)")
end

I considered rolling my own for this, but I have other elements like the ball being brought into the plot using UnitfulRecipes so I would either have to enforce that everything comes in using the correct units or handle it elegantly or not use UnitfulRecipes for any of it and roll my own for the whole thing.

I've only just started making recipes of my own so when I open the code base it's a little beyond me. I think two potential solutions would be:

Allow a user to turn off the automatic units application to a specific axis
Have the colorbar pick up the units of the series its plotting

Please let me know if I've missed an easy solution or I'd be happy to contribute a solution with a little direction. Thanks!

gustaphe commented 3 years ago

Thank you for the report!

I'm having some problems replicating. For one, there's some commae that need to be removed from your MWE, and some library crashes when I try to do a 3d plot here, I probably need to do some installations.

marker_z works fine, line_z doesn't add a unit label at all (?) and for previously mentioned reason I can't test fill_z (which is the real name of your surfacecolor). There seems to be a special case in place for marker_z, maybe we can replicate it for the others? If you want to give it a go it'd be appreciated.

dmoored4 commented 3 years ago

Hi, thanks for having a look and the tip about the fill_z. Now that I'm more alert, let me provide a better MWE.

begin
    using Plots, Unitful, UnitfulRecipes
    plotly()
end

begin
    N = 25
    X = range(1, 10, length=N) .* u"m"
    Y = range(1, 10, length=N) .* u"m"
    Z = rand(N, N) .* u"m"
    surf_color_nounits = rand(N, N)
    surf_color_units = rand(N, N) .* u"kPa"
end

Contouring on the Z value, applying dimension and unit to colorbar

surface(X, Y, Z, color=:turbo)

Contouring on the desired color values, but they are unitless while the color bar still has `(m)` applied

surface(X, Y, Z, fill_z=surf_color_nounits, color=:turbo)

Failing when I try to contour on a matrix with units applied.

surface(X, Y, Z, fill_z=surf_color_units, color=:turbo)

MethodError: no method matching min(::Float64, ::Unitful.Quantity{Float64, 𝐌 𝐋^-1 𝐓^-2, Unitful.FreeUnits{(kPa,), 𝐌 𝐋^-1 𝐓^-2, nothing}})

You may have intended to import Base.min

Closest candidates are:

min(::Any, ::Any, !Matched::Any, !Matched::Any...) at C:\Users\Daniel-L.Moore\.julia\packages\NaNMath\L8ebG\src\NaNMath.jl:351

min(::T, !Matched::T) where T<:AbstractFloat at C:\Users\Daniel-L.Moore\.julia\packages\NaNMath\L8ebG\src\NaNMath.jl:302

min(::Real) at C:\Users\Daniel-L.Moore\.julia\packages\NaNMath\L8ebG\src\NaNMath.jl:346

...

_update_clims(::Float64, ::Float64, ::Unitful.Quantity{Float64, 𝐌 𝐋^-1 𝐓^-2, Unitful.FreeUnits{(kPa,), 𝐌 𝐋^-1 𝐓^-2, nothing}}, ::Unitful.Quantity{Float64, 𝐌 𝐋^-1 𝐓^-2, Unitful.FreeUnits{(kPa,), 𝐌 𝐋^-1 𝐓^-2, nothing}})@colorbars.jl:59
update_clims(::Float64, ::Float64, ::Matrix{Unitful.Quantity{Float64, 𝐌 𝐋^-1 𝐓^-2, Unitful.FreeUnits{(kPa,), 𝐌 𝐋^-1 𝐓^-2, nothing}}}, ::typeof(Plots.ignorenan_extrema))@colorbars.jl:55
update_clims(::Float64, ::Float64, ::RecipesPipeline.Surface{Matrix{Unitful.Quantity{Float64, 𝐌 𝐋^-1 𝐓^-2, Unitful.FreeUnits{(kPa,), 𝐌 𝐋^-1 𝐓^-2, nothing}}}}, ::Function)@colorbars.jl:53
update_clims(::Plots.Series, ::Function)@colorbars.jl:48
update_clims(::Plots.Subplot{Plots.PlotlyBackend}, ::Function)@colorbars.jl:20
update_clims(::Plots.Subplot{Plots.PlotlyBackend})@colorbars.jl:17
_update_subplot_colorbars@colorbars.jl:104[inlined]
_add_the_series(::Plots.Plot{Plots.PlotlyBackend}, ::Plots.Subplot{Plots.PlotlyBackend}, ::RecipesPipeline.DefaultsDict)@pipeline.jl:409
add_series!(::Plots.Plot{Plots.PlotlyBackend}, ::RecipesPipeline.DefaultsDict)@pipeline.jl:320
_process_seriesrecipe(::Any, ::Any)@series_recipe.jl:46
_process_seriesrecipes!(::Any, ::Any)@series_recipe.jl:27
recipe_pipeline!(::Any, ::Any, ::Any)@RecipesPipeline.jl:97
_plot!(::Plots.Plot, ::Any, ::Any)@plot.jl:208
#plot#157@plot.jl:91[inlined]
var"#surface#446"(::Any, ::typeof(Plots.surface), ::Any, ::Vararg{Any, N} where N)@RecipesBase.jl:404
top-level scope@Local: 1[inlined]

I couldn't get any other aliases to fill the series to work until the surfacecolor. As you pointed out fill_z did also work, but the others did not.

So I gather that the fill_z can take something unitless or something of Unitful.Length only, but nothing else. Maybe the default in the recipe takes the Z values and applies the units to other attributes (like fill_z) before other kwargs are processed? So maybe it should take the given Z as a default kwarg, but still allow a user to override? Not sure if this is the issue or if this would be the fix. If you think that sounds right I'll start working on that.

gustaphe commented 3 years ago

If I was to fix this, I would start with these lines:

https://github.com/jw3126/UnitfulRecipes.jl/blob/d98de3d0a853a790364c02ca606e6a6a8c28a640/src/UnitfulRecipes.jl#L128-L135

fixmarkercolor! does the right thing:

Finds what unit marker_z is, and turns marker_z into a unitless number (for Plots to be able to do its thing)
In the same way goes through clim and removes the unit (now graciously failing if the units don't match up)
Adds a unit label to the c guide (colorbar_title, because sometimes nothing is called what it is)

I think (without having thought too hard about it) fixlinecolor! could easily be adapted to do the same thing. If you copy the code for fixmarkercolor! and just make the obvious edits, that should kind of work (would need to check if marker_z=rand(5)u"m", line_z=rand(5)u"s" fails as it should, but otherwise that's simple).

And if that works, adding a fixfillcolor! function with the same functionality should be a piece of cake. You just need to figure out where to call it (probably just the same place the other two fix...color!s are called.

gustaphe commented 3 years ago

Now if fixlinecolor! doesn't make the unit mismatch fail correctly, it's probably the case that we need to go through and add c unit as an axis unit the same way the others are (which I don't quite recall right now, it's stored in the axis guide?). That's probably a bit more involved, let me know if you don't think you are up to it. But feel free to try!

gustaphe commented 3 years ago

Sorry, I had a think about it and realized it would be more complicated than I previously thought. #70 is the start of an implementation of this.

dmoored4 commented 3 years ago

Thanks for the update and the work trying to solve it. It looks like this pretty well solves it? Lmk if there's anything you want me to progress

gustaphe commented 3 years ago

It solves the individual cases of specifying a fill_z or a line_z or a marker_z? The things that don't work worry me a bit, because they reveal that I don't know what the recipe is really doing. If you have the time, it could be really helpful if you could play around with it and see if you can write down what parts don't work - especially if there is some problem that intersects with your specific use case.

dmoored4 commented 3 years ago

Ok I'll add this to my TDL. Hopefully I'll have an update one way or another towards the end of the week.

jw3126 / UnitfulRecipes.jl