Rewrite of Coord/Project syntax and implementation#151
Rewrite of Coord/Project syntax and implementation#151thomasp85 merged 30 commits intoposit-dev:mainfrom
Conversation
teunbrand
left a comment
There was a problem hiding this comment.
I got through about half of this PR before the weekend enticed me to unglue myself from my screen. I'll get around reviewing the rest after the weekend
teunbrand
left a comment
There was a problem hiding this comment.
I think this PR is great, I love that we can now have arbitrary position aesthetics, which would pave the way for all sorts of future coords.
A few points to think about:
- How do we envision the bidirectionality of layers? Currently when we say
PROJECT y, x TO cartesian, we don't automatically swap any direction or orientation parameters. Is that something that layers would have to detect? - Would we need coord munching for lines/polygons (generally any non-axis-aligned stuff) when we use polar coordinates?
- If we some day decide want want a
pos3/zaesthetic, how difficult would that be with the current setup?
I'm also planning to do away with the intercept aesthetics. We can omit these here already or I can adapt my line PR, but we need to pick.
|
Answering your general questions here:
From the layer side of things they should just work from the assumption that there is a primary and a secondary position, then we should have facilities to switch these around before and after stats etc. I envision that the layer declare their scale/mapping requirements for the primary and secondary positional aesthetics and then we can automatically detect if we need to switch them around. And then have an escape hatch parameter when we can't deduce it from the mapping (very much like how ggplot2 does it). So e.g. the histogram can always assume that it is binning along the primary positional aesthetic
For now let's ignore it. I think maybe it should be a writer thing because they might handle it differently
The setup should be count agnostic so pretty easy. Only thing is that we might want layers to declare their minimum and maximum number of positional aesthetics they understand. E.g. points can work with any number, but boxplots only 2 |
teunbrand
left a comment
There was a problem hiding this comment.
This LGTM. A small nitpick but won't hurt anyone if unadressed.
After #142 and #151, metadata.columns returned internal aesthetic names (pos1, pos2) and geom defaults (fill, stroke, opacity, etc.) instead of the original SQL column names from the user's data. Use label_name() which returns the original SQL column name (stored in original_name) for Column entries and None for Literal entries, fixing both issues in one filter_map. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fix #19
This PR represent the last of the major refactoring and syntax updates of the main ggsql clauses. Key points:
COORDtoPROJECTedit:
Ok, some feature creep happened. Coords are now responsible for declaring their positional aesthetics, however many they have. All of this is now handled by translating positional aesthetics into an internal agnostic representation (pos1, pos2, ...) and manage it with a AestheticContext that gets created based on the plot and passed around. The syntax is now
PROJECT aes1, aes2, ... TO coordwith theaes1... part being optional but allows the user to control the naming of positional aestheticsedit2:
This PR bumps into a bunch of vegalite deficiencies, especially surrounding the clip setting which is severely broken in vegalite and cannot be made to work. Also, aspect ratio is not possible in vegalite. Also, polar coordinates don't have axes and background and gridlines (#156)