fix(basis): B-spline LS round-trip transpose + point-wise nbasis CV (GH #33) - #36
Merged
Conversation
#33) Bug 1 — fdata_to_basis / basis_to_fdata transposed results for n > 1 curves. Both built a curve-major (row-major) flat buffer and passed it to FdMatrix::from_column_major, which reads it column-major — so for multiple curves the coefficients/reconstruction were scrambled. n = 1 coincides (row==column major), which is why a single smooth curve round-tripped perfectly while a 35-curve matrix gave residuals worse than a constant fit, non-monotone in n_basis, and outside the data range. The least-squares solve itself is correct. Fix: collect per-curve rows and scatter them into the matrix via [(i, k)] / [(i, j)] indexing. Bug 2 — basis_nbasis_cv (Cv criterion) always selected the maximum n_basis. evaluate_nbasis_cv fit each fold's training curves but discarded those coefficients, then projected every TEST curve onto the basis using its own data and scored it against itself — an in-sample residual with no hold-out, monotone decreasing in n_basis. Leaving out curves is ill-posed for per-curve basis fitting anyway. Fix: cross-validate over TIME POINTS — fit on the retained points, predict the held-out points — a genuine predictive score that penalizes overfitting and shows an interior minimum. (GCV/AIC/BIC were already correct and are unchanged.) Regression tests: multi-curve B-spline round-trip beats a constant fit and improves with n_basis while staying in range; CV no longer selects the max n_basis on smooth+noise data. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #36 +/- ##
=======================================
Coverage 89.01% 89.02%
=======================================
Files 200 200
Lines 44099 44124 +25
=======================================
+ Hits 39255 39281 +26
+ Misses 4844 4843 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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
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.
Fixes #33.
Two independent defects in the 1-D B-spline basis path.
Bug 1 —
fdata_to_basis/basis_to_fdatatranspose the result forn > 1Both functions built a curve-major flat buffer (
[i*K + k],[i*m + j]) and passed it toFdMatrix::from_column_major, which reads it column-major ([row + col*n]). For a single curve the two layouts coincide (why a smooth 1-curve round-trip was perfect), but for multiple curves the coefficients and reconstruction were transposed/scrambled — hence residuals worse than a constant fit, flat inn_basis, and outside the data range on the 35×365 weather matrix. The least-squares solve(BᵀB)⁻¹Bᵀitself is correct.Fix: collect per-curve rows and scatter them into the matrix via
[(i, k)]/[(i, j)]indexing (column-major-correct), preserving parallelism.Bug 2 —
basis_nbasis_cv(Cv) always selects the maximumn_basisevaluate_nbasis_cvfit each fold's training curves but threw those coefficients away, then projected every test curve onto the basis using its own data and scored it against itself — a pure in-sample residual with no hold-out, monotone-decreasing inn_basis. (Leaving out whole curves is ill-posed for per-curve basis fitting: a held-out curve can't be predicted from other curves' coefficients.)Fix: cross-validate over time points — fit coefficients on the retained points, predict the held-out points, score the prediction error. This is a genuine predictive criterion that penalizes overfitting and shows an interior minimum. The GCV/AIC/BIC paths were already correct and are unchanged.
Tests
test_bspline_roundtrip_multi_curve_is_least_squares: multi-curve round-trip beats a constant fit, improves withn_basis, stays in range.test_basis_nbasis_cv_penalizes_overfitting: CV no longer selects the maxn_basis(and scores aren't monotone) on smooth+noise data.--all-targets+ rustfmt clean.🤖 Generated with Claude Code