Merged
Conversation
Restructure GeomAesthetics to prepare for centralized default aesthetic values. Each geom now defines all aesthetic metadata (required, optional, delayed) in a single defaults field, enabling future control over visual appearance defaults. Key changes: - Rename GeomAesthetics to DefaultAesthetics with unified defaults field - Add Required, Null, Delayed variants to DefaultAestheticValue enum - Derive supported/required lists via helper methods instead of separate fields - Update all 22 geom implementations with new structure - Fix semantic bugs where MAPPING validation incorrectly included Delayed aesthetics Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Implement Step 4 of the plan: writer now applies default aesthetic values from each geom's DefaultAesthetics when building encodings. Defaults are applied with lowest priority (after MAPPING and SETTING). Key changes: - Add logic in build_layer_encoding() to iterate over geom defaults - Skip if encoding already set by MAPPING or SETTING - Convert DefaultAestheticValue to AestheticValue via to_aesthetic_value() - Reuse existing build_encoding_channel() for unit conversions - Skip Required, Null, and Delayed variants (no literal defaults to apply) Precedence order: MAPPING > SETTING > defaults (geom) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Remove redundant hardcoded defaults from BoxplotRenderer and PolygonRenderer.
These defaults are now handled by the centralized default aesthetics system
in build_layer_encoding().
Changes:
- BoxplotRenderer: removed default_stroke, default_fill, default_linewidth
- PolygonRenderer: removed hardcoded fill and stroke ("#888888")
Encoding-level properties (from geom defaults) override mark-level properties,
so the hardcoded values were redundant. All defaults now come from each geom's
DefaultAesthetics definition.
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add unit and integration tests to verify default aesthetic behavior: Unit tests (types.rs): - test_default_aesthetics_methods: Comprehensive test of all helper methods (get, names, supported, required, is_supported, contains, is_required) on a DefaultAesthetics instance with various value types Integration tests (vegalite/mod.rs): - test_default_aesthetics_applied: Verify defaults appear in Vega-Lite output (stroke="black", opacity=1.0) - test_setting_overrides_default: Verify SETTING takes precedence over defaults - test_mapping_overrides_default: Verify MAPPING takes precedence over defaults - test_null_defaults_not_applied: Verify Null variants don't create encodings Also adds get() helper method to DefaultAesthetics for cleaner test code. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Use existing linetype_to_stroke_dash() function to convert linetype strings to Vega-Lite strokeDash arrays in two places: - SETTING parameters (e.g., SETTING linetype => 'dashed') - Default aesthetic values (e.g., line geom's default "solid") Named patterns like "solid", "dashed", "dotted" are converted to numeric arrays ([6,4] for dashed), while unrecognized patterns pass through as strings. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
teunbrand
commented
Feb 19, 2026
| // If x is missing: single bar showing total | ||
| // If y is missing: stat computes COUNT or SUM(weight) | ||
| // weight: optional, if mapped uses SUM(weight) instead of COUNT(*) | ||
| supported: &["x", "y", "weight", "fill", "stroke", "width", "opacity"], |
Collaborator
Author
There was a problem hiding this comment.
Note: width was removed. It is already a parameter and width cannot be a vegalite encoding (vegalite uses size when mapped to data).
Unify SETTING parameter and default aesthetic processing into a single loop to eliminate code duplication and clarify precedence order. Changes: - Merge separate SETTING and defaults loops into unified loop - Use build_encoding_channel() for both (eliminates conversion duplication) - Make precedence explicit: SETTING > defaults (both checked per aesthetic) - Remove linetype_to_stroke_dash import (now only used in encoding.rs) Benefits: - Single source of truth for SETTING/defaults precedence logic - All conversions (size, linewidth, linetype) go through same code path - Reduced code: ~40 lines → ~28 lines - Prepares for future writers (ggplot2, plotters) to reuse precedence logic Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add Layer::get_aesthetic_value() to centralize SETTING > defaults precedence logic, preparing for multiple writers (ggplot2, plotters). Changes: - Add DefaultAestheticValue::to_parameter_value() to extract literal values (returns ParameterValue, not Option - uses Null for non-literals) - Refactor to_aesthetic_value() to reuse to_parameter_value() (eliminates duplication) - Add Layer::get_aesthetic_value() for SETTING > defaults resolution - Update Vega-Lite writer to use get_aesthetic_value() (28 lines → 18 lines) Benefits: - Precedence logic centralized in Layer (not duplicated per writer) - Future writers can reuse get_aesthetic_value() for consistent behavior - Conversions (size, linewidth, linetype) remain writer-specific - Cleaner, more maintainable code with simpler signatures Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
thomasp85
reviewed
Feb 24, 2026
Collaborator
thomasp85
left a comment
thomasp85
left a comment
There was a problem hiding this comment.
A few comments and suggestions but I really like the approach taken here
Aesthetic harmonization (MAPPING > SETTING > defaults) now happens once during execution rather than per-writer. This creates a single source of truth for aesthetic values. Changes: - Add resolved_aesthetics field to Layer (HashMap of resolved values) - Add Layer::resolve_aesthetics() method called during execution - Remove Layer::get_aesthetic_value() (logic now inlined) - Simplify Vega-Lite writer to use resolved_aesthetics directly - Add comprehensive tests for resolution logic This ensures all writers handle aesthetics consistently and reduces duplication across writer implementations. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Consolidate aesthetic resolution by removing the separate resolved_aesthetics
field and inserting SETTING/defaults directly into mappings as Literal values.
The key insight is timing: resolve_aesthetics() now runs AFTER query literals
have been converted to columns, ensuring proper distinction.
Changes:
- Remove resolved_aesthetics field from Layer struct
- Update resolve_aesthetics() to insert into mappings as AestheticValue::Literal
- Move resolve_aesthetics() call to after remappings (line 733)
- Simplify Vega-Lite writer to single loop over mappings
- Update all tests to check mappings instead of resolved_aesthetics
- Add comprehensive documentation explaining query literal vs SETTING/default distinction
Flow:
1. Query literals ('foo' AS color) → parsed as Literal in mappings
2. build_layer_select_list() → converts to SQL columns
3. update_mappings_for_aesthetic_columns() → Literal → Column in mappings
4. resolve_aesthetics() → adds SETTING/defaults as NEW Literals (key change)
5. Writer → Column gets scales, Literal renders as constant value
This ensures:
- Query literals can have scales applied (they're columns)
- SETTING/defaults remain constant values (they're Literals)
- Single source of truth for all aesthetic values
- Correct precedence: MAPPING > REMAPPING > SETTING > defaults
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Resolved conflicts in src/writer/vegalite/mod.rs: - Kept all imports from both branches - Merged test sections (aesthetic tests + facet tests) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Collaborator
Author
|
Ok resolving the defaults/settings now feeds into the |
thomasp85
approved these changes
Feb 24, 2026
cpsievert
added a commit
that referenced
this pull request
Mar 4, 2026
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR aims to fix #76.
Briefly, it replaces the
GeomAestheticsstruct with aDefaultAestheticsstruct that evokesggplot2::Geom$default_aestheticsenergy and gives it a few methods. The point of this is that we control the defaults, and don't rely on the writer's defaults. The defaults are woven into layers, giving precedence to SETTING and MAPPING clauses.I've not necessarily mirrorred ggplot2's aesthetic default here. I've often opted for a semitransparent fill, because I believe it makes it easier to see if you're (accidentially or on purpose) overplotting things. Most outlines have a linewidth of 1, whereas line/path has a linewidth of 1.5, which I justify because the line is the layer, rather than a component of the layer.
Setting this as draft for some polishing.