-
Notifications
You must be signed in to change notification settings - Fork 2
Implement advanced guide functions #88
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
base: main
Are you sure you want to change the base?
Conversation
…ticks, and guide_custom Add three new guide types to the ggplot2 implementation: 1. guide_coloursteps() / guide_colorsteps() - Renders binned color scales as discrete color blocks - Supports even steps vs proportional-to-range sizing - Optional limit labels and reverse direction - Full test coverage in GuideColorStepsTest (14 tests) 2. guide_axis_logticks() - Logarithmic axis with varying tick densities - Long ticks (2.25x) at powers of 10 with labels - Mid ticks (1.5x) at 2x and 5x multiples - Short ticks (0.75x) at intermediate values - Customizable tick length multipliers - Full test coverage in GuideAxisLogTicksTest (11 tests) 3. guide_custom() - User-defined guide rendering with Groovy closures - Closure receives context map (svg, x, y, width, height, theme, scales) - Error handling with visual error placeholders - Fixed renderLegend() to handle charts with only custom guides - Full test coverage in GuideCustomTest (12 tests) Also added factory methods for guide_axis_theta() and guide_axis_stack() (rendering implementations pending). All tests passing. Updated ggTodo.md to reflect completion of section 4.8.1, 4.8.2, and 4.8.5. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
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.
Pull request overview
This pull request implements three advanced guide functions for ggplot2-style visualizations: guide_coloursteps(), guide_axis_logticks(), and guide_custom(). These additions significantly enhance the charting library's flexibility and visual capabilities.
Changes:
- Added
guide_coloursteps()/guide_colorsteps()for binned color scales with discrete blocks instead of smooth gradients - Added
guide_axis_logticks()for logarithmic axes with varying tick densities (long, mid, and short ticks) - Added
guide_custom()for user-defined guide rendering using Groovy closures - Added factory methods for
guide_axis_theta()andguide_axis_stack()(rendering pending) - Fixed
renderLegend()to support charts with only custom guides - Comprehensive test coverage with 37 tests across three test files
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| GuideCustomTest.groovy | Comprehensive test coverage (12 tests) for custom guide functionality including factory methods, closure context, error handling, and integration |
| GuideColorStepsTest.groovy | Complete test suite (14 tests) for stepped color guides with even/proportional sizing, limit labels, and reverse direction |
| GuideAxisLogTicksTest.groovy | Thorough testing (11 tests) of logarithmic tick marks with customizable multipliers across various data ranges |
| GgRenderer.groovy | Implements rendering logic for all three guide types, adds log tick axis rendering methods, and fixes legend rendering for custom-only guides |
| GgPlot.groovy | Adds factory methods for all new guide types with comprehensive documentation |
| ggTodo.md | Updates completion status for sections 4.8.1, 4.8.2, and 4.8.5 |
matrix-charts/src/main/groovy/se/alipsa/matrix/gg/render/GgRenderer.groovy
Outdated
Show resolved
Hide resolved
matrix-charts/src/main/groovy/se/alipsa/matrix/gg/GgPlot.groovy
Outdated
Show resolved
Hide resolved
matrix-charts/src/main/groovy/se/alipsa/matrix/gg/GgPlot.groovy
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Fix #1: Remove misleading comment in custom guide rendering - Simplified comment in renderCustomGuide() to clarify that closure result is intentionally unused - Closures add elements directly to context.svg, return value not needed Fix #2: Implement missing parameters in guide_axis_logticks - Added prescaleBase/prescale.base parameter: treats domain values as log-transformed exponents when set (for pre-transformed data) - Added negativeSmall/negative.small parameter (default: 0.1): filters out ticks with |value| < negativeSmall to prevent clutter near zero - Added expanded parameter (default: true): controls whether to use computedDomain (with padding) or exact scale limits - Applied negativeSmall filtering to all tick types (long, mid, short) - Enhanced documentation with clear parameter descriptions and examples Added tests for new parameters: - testGuideAxisLogTicksWithNegativeSmall() - verifies small value filtering - testGuideAxisLogTicksWithExpanded() - tests expanded vs non-expanded ranges - testGuideAxisLogTicksWithPrescaleBase() - tests pre-transformed data handling All tests passing (14 total for GuideAxisLogTicksTest). Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
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.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 12 comments.
matrix-charts/src/main/groovy/se/alipsa/matrix/gg/render/GgRenderer.groovy
Outdated
Show resolved
Hide resolved
matrix-charts/src/main/groovy/se/alipsa/matrix/gg/render/GgRenderer.groovy
Show resolved
Hide resolved
matrix-charts/src/main/groovy/se/alipsa/matrix/gg/render/GgRenderer.groovy
Outdated
Show resolved
Hide resolved
matrix-charts/src/main/groovy/se/alipsa/matrix/gg/render/GgRenderer.groovy
Outdated
Show resolved
Hide resolved
matrix-charts/src/main/groovy/se/alipsa/matrix/gg/render/GgRenderer.groovy
Outdated
Show resolved
Hide resolved
matrix-charts/src/main/groovy/se/alipsa/matrix/gg/render/GgRenderer.groovy
Show resolved
Hide resolved
matrix-charts/src/main/groovy/se/alipsa/matrix/gg/render/GgRenderer.groovy
Outdated
Show resolved
Hide resolved
matrix-charts/src/main/groovy/se/alipsa/matrix/gg/render/GgRenderer.groovy
Show resolved
Hide resolved
…derer.groovy Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…derer.groovy Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Fix #1: Add null validation to guide_custom factory method - Validate that renderClosure parameter is not null - Throw IllegalArgumentException with descriptive message if null - Prevents potential NullPointerException during rendering - Added test testGuideCustomRequiresClosure() to verify validation Fix #2: Extract complex legend title determination logic - Created determineLegendTitle() helper method - Replaces complex multi-line ternary expression - Improves readability and maintainability - Logic unchanged: tries chart.labels.legendTitle first, then falls back to first scale's name, then null All tests passing (13 tests for GuideCustomTest, full suite successful). Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
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.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
matrix-charts/src/main/groovy/se/alipsa/matrix/gg/render/GgRenderer.groovy
Outdated
Show resolved
Hide resolved
matrix-charts/src/main/groovy/se/alipsa/matrix/gg/render/GgRenderer.groovy
Show resolved
Hide resolved
matrix-charts/src/main/groovy/se/alipsa/matrix/gg/render/GgRenderer.groovy
Outdated
Show resolved
Hide resolved
matrix-charts/src/main/groovy/se/alipsa/matrix/gg/render/GgRenderer.groovy
Show resolved
Hide resolved
Fix #1: Reverse labels along with breaks in guide_colorsteps - When reverse=true, both breaks and labels are now reversed to maintain proper correspondence - Previously only breaks were reversed, causing mismatched labels - Updated all label references to use displayLabels instead of labels Fix #2: Add defensive check for division by zero in guide_colorsteps - Check if totalRange == 0 (all breaks have same value) before division - Fall back to even steps rendering when range is zero - Prevents ArithmeticException from division by zero Fix #3 & #4: Fix prescaleBase logic in guide_axis_logticks - When prescaleBase is set, domain values are in log space (exponents) - Fixed comparison logic to compare exponents against log-space domain instead of actual values against exponents - For long ticks: compare exp vs minVal/maxVal when prescaleBase is set - For mid/short ticks: calculate log of multiplied values and compare in log space when prescaleBase is set - Applied fix to both renderXAxisLogTicks and renderYAxisLogTicks - Ensures correct tick positioning for pre-transformed logarithmic data All tests passing (GuideColorStepsTest: 14, GuideAxisLogTicksTest: 14, full suite: BUILD SUCCESSFUL). Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
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.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
matrix-charts/src/main/groovy/se/alipsa/matrix/gg/render/GgRenderer.groovy
Outdated
Show resolved
Hide resolved
matrix-charts/src/main/groovy/se/alipsa/matrix/gg/render/GgRenderer.groovy
Outdated
Show resolved
Hide resolved
Extract duplicated logic into helper methods: 1. Log ticks calculation (renderXAxisLogTicks/renderYAxisLogTicks): - Created LogTickInfo data class to hold tick position data - Created calculateLogTicks() helper method with all calculation logic - Reduced ~300 lines of duplication to single shared implementation - Both X and Y axis methods now call the same helper 2. Color steps rendering (renderColorStepsLegend): - Created renderEvenColorSteps() helper method - Used by both evenSteps branch and totalRange==0 fallback - Eliminated ~30 lines of duplicated rendering code All tests passing: - GuideAxisLogTicksTest: 14/14 tests passing - GuideColorStepsTest: 14/14 tests passing - GuideCustomTest: 13/13 tests passing Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
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.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
matrix-charts/src/main/groovy/se/alipsa/matrix/gg/render/GgRenderer.groovy
Outdated
Show resolved
Hide resolved
matrix-charts/src/main/groovy/se/alipsa/matrix/gg/render/GgRenderer.groovy
Outdated
Show resolved
Hide resolved
matrix-charts/src/main/groovy/se/alipsa/matrix/gg/render/GgRenderer.groovy
Outdated
Show resolved
Hide resolved
matrix-charts/src/main/groovy/se/alipsa/matrix/gg/render/GgRenderer.groovy
Outdated
Show resolved
Hide resolved
matrix-charts/src/main/groovy/se/alipsa/matrix/gg/render/GgRenderer.groovy
Outdated
Show resolved
Hide resolved
Add defensive checks for empty displayLabels list in renderColorStepsLegend. Instead of directly accessing .first() and .last() on potentially empty lists, check if the list is empty first and fall back to formatNumber() if needed. Changes: - Line 2820: Check !displayLabels.isEmpty() before .first() (vertical min) - Line 2827: Check !displayLabels.isEmpty() before .last() (vertical max) - Line 2836: Check !displayLabels.isEmpty() before .first() (horizontal min) - Line 2843: Check !displayLabels.isEmpty() before .last() (horizontal max) This prevents IndexOutOfBoundsException when displayLabels is empty while displayBreaks is not, ensuring labels always display correctly. All tests passing: - GuideColorStepsTest: 14/14 tests passing - All Guide*Test: 41 tests passing Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
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.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
matrix-charts/src/main/groovy/se/alipsa/matrix/gg/render/GgRenderer.groovy
Outdated
Show resolved
Hide resolved
matrix-charts/src/main/groovy/se/alipsa/matrix/gg/render/GgRenderer.groovy
Outdated
Show resolved
Hide resolved
matrix-charts/src/main/groovy/se/alipsa/matrix/gg/render/GgRenderer.groovy
Outdated
Show resolved
Hide resolved
matrix-charts/src/main/groovy/se/alipsa/matrix/gg/render/GgRenderer.groovy
Outdated
Show resolved
Hide resolved
matrix-charts/src/main/groovy/se/alipsa/matrix/gg/render/GgRenderer.groovy
Outdated
Show resolved
Hide resolved
…derer.groovy Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Four improvements based on code review:
1. Remove misleading comment (line 2766):
- Deleted "Skip to border and labels rendering" comment
- Code naturally flows to border rendering without any skip/jump
2. Fix list mutation in reverse() calls (lines 2749-2750):
- Changed breaks.reverse() to breaks.reverse(false)
- Changed labels.reverse() to labels.reverse(false)
- Prevents mutating shared lists, avoiding side effects
3. Make custom guide IDs unique (lines 2421, 2448, 2394):
- Updated renderCustomGuide() to accept aesthetic name parameter
- Changed ID from 'custom-guide' to 'custom-guide-${aesthetic}'
- Prevents duplicate IDs when multiple custom guides are used
- Updated test to expect 'custom-guide-custom' instead of 'custom-guide'
4. Simplify redundant ternary operator (line 2852):
- Changed: vertical ? y + barHeight + spacing : y + barHeight + spacing
- To: y + barHeight + spacing
- Both branches returned the same value
All tests passing:
- GuideColorStepsTest: 14/14 tests passing
- GuideCustomTest: 13/13 tests passing
- All Guide*Test: 41 tests passing
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
guide_coloursteps, guide_axis_logticks, and guide_custom
Add three new guide types to the ggplot2 implementation:
guide_coloursteps() / guide_colorsteps()
guide_axis_logticks()
guide_custom()
Also added factory methods for guide_axis_theta() and guide_axis_stack() (rendering implementations pending).
All tests passing. Updated ggTodo.md to reflect completion of section 4.8.1, 4.8.2, and 4.8.5.