Merged
Conversation
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Smooth and violin layers now trim their rendering to the actual data range instead of extrapolating beyond it. Grid expansion uses 3×bandwidth (kernel-aware) instead of a fixed 10% extension. Key optimizations: - Consolidated min/max computation into bandwidth CTE (eliminates redundant query) - Unified grid construction logic in build_grid_cte() with per-group trimming - Removed compute_range_sql() and unused execute parameter throughout call chain - Extracted shared SQL generation logic to reduce duplication All density/smooth/violin tests pass (269 tests verified). Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
The violin renderer was filtering out low-density points (< 0.001) to trim thin tails, but this is now handled properly upstream with grid trimming in the stat transform. The stat_violin function trims the grid to data range + 3×bandwidth, making this downstream filter redundant. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Resolved merge conflicts in density.rs by combining: - Grid trimming with dynamic range computation (917fde8) - Dialect support with SqlDialect parameter (88baccd) Key changes: - density_sql_bandwidth: Combined dynamic MIN/MAX range with dialect-aware silverman_rule - build_grid_cte: Added dialect support while keeping grid trimming - choose_kde_kernel: Changed ANY_VALUE to MIN for portability, kept Nadaraya-Watson support - Fixed smooth.rs and violin.rs to include dialect parameter All tests pass: density (14), violin (8), smooth (4) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
thomasp85
approved these changes
Mar 18, 2026
Collaborator
thomasp85
left a comment
thomasp85
left a comment
There was a problem hiding this comment.
Not many comments - the queries looks as crazy as always :-)
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 adds working smooth layer.
It comes in 3 variants, OLS, TLS and Kernel regression. The first 2 are straight line fits, the 3rd is a locally weighted average.
In addition to the layer, this PR also adds trimming logic to the core density code.
A think I wish I could've handled was using these layers for date position scales like timeseries. However, I've not found palatable solutions to switch between date(time) to numeric back to date(time) again.