Skip to content
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.

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

Units from 3rd axis applied to 4th plot #68

Open
dmoored4 opened this issue Oct 30, 2021 · 8 comments · May be fixed by #70
Open

Units from 3rd axis applied to 4th plot #68

dmoored4 opened this issue Oct 30, 2021 · 8 comments · May be fixed by #70

Comments

@dmoored4
Copy link

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

image

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:

  1. Allow a user to turn off the automatic units application to a specific axis
  2. 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
Copy link
Collaborator

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
Copy link
Author

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

image

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

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

image

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
Copy link
Collaborator

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

# Markers / lines
function fixmarkercolor!(attr)
u = ustripattribute!(attr, :marker_z)
ustripattribute!(attr, :clims, u)
u == Unitful.NoUnits || append_unit_if_needed!(attr, :colorbar_title, u)
end
fixmarkersize!(attr) = ustripattribute!(attr, :markersize)
fixlinecolor!(attr) = ustripattribute!(attr, :line_z)

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
Copy link
Collaborator

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 gustaphe linked a pull request Nov 1, 2021 that will close this issue
@gustaphe
Copy link
Collaborator

gustaphe commented Nov 1, 2021

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.

@gustaphe gustaphe linked a pull request Nov 1, 2021 that will close this issue
@dmoored4
Copy link
Author

dmoored4 commented Nov 1, 2021

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
Copy link
Collaborator

gustaphe commented Nov 3, 2021

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
Copy link
Author

dmoored4 commented Nov 8, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants