-
Notifications
You must be signed in to change notification settings - Fork 8
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
Continuous / gradient legend #122
Conversation
Looks amazing! I was only able to try the examples above (crazy week), but it looks excellent on my setup. Unrelated, but could |
Ah thanks for the reminder. That's definitely a goal. Can you please file an issue so I remember to implement? |
This looks very cool. I'll play around some more with it on Friday. |
Okay, this is ready to go for full review from my side. I updated the tests and documentation, and also fixed a few corner cases. One more example (gradient for point interiors, but white borders): pkgload::load_all("~/Documents/Projects/tinyplot")
#> ℹ Loading tinyplot
plt(
lat ~ long | depth, quakes,
grid = TRUE,
palette = hcl.colors(palette = "rocket", alpha = 0.7),
pch = 21, col = "white", bg = "by"
) Created on 2024-02-15 with reprex v2.1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @grantmcdermott, I was intrigued by the hash set method for counting unique values so I made a small benchmark (+ fixed a couple of typos), I hope it's useful
Grant @grantmcdermott, thanks again for implementing this really nice feature. The examples you posted all look great. I started playing around with these examples (without looking at the internals, yet). I noticed a bug in handling the the |
The bug: The
However, when set to a scalar color, it appears to be only used for the first level(s):
For discrete variables this works:
However, even for discrete variables it is not possible to set a vector of background colors (e.g., semi-transparent versions of the border color.
Additional idea: Should we support some sort of shortcut for the latter application? Rather than just having the same color via |
Palette/legend handling for numeric by variables: I think that the current implementation is ad hoc and confusing. For example when you happen to draw different subsets of the same type of data:
Even more seriously, the palette may yield an error in one but not the other case:
And finally, the killer argument in my opinion is that we get a completely different handling when the discrete values are not equidistant. The categorical version just ignores the distances completely:
Conceptual considerations: So I thought a bit more about what style of palette and corresponding legend I would expect for
So at the very least we should change the default palette for ordered factors. And then the question remains whether we should have a special handling of If you disagree with me here, then at least this case should be handled like an ordered factor (i.e., with a sequential palette) and not like an unordered factor. Also I would prefer to employ a palette that preserves distances between the values. This would also imply that for numeric If all of this is incorporated then I think I could live with the discrete palette but would still think it's confusing. Also, there is no simple/intuitive argument to change the behavior because an extra call to |
Wow, this is great feedback. Thanks @zeileis! I think that I agree with all of your major points. I won't be able to action anything immediately, since I'm heading out for a weekend at the coast. But I'll mull on some of your high level design decision ideas while I'm looking at the waves rolling in :-) |
@zeileis @vincentarelbundock Okay... I believe that all of the outstanding issues should now be addressed. I ended up going with the adjusted viridis palette as the default after all, and any other changes should be in line with your suggestions. (See the updated examples right the top of the thread for some illustrations.) Please kick the tires once more to check that you're happy. Assuming that everything looks good to you, please feel free to squash and merge. |
Thanks, Grant, for the thorough update! I played around with it and noticed that my recommendation of reversing the scale has led to some inconsistencies. Sorry about that! First, I noticed that the case of 100 colors is handled differently from other settings:
I guess that this might be due to reversing the order in two different places in the code? Also, the default ordering is different for ordered factors vs. numeric variables, e.g.,
Maybe we also want to use the same restricted viridis palette for the ordered factors? P.S.: Given that you were already kind enough to list me with an "aut" role for the package, you never need to thank me in the NEWS. :-) |
Quick clarification/confirmation on this: We can certainly match the restricted colors for ordered factors and ensure that low values = dark. But do we want the legend to be reversed and run from bottom to top too? I understand that it will be better for internal consistency, but we are deviating from established norms in other packages. Both ggplot2 and lattice run ordered factors from top to bottom, e.g. |
Good point. So we can either be consistent within |
@zeileis Thanks for confirming (and for catching these cases). Both should be fixed now: pkgload::load_all("~/Documents/Projects/tinyplot")
#> ℹ Loading tinyplot
tinyplot(lat ~ long | depth, data = quakes, pch = 19, col = hcl.colors(100, "ag_Sunset")) tinyplot(mpg ~ wt | ordered(carb), data = mtcars, pch = 19, cex = 1.5) Created on 2024-02-27 with reprex v2.1.0 |
Is there anything we still need to do/check before merging? |
Thanks for the reminder, Grant. I'm just traveling home from a conference and didn't have time, yet, to play with the code. I just noticed one last inconsistency that I wanted to mention. But as I explain below, I think that this is the best solution we can do. So I wouldn't change anything. Compare:
In the numeric case we reverse the order to obtain "dark = high". But in the ordered and factor case we don't reverse the order so that dark = low. While this is somewhat inconsistent, I think this is the best we can do. I just wanted to point out why - so that you can check whether you agree with these considerations or whether you would prefer a different solution.
|
Hmmm. Yes, I think you're right that this is the "least bad" tradeoff that we can make here. And users can always use
Sorry, I don't mean to be a rash, I was mostly checking in, since I realised that my last message was probably a bit ambiguous. I just pushed another small commit now, but that should be it from me unless you pick up any more issues in testing. Catching these edge cases is important, so take your time... although it would be great if we could merge this PR fairly soon, since that will clear the way for the last few things before CRAN submission ;-) I'm hoping to submit before I head out for an extended vacation around spring break. Let me know! |
|
|
Thank you for doing all the actual hard work!! |
Closes #84.
Closes #124.
Closes #130.
Some notes:
I added a threshold for unique number of groups (default = 5) before the continuous legend kicks in. This can be over-ridden by the user (tpar("legend.ugc")
). But it's my way to avoid something I personally find annoying about ggplot2's default behaviour, which automatically converts any numeric grouping variable into a gradient swatch, even if there are only (say) two categories.Quick examples. [UPDATED based on bug catches and feedback in thread below.]
Created on 2024-02-22 with reprex v2.1.0