Improve dim converts recipe compatibility, usability #5323
Conversation
Benchmark ResultsSHA: 7feb429b0d3a374a6310058021379b9441e9d458 Warning These results are subject to substantial noise because GitHub's CI runs on shared machines that are not ideally suited for benchmarking.
|
SimonDanisch
left a comment
SimonDanisch
left a comment
There was a problem hiding this comment.
This looks great! :)
A few things I'd like to change:
argument_dims(::Type{<:Annotation}, x, y) = (1, 2)
argument_dims(::Type{<:Annotation}, xy::VecTypes{2}, xy2::VecTypes{2}) = ((1, 2), (1, 2))
I think the fallback for these should be smart enough so that these don't need to get defined?
The suffix format "[{}]" seems a bit unusal.
I think we should offer:
- FormatExpr("({1})") via Format.jl
- Functional unit-> "[$(unit)]"
Maybe it would also make sense to have one big ticklabel function one can overload?
(unit, suffix, prefix, label) -> ... Not sure if that could entirely replace suffix?
I was also thinking if we should take the chance to move unitful integration into an Extension. I could take a look at that, I just moved a couple of things to extensions recently!
@jkrumbiegel what do you thing? Any comment about the new axis attributes?
|
I now added conversion target types for almost all recipes because I'm worried there will be issues otherwise. If you define a recipe that uses a plot that does not have them set, you will end up with |
| Plots a surface, where `(x, y)` define a grid whose heights are the entries in `z`. | ||
| `x` and `y` may be `Vectors` which define a regular grid, **or** `Matrices` which define an irregular grid. | ||
| """ | ||
| @recipe Surface (x::VecOrMat{<:FloatType}, y::VecOrMat{<:FloatType}, z::VecOrMat{<:FloatType}) begin |
There was a problem hiding this comment.
Does z actually allow vectors? I didn't allow it in the conversion trait...
| if !is_data_space(space) | ||
| # dim converts do not apply in relative, pixel or clip space | ||
| map!(attr, :args, :dim_converted) do args | ||
| return Ref{Any}(args) | ||
| end | ||
| elseif force_dimconverts && needs_dimconvert(dim_converts) | ||
| add_dim_converts!(P, attr, dim_converts, args, args_converted, user_kw) |
There was a problem hiding this comment.
The changes here are:
- relative, pixel and clip space skip dim converts
need_dimconverts()now only triggers whenforce_dimconverts == truefor the plot, which happens by default for plots to a scene but not for plots to another plot. This was necessary to avoid forcing dim converts in recipes, where the parent had already done the conversion. Previously this was decided based on whether the argument was aVector{<:Point}, more or less.Axis3also needed this behavior to be overwritable for axis decorations, so you can passforce_dimconverts = falseto plots like an argument.- the
force_dimconvertandstatus == nothingbranches now apply recursive/early convert_arguments like thefalsebranch did. (This moved toadd_dim_converts)
| # Something we can convert to an EndPoints type | ||
| const EndPointsLike = Union{ClosedInterval, Tuple{Real, Real}} | ||
|
|
||
| const RangeLike = Union{AbstractVector{T}, ClosedInterval{T}, Tuple{T, T}} where {T} |
There was a problem hiding this comment.
This type changed from
Union{AbstractVector, ClosedInterval, Tuple{Real, Real}}
i.e. the eltype is now settable instead of being partially fixed to Real
| get_label_suffix(dc, format, use_short) = apply_format(get_label_suffix(dc, use_short), format) | ||
| get_label_suffix(dc, format) = apply_format(get_label_suffix(dc), format) | ||
| get_label_suffix(dc, ::Bool) = get_label_suffix(dc) | ||
| get_label_suffix(dc) = error("No axis label suffix defined for conversion $dc.") | ||
| get_label_suffix(dc::Union{Nothing, NoDimConversion}) = "" |
There was a problem hiding this comment.
I set this up to error when setting ax.x_unit_in_label = true for a dim convert that doesn't implement get_label_suffix. We could also have it return an empty suffix, but then we would probably want to not automatically apply the format. Because that results in "[]".
And if this does error we should probably remove the Nothing/NoDimConversion case.
| "Controls whether the x dim_convert is shown in ticklabels." | ||
| x_unit_in_ticklabel::Union{Bool, Automatic} = automatic | ||
| "Controls whether the y dim_convert is shown in ticklabels." | ||
| y_unit_in_ticklabel::Union{Bool, Automatic} = automatic | ||
| "Controls whether the x dim_convert is shown in the xlabel." | ||
| x_unit_in_label::Union{Bool, Automatic} = automatic | ||
| "Controls whether the y dim_convert is shown in the ylabel." | ||
| y_unit_in_label::Union{Bool, Automatic} = automatic | ||
| """ | ||
| Formatter for the xlabel suffix generated from dim_converts. Can be a | ||
| Format.jl format string or a callback function acting acting on the | ||
| string or rich text generated from the dim convert. | ||
| Can also be a plain String replacing an active dim_convert label. | ||
| """ | ||
| xlabel_suffix = "[{}]" | ||
| """ | ||
| Formatter for the ylabel suffix generated from dim_converts. Can be a | ||
| Format.jl format string or a callback function acting acting on the | ||
| string or rich text generated from the dim convert. | ||
| Can also be a plain String replacing an active dim_convert label. | ||
| """ | ||
| ylabel_suffix = "[{}]" | ||
| "Switches between short and long x units, e.g. \"s\" vs \"Second\"" | ||
| use_short_x_units::Bool = true | ||
| "Switches between short and long y units, e.g. \"s\" vs \"Second\"" | ||
| use_short_y_units::Bool = true |
There was a problem hiding this comment.
These probably need some renaming to fix with Axis attributes better? I guess just remove all the _?
| Note that the `::Type{<:Plot}` methods take precedence over `::ConversionTrait`. | ||
| """ | ||
| function argument_dims(PT, args...; kwargs...) |
There was a problem hiding this comment.
Do we want to export argument_dims and argument_dim_kwargs?
There was a problem hiding this comment.
Nope... Also, I think it would be nice to get rid of arguments_dim_kwargs!
|
As this is similar to what AoG needs to do, maybe we can compare the two. Does this implementation have something that can respect the direction keywords and similar attributes? Also in AoG I have a DeltaY scale for errorbar for example where the unit needs to match. For example a DateTime axis can't have a DateTime as an error magnitude. |
Didn't do anything like that. This pr just tags args as "should convert with the n-th dim_convert` or no dim convert. |
Description
This pr adds
argument_dims()as an overloadable function similar toconvert_arguments()to allow recipes to define how their arguments map to plot dimensions. With that you can specify which dim_convert applies to first, second, ... argument by returning the index of the appropriate dimension/dim convert.This pr fixes:
hist(dates)MethodError #3889, fixeshistshould work on categorical inputs #4409meshscatterlinesdoes not work with Point2 with DateTime #4774, fixes Sometimes units are not handled #3945base_unit(::Float64)#4085, fixes plot() returns error when called on data of Vector{Quantity{Int}} due to arguments passed to Unitful.Quantity() #5231. Well sort of, the input data is aVector{Quantity{Float64}}which now successfully gets converted as a unitless unit. Generates and empty label suffix though[]scatter!fails forVector{Union{missing, Float64}}#5089, maybe because earlyconvert_arguments()run more consistently?Other affected:
types_for_plot_argumentstrick/interfaceThis pr also aims to make some usability improvements:
Changes:
1e-6u"m"does not convert to1u"µm"anymore)units_in_labelfromUnitfulConversionas it's now controlled by Axisforce_dimconvertsas a plot kwarg. This is automatically set to true when plotting to a scene and false when plotting to another plot. When the kwarg is true the plot must conform to existing dim_converts, otherwise unitless are also allowed. (Needed for various axis decoration plots in Axis3)TODO:
PolarAxis, LSceneto include new dim_convert attributes and axis label suffixesspacewhen setting up conversions (only data should be allowed to use dim converts)Type of change
The pr has some breaking changes:
UnitfulConversionandDQConversionno longer haveunits_in_labelUnitfulConversionno longer rescales unitsget_ticks()signature for dim converts now also includesshow_in_labelas an argument, though it defaults to the method without it)Checklist