-
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
Spineplots and spinograms for factor y-variables #233
Conversation
First questions:
|
What functions consume these arguments? With your current code, the variables themselves should already be available in the
What "shape" do these arguments have? One simple option might be to use |
The Regarding the extra arguments: The most prominent case are the As they are neither a scalar, nor of length "n", I did not put them as a column in You could argue that I ought to |
If we think users will want to specify xaxs = yaxs = NULL just before
Here's one idea: In the main Since every |
…ass them on to draw_facet_window() where they are set via par()
Great, Vincent @vincentarelbundock, very useful. I've added the |
And nice idea with the I didn't do extensive tests, yet, but I think that Next I want to polish the faceted displays and then have a stab at handling
|
Very, very cool!
I don't know about margins. Paging @grantmcdermott
I see a |
Very exciting 🚀
Yes. That's the Reading and typing quickly on my phone, so I hope I didn't misunderstand the question. I'll be able to look properly in an hour or so. Edit: Details and default values here. https://grantmcdermott.com/tinyplot/man/tpar.html#additional-graphical-parameters |
Thanks, Grant. Then I see two ways of setting this:
2 is leaner but I guess 1 is cleaner? |
Yeah, I don't see a good reason to keep away too many things from type_draw(). Seems like a general design. |
RE: facet margin adjustments. Another option would be to check for Lines 127 to 152 in 068b431
E.g. In the last bit of the above code chunk, we subtract 0.5 lines from the Summarising: maybe we just try adding the following below line 152?
|
…is(4) in the panels on the right, increase default facet margins
Thanks for the advice, as usual very helpful. I now did the following:
|
Thanks @zeileis, I'll take a look. Would this supplant (duplicate?) the existing logic that we use here for only drawing axes of "outer" facets if the plot frame is turned off? Lines 38 to 39 in 068b431
and https://github.com/grantmcdermott/tinyplot/blob/main/R/facet.R#L250-L265 |
…port, refined axis type and lwd handling
Thanks, Grant, I overlooked that feature. Why is that logic only applied if there is no For spineplots, at the moment, I always repeat axis 1 and 3 but axis 4 is only shown for the last panel in a row. But I'm happy to adapt |
SummaryThe My latest additions are:
Examples
ProblemsLegend symbols: Note that I have to set Colors: Often one would want to select a single set of colors coding the levels of the y-variable like this: Users transitioning from
For now, I have worked around this by giving the
|
Whoa, these look great. I'll try to do a proper review tomorrow. (@vincentarelbundock please feel free to jump in first if you have time.) Really excited to see this long-standing issue nearing a resolution! |
Honestly, I wasn't sure whether we would really get here because there were so many special cases in the old monolithic code 🙈 Also I expected the modularization to be even more complicated. But Vincent's trick of passing a lot of arguments to the workers which can then overwrite them via |
Me neither 😭 |
How did you come across this idea? Was this Grant's input? (I didn't follow the details about the initial discussion of the modularization.) Bonus question: The standard design for |
No, I just started by returning a bunch of arguments in a list and reassigning them. Then, I got lazy and used list2env() as a hack. Only later I realized it was kind of a neat trick. Don't have a great idea now, and I'm not going to be able to concentrate on this for a few days at least since it's holiday here. Sorry! |
That's perfectly fine! I should really be doing other things as well (not vacations unfortunately). So I'll wait for Grant's feedback first and then return to handling the |
- move all data prep to data_spineplot() to leverage standard by+factor logic. - pass spineplot-specific extras (e.g., x(y)axlabels, grayscale flag, etc.) to spineplot_draw() via type_info list. - adjust to_hcl()/seq_palette() to account for potential alpha transparency. - require customization arguments like 'breaks' and 'weights' to be passed through type_spineplot(). - add some examples.
Hi @zeileis, I've finished my review+refactor in 26e5635. (Sorry to push these changes directly to your branch, but on balance I think that this is the simplest way to share my changes. We can always rewind this commit if there's something you don't like.) Again, the "refactor" here was mostly me just moving parts of your existing code around. But FWIW the major differences from before are:
From the (also newly add) examples in the docs: pkgload::load_all("~/Documents/Projects/tinyplot")
#> ℹ Loading tinyplot
# "spineplot" type convenience string
tinyplot(Species ~ Sepal.Width, data = iris, type = "spineplot") # Use `type_spineplot()` to pass extra arguments for customization
tinyplot(Species ~ Sepal.Width, data = iris, type = type_spineplot(breaks = 4)) p = palette.colors(3, "Pastel 1")
tinyplot(Species ~ Sepal.Width, data = iris, type = type_spineplot(breaks = 4, col = p)) # Grouped and faceted spineplots
ttnc = as.data.frame(Titanic)
tinyplot(
Survived ~ Sex, facet = ~ Class, data = ttnc,
type = type_spineplot(weights = ttnc$Freq)
) # For grouped "by" spineplots, it's better visually to facet as well
tinyplot(
Survived ~ Sex | Class, facet = "by", data = ttnc,
type = type_spineplot(weights = ttnc$Freq)
) # Fancier version. Note the smart inheritance of spacing etc.
tinyplot(
Survived ~ Sex | Class, facet = "by", data = ttnc,
type = type_spineplot(weights = ttnc$Freq),
palette = "Dark 2", facet.args = list(nrow = 1), axes = "t"
) # PS. If you really need to drop faceting for grouped spineplots, then at
# least add alpha transparency
tinyplot(
Survived ~ Sex | Class, data = ttnc,
type = type_spineplot(weights = ttnc$Freq),
alpha = 0.3
) Created on 2024-10-28 with reprex v2.1.1 Overall, I think this PR is good shape now and I'm happy to merge if you're satisfied with the changes that I've added on top of. (Edit: Oh, and once we've added some snapshot tests!) |
PS. Forgot to show in the examples, but tinyplot(
Survived ~ Class | Sex, facet = "by", data = ttnc,
type = type_spineplot(weights = ttnc$Freq),
palette = "Dark 2", facet.args = list(ncol = 1), axes = "t",
flip = TRUE
) |
Grant @grantmcdermott, thanks a lot for this, very much appreciated. And certainly ok to make the changes directly in the same branch So far, I just had time to read your summary here and scroll through the examples. I will have to take a closer look at the code later. Just two quick comments:
|
Ah, I see. I think that moving data construction to
Hmmm. Yes, it should be easy enough to revert. But I worry that this will introduce inconsistency into the overall API. For example, following #222 and #232 we have required that histogram breaks are passed through tinyplot(Nile, type = type_histogram(breaks = 30)) # breaks work
tinyplot(Nile, type = 'histogram', breaks = 30) # breaks ignored So supporting |
I think I'll disagree with this. It can be even shorter to use the proper form, because plt(x, y, type = type_spineplot(10))
plt(x, y, type = "spineplot", breaks = 10) Even the full-length version is just 4 characters longer: plt(x, y, type = type_spineplot(breaks = 10)) For the long term sanity of the interface, I think it's very important to be strict, and to stop adding new arguments to the main function. Sometimes it'll be a bit more verbose, but consistency of user interface is a super important feature, IMHO. And with completions with most IDE, this is a non-issue for most (though not all) users. |
I think this is actually quite important. And @grantmcdermott's histogram example points exactly where we'll end up: unending type-specific exceptions. The main motivation for my refactor was to enforce a strict separate of code and documentation for each type. This was to ensure long term viability of the code base, but most importantly, sanity of the interface. If we start making lots of exceptions, I think it defeats the purpose. |
Thanks for the feedback. I understand all the points and agree that consistency is important. I think what bothers me about it in this particular case that I have to move from
to
i.e., losing some consistency with |
Thinking about this some more, I wondered whether it would make sense if
|
Yeah, I agree that these kind of edge case inconsistencies with
This is a reasonable proposal and I'll note that we already do something similar when passing the main plotting arguments here: Lines 208 to 210 in 5090327
At the same time, I can think of some potential downsides, including argument duplication/clashes at the top and type-specific level. One simple example: Regardless of whether we ultimately decide to support the If you agree @zeileis (or, disagree but can live with it!), then I'll wait on your review of my refactoring. We still need to implement your "by" interaction logic. But that should hopefully be the final step. |
Pushed a few extra changes. including tests and two user-level features:
tinyplot(
Species ~ Sepal.Width | Species, data = iris, type = type_spineplot(breaks = 4),
palette = "Pastel 1", legend = FALSE
)
data("SwissLabor", package = "AER")
tinyplot(participation ~ age, data = SwissLabor) ## Factor ~ Numeric tinyplot(participation ~ foreign, data = SwissLabor) ## Factor ~ Factor |
I know that we haven't quite cracked the (And by "good enough" I mean very good; I'm super happy with this spineplot functionality.) We can/should revisit the spineplot Any objections? |
I might release a |
Grant, thanks for these additions, super nice! And I agree that the additions are already useful enough to be worth merging/releasing. Do we need to put in some warning (or comment in the documentation) about which features are not yet supported? |
Good call on the warnings. I'll add and then merge. Thanks both, and especially you @zeileis for this excellent addition! |
Great, thanks! And feel free to put up a new issue about adding proper |
Fixes #2
This PR will eventually support spineplots (factor ~ factor) and spinograms (factor ~ numeric) using the
type_
infrastructure from the epic #222There is already some good progress. For example, you can do:
But as you can see the axis labeling is not great and the
by
handling does not work properly, yet. I will ask various questions, mostly to you Vincent @vincentarelbundock, I'm afraid. But I'm optimistic that we can sort out the details.tbc