Boxplot widths

[grantmcdermott]

Closes #195.

[grantmcdermott]

@zeileis I think that this should address the various examples that we discussed in #195. Let me know if you have time to do a quick review / kick the tyres. Thanks!

[zeileis]

Thanks, will have a look tonight!

[zeileis]

The boxplots look great, thanks Grant @grantmcdermott ! I played around with various combinations of by and facet and boxplot-specific arguments. Everything worked as intended except one extremely contrived example (see below).

I mainly wondered about the computation of x_by. First, why is it set to FALSE except for type == "boxplot" in

https://github.com/grantmcdermott/tinyplot/blob/boxplot_widths/R/tinyplot.R#L654-L658

Second, it is done rather early now but x and by might have been modified already in tinyplot.default.

As for the first point: I don't understand the logic why this should be limited to boxplots. The logic should also apply to other plot types, even if these choose to ignore the x_by argument.

And an example, where the x_by gets FALSE even though it should be TRUE is if both variables are numeric. The reason is that by gets coerced to factor before the comparison. Due to this the following plots are not the same, although they should:

tinyplot(mpg ~ factor(gear) | factor(gear), facet = "by", data = mtcars, type = "boxplot")
tinyplot(mpg ~ gear | gear, facet = "by", data = mtcars, type = "boxplot")

[grantmcdermott]

Ah, good catch!

Let me take your points in reverse order:

Due to this the following plots are not the same, although they should:

Thanks. I bumped this x_by assignment further up in the code (per your helpful suggestion) to avoid any earlier coercion "mismatches". So the examples that you gave should now be working correctly. I included a test along these lines.

I mainly wondered about the computation of x_by. First, why is it set to FALSE except for type == "boxplot" in

The x_by flag only matters (i.e., is only used) for boxplots. So I was trying to avoid doing unnecessary evaluations for other plot types, which might slow down the overall tinyplot() function. I admit that this probably doesn't make a material difference, since identical is normally fast. But I do like the principle of only doing work you need and would really like to preserve the overall snappiness of tinyplot. (I don't want to be dogmatic about this though, and could be convinced to evaluate for other plot types if you think it best.)

[zeileis]

Thanks for the update! Regarding the second point: I agree that avoiding unnecessary computations is a good idea in general - but so is modularity. The more we have these scattered special cases in tinyplot.default the more difficult it becomes to make the plot types self-contained.

Another option (I haven't tried it) could be to compare something like identical(as.integer(x), as.integer(by)) or something along those line within the boxplot code (rather than doing it before).

[grantmcdermott]

I agree that avoiding unnecessary computations is a good idea in general - but so is modularity. The more we have these scattered special cases in tinyplot.default the more difficult it becomes to make the plot types self-contained.

Okay, done. Feel free to squash+merge if you are happy.

[zeileis]

Great, thanks, Grant! Should I also delete the branch after merging it?

[grantmcdermott]

Thanks! (Free free to delete the branch in the future. I'll do it here.)