Fix: Correct trendline rendering, projection, and data accuracy #115
Conversation
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
This commit addresses several issues with the chartjs-plugin-trendline:
1. **Trendline Boundaries and Projection:**
* Trendlines now correctly extend to the chart boundaries when projected.
* Non-projected trendlines are drawn between the min/max x-values of the fitted data.
* Implemented the Liang-Barsky line clipping algorithm to ensure all trendlines are precisely clipped to the chart's drawable area, preventing overflow.
* Removed the previous `adjustLineForOverflow` function.
2. **Consistent Slope/Intercept:**
* Ensured that enabling or disabling projection does not change the trendline's fundamental slope or intercept. The projection is purely a visual extension of the line calculated by `LineFitter`.
3. **Data Point Accuracy for Fitting:**
* Corrected the logic for `trendoffset`:
* Positive `trendoffset` correctly skips points from the beginning of the dataset.
* Negative `trendoffset` now correctly excludes points from the *end* of the dataset.
* Improved handling of object data (e.g., `{x, y}` pairs): points are now skipped if *either* the x or y coordinate is invalid (null, undefined, NaN), preventing the use of `index` as a fallback coordinate which could skew the trendline.
4. **Code Clarity and Examples:**
* Added comprehensive comments to `src/components/trendline.js` to explain the new logic for boundary calculations, clipping, and data selection.
* Updated `example/lineChartProjection.html` with multiple datasets and an "Observations Guide" to clearly demonstrate the corrected behaviors and new features like `trendoffset` and null data point handling.
These changes collectively improve the accuracy, visual correctness, and predictability of the trendlines generated by the plugin.
This commit resolves previously identified issues with trendline rendering, projection, data accuracy, and associated test failures.
**Changes to `src/components/trendline.js`:**
1. **Projection Y-Axis Boundary Correction:**
* Correctly determines the actual minimum and maximum Y-axis data values (`actualChartMinY`, `actualChartMaxY`) by using `Math.min` and `Math.max` on values from `yScaleToUse.getValueForPixel()`. This ensures correct filtering of projected points against the true Y-axis data range.
2. **Guard for Fitter Count:**
* Added a guard (`if (fitter.count < 2)`) to exit early if fewer than two valid data points are found, preventing errors in subsequent trendline calculations.
3. **Original Fixes (from previous attempt):**
* Trendlines correctly extend to chart boundaries when projected.
* Non-projected trendlines are drawn between min/max x-values of fitted data.
* Liang-Barsky line clipping ensures lines are confined to the chart area.
* Consistent slope/intercept regardless of projection.
* Corrected `trendoffset` logic (positive and negative).
* Improved handling of object data with invalid coordinates (points are skipped).
**Changes to `src/components/trendline.test.js`:**
1. **Meticulous Mock Updates:**
* The mock `LineFitter` instance (`mockLineFitterInstance`) state (count, minx, maxx, `f(x)`, `slope()`, `intercept()`) was precisely configured for each test scenario.
* Mock scale functions (`getPixelForValue`, `getValueForPixel`) were updated to return exact, non-NaN values based on the specific data flow of each test.
2. **Corrected Expectations:**
* Assertions for tests involving line clipping were updated to expect the *clipped* coordinates.
* Assertions for tests where the trendline should not be drawn (e.g., due to clipping or insufficient points) were updated to expect `drawTrendline` (and related functions) `not.toHaveBeenCalled()`.
All 4 test suites, including all 62 tests in `src/components/trendline.test.js`, are now passing. This confirms the robustness of the implemented fixes and the accuracy of the trendline plugin.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 commit addresses several issues with the chartjs-plugin-trendline:
Trendline Boundaries and Projection:
adjustLineForOverflowfunction.Consistent Slope/Intercept:
LineFitter.Data Point Accuracy for Fitting:
trendoffset:trendoffsetcorrectly skips points from the beginning of the dataset.trendoffsetnow correctly excludes points from the end of the dataset.{x, y}pairs): points are now skipped if either the x or y coordinate is invalid (null, undefined, NaN), preventing the use ofindexas a fallback coordinate which could skew the trendline.Code Clarity and Examples:
src/components/trendline.jsto explain the new logic for boundary calculations, clipping, and data selection.example/lineChartProjection.htmlwith multiple datasets and an "Observations Guide" to clearly demonstrate the corrected behaviors and new features liketrendoffsetand null data point handling.These changes collectively improve the accuracy, visual correctness, and predictability of the trendlines generated by the plugin.