resolution(<mapped_discrete>) should not always return 1 (ggplot2 3.5.0)

[mjskay]

I noticed a minor bug in ggdist under ggplot2 3.5.0, which is caused by a change in behavior that I believe is a regression: resolution(<mapped_discrete>) now always returns 1.

E.g. in 3.4:

resolution(ggplot2:::new_mapped_discrete(c(0.2,0.4,0.6,0.8)))
## [1] 0.2

And in 3.5:

resolution(ggplot2:::new_mapped_discrete(c(0.2,0.4,0.6,0.8)))
## [1] 1

I believe the old behavior is correct. mapped_discrete is applied after discrete values are scaled into (0,1), and when asking the resolution one wants to know the resolution in plot units, not in original data units. This would also be consistent with what resolution() does on continuous variables, as it would be impossible to return the resolution in original data units when passed a numeric, as that information is not retained.

From my side it's an easy fix to just cast to numeric() before using resolution(), but I am fairly sure I should not have to do that.

[teunbrand]

The change was intentional and was designed to solve #5211. The idea was that when some variable is a <mapped_discrete>, we can infer that it came from a discrete variable where there is a 1-unit difference between one label and the next in position scales. Sometimes, this does not quite hold, and ggplot2 itself also escapes such cases in e.g. #5638. The <mapped_discrete> variables are not exclusively between 0-1 though.

[mjskay]

Hmm, I would argue this mixes concerns: resolution used to be a very straightforward function with a simple contract: you give it a vector and it returns "the smallest non-zero distance between adjacent values" (per the docs --- which are no longer correct). The exception for integer fit within this contract, since you can't have integers closer to each other than 1, but the exception for mapped_discrete does not fit within this contract, since you can have mapped_discrete values closer to each other than 1.

Now its contract is: you give it a vector and it returns the smallest non-zero distance between adjacent values, unless you give it a mapped_discrete, in which case it might give you the smallest non-zero distance between adjacent values depending on if that value happens to be 1.

I think a cleaner solution would be for resolution() to have an easy-to-reason-about contract and for the exception for #5211 to be made in that geom, rather than giving resolution() a more complicated contract and sprinkling exceptions throughout the codebase.

Another solution might be to change the heuristic in resolution() to be: for mapped_discrete(), it returns the minimum of the smallest distance and 1. That would solve the issue with bar charts without breaking other code (I think?), and still fulfill the contract.

[teunbrand]

You raise some useful points and maybe it is better to make an exception for #5211 rather than adjusting resolution(). We'll see what bugs roll in over the next weeks and hopefully crank out a hotfix soon.

[mjskay]

Cool makes sense! Congrats on the great release btw.

FWIW, thinking about this more, a relatively simple contract could be: given d, the smallest difference between adjacent non-equal values in the input x (accounting for floating point precision), resolution(x) returns a value such that 0 < resolution(x) <= d. That would cover returning d for doubles, 1 for integers, and min(d, 1) for mapped_discrete.

[teunbrand]

Simply reverting the solution and making an exception for geom_bar() doesn't quite work out because default widths may also come from the position, and other geoms suffer the same issue, e.g. geom_boxplot(). I think probably the best way forward is to add an argument that discriminates between treating <mapped_discrete> as discrete (e.g. resolution = 1) or continuous (like doubles).

[mjskay]

Makes sense. Though, does returning min(d, 1) for mapped_discrete not solve the problem for those other use cases? Might save you an extra argument and a more complicated set of post conditions on the function.