Add unit axis attribute (for v2)

[Ickaser]

Description

In an attempt to fix #4750 and #4822 , following the suggestion in #4822 (comment) to move units from the label (so a string) to an actual axis attribute.

This currently generates correct plots and solves the above issues, but the axis label behavior is probably still a little wonky--e.g., units may get added to an axis label after the user sets the label not to have them. That would be an important part of making this work properly, I think, so I'll mark this "draft" for now, but I would welcome suggestions about how that should behave: for example, should unitformat always be used, and users have to set unitformat to :nothing if they don't want the units printed? Or perhaps respect the ProtectedString API? In the past I found the ProtectedString to be a little annoying but haven't thought yet about how it will interact.

The current code is almost certainly not as elegant as it should be, and once I have an idea of desired label behavior there will probably be some pieces leftover that can be deleted.

MWE for testing

using Unitful, UnitfulLatexify # UnitfulLatexify currently needed to trigger extension, see #5093 
using Plots

x = range(1u"s", 10u"s", length = 100)
y = @. x * 5u"m/s" 

plot(u"hr", u"m") 
plot!(x, y)
plot!(xlabel="time")
plot!(x, -y)
pl2 = twinx()
plot!(pl2, x, 1 ./y)
plot!(pl2, ylabel="1/distance")

Attribution

• I am listed in .zenodo.json (see How to get acknowledged #3503)
  (assuming UnitfulExt: Check if PGFPlotsX is available, then check if it's the current backend #5094 merges)

Things to consider

• Does it work on log scales?
• Does it work in layouts?
• Does it work in recipes?
• Does it work with multiple series in one call?
• PR includes or updates tests?
• PR includes or updates documentation?

[Ickaser]

@gustaphe should probably weigh in on this, I suspect

[gustaphe]

I am generally in favor, have not checked the details yet. I can't even run the code, something about key :pgfplotsx not found on line 240 of UnitfulExt, but that may be down to some misconfiguration on this end.
I would personally not mind killing off the different UnitfulRecipes string types, but that would be technically breaking behavior.

The elegant way to do this would be to have axis properties unit, guide and unitformat which are stored separately and spit out a string at the point of display. No need for a ProtectedString if you can just set your unitformatter to (a,b) -> string(a) or something. The somewhat less elegant way is to have unit, guide_string and unitformat, and then updating guide every time one of them changes. I guess the current status is this second option. Workable.

I have a resting project of baking UnitfulLatexify into Unitful, which would solve the loading bug. Just need a bit of vacation to work on it.

[Ickaser]

I am generally in favor, have not checked the details yet. I can't even run the code, something about key :pgfplotsx not found on line 240 of UnitfulExt, but that may be down to some misconfiguration on this end.

That's a two-line fix in #5094 😅, I left that as a separate PR because it seemed like it would need less discussion.

The elegant way to do this would be to have axis properties unit, guide and unitformat which are stored separately and spit out a string at the point of display. No need for a ProtectedString if you can just set your unitformatter to (a,b) -> string(a) or something. The somewhat less elegant way is to have unit, guide_string and unitformat, and then updating guide every time one of them changes. I guess the current status is this second option. Workable.

The current current depends whether a plot call happened last, so I need to change at least a bit. Possibly I could split into two PRs, one with a less-elegant fix for 1.x and a better one for 2.0, depending how easy it is for me to figure this out.

[Ickaser]

With the most recent commit, this implements the more "elegant" solution of waiting until render time to generate the guide, at which point unit and unit format are both known. If no units are supplied, then it ignores unitformat altogether and gives back the default axis[:guide].

If this looks too invasive, I can walk it back and look at other solutions.

Without something like this, though, it's hard to update the axis[:guide] every time it should change--calls to the Unitful recipe will behave differently than calls like plot!(xlabel="time"). 1908c22 shows what that might look like, except that it didn't respect unitformat because the keyword args may be processed in any order, so unitformat was (in my testing) getting set after guide, and so we would have to go in again and update guide after the fact. I guess it would mean adding another check like

Plots.jl/PlotsBase/src/Axes.jl

Lines 324 to 334 in 4d289ba

	for (k, v) ∈ kw
	haskey(plotattributes, k) || continue
	if k ≡ :discrete_values
	foreach(x -> discrete_value!(axis, x), v) # add these discrete values to the axis
	elseif k ≡ :lims && isa(v, NTuple{2,Dates.TimeType})
	plotattributes[k] = (Dates.value(v[1]), Dates.value(v[2]))
	else
	plotattributes[k] = v
	end
	end

if haskey(kw, :guide) || haskey(kw, :unit) || haskey(kw, :unitformat)
    plotattributes[:guide] = ...
end

but that felt more awkward to me, especially since there is already a function get_lims for the axes so get_guide matches that pattern.

[Ickaser]

I believe all the tests are passing now. To make that so, I had to do a couple things that may merit discussion.

• Move ProtectedString into PlotsBase, rather than the extension. 2.0 might be the most natural place to drop this API, so if desired, that could be implemented now or moved to a second PR so this can be backported to 1.x.
• If "" is passed as a guide label, old API does not add units. Seems a reasonable expectation, so if "" is passed to a guide, set axis[:unitformat]=:none as a side effect, achieving the desired result. This could potentially be annoying if someone passes a guide string later on and expects the unit to come back. An alternative might be to wrap the empty label in a ProtectedString, if we keep that API.
• The PGFPlotsX unit handling requires Latexify, so I refer to the import in the PGFPlotsX extension.

[gustaphe]

Failing CI notwithstanding, I like this a lot. I think it would be good to get the opinion of someone else (github suggests @t-bltg), because I have a quite Unitful focused view and this might in theory make handling axis guides slightly more cumbersome for someone who does not.

• If we intend to remove ProtectedString in 2.0, you can put Base.depwarn in the constructor. I think it's a good idea.
• I never liked this behavior, but inertia is a good thing (My preferred way to say "no label at all" is guide=nothing, which iiuc does the right thing even without changing unitformat). This way of doing it is acceptable (for now).
• Yeah, that makes sense.

[t-bltg]

Please fix formatting using https://docs.juliaplots.org/stable/contributing/#Write-code,-and-format.

I also think it's a good idea to implement get_guide.
I don't have time to review this thoroughly though.
It would be great to check manually with a scenario, that all backends behave the same and don't regress with units.