Fixes based on comprehensive code review - #48
Open
poldrack wants to merge 29 commits into
Open
Conversation
The normal-equations oracle np.linalg.pinv(X.T @ X) @ X.T @ y is itself numerically inaccurate for wide designs, which caused eight test_fracridge_singleton_target failures at nn=284, pp=1000. Use np.linalg.pinv(X) @ y, the stable minimum-norm least-squares solution. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three failing tests: the constructor must leave fracs unset rather than assigning self, and cross-validated fits on noiseless affine data must recover a nonzero intercept for one and two targets. A fit_intercept=False control pins the behavior that must not change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
FracRidgeRegressorCV passed self as the base class fracs argument and centered the complete dataset before GridSearchCV built folds, so the copied intercept was defined in centered coordinates while predict received raw input. Pass fracs=None explicitly and validate raw inputs with validate_data, letting each inner estimator own its preprocessing. The diagnostic notebook still asserts the old broken behavior and is updated in a later commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Failing tests on a deterministic tall design with a near-duplicate column (cond(X) = 2.4e8): fraction-1.0 coefficients must agree with np.linalg.lstsq to 1e-6 relative error for both JIT modes and for two targets, a fraction below one must achieve the requested norm ratio, and a rank-deficient design must return finite minimum-norm coefficients. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
_do_svd decomposed X.T @ X for tall designs and took square roots of the singular values, squaring the condition number and destroying coefficient accuracy before shrinkage. Compute one economy SVD of X for every shape, project y into the left singular basis, and divide only where singular values exceed tol. fracridge now passes tol into _do_svd and uses the same active criterion for its warning and mask. The diagnostic notebook still asserts the old broken behavior and is updated in the next commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An exactly-zero singular value makes fracridge return all-NaN coefficients and alphas, because the shrinkage factor computes 0 / 0 against the zero entry of the alpha grid. A near-duplicate column does not reproduce this: LAPACK returns ~1e-15 there, not 0.0. These tests use a literally zero column and cover both JIT modes, a shrunk fraction, fraction 1.0, and a two-target response. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The scaling factor lambda**2 / (lambda**2 + alpha) was computed unmasked, and the alpha grid starts at exactly 0, so a zero singular value produced 0 / 0. That NaN propagated through the interpolation into every fraction and every target, so a design with one zero column returned all-NaN coefficients and alphas behind a bare RuntimeWarning. Compute the factor through a masked divide keyed on the same active criterion _do_svd already uses. Inactive directions carry zero coefficients, so leaving their factor at zero is numerically inert: the whole suite is unchanged apart from NaN becoming 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Update the expected code-cell digest to the proposed final notebook and require headings that frame each defect as a legacy code path measured against the corrected package, including an explicit statement that the legacy reproductions are local rather than monkeypatches. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Replace the assertions that required broken package behavior with local reproductions of the two removed algorithms. fit_legacy_cv reproduces the outer-preprocessing sequence and legacy_normal_equations_coefficients reproduces the tall fraction-1.0 X.T @ X path, both measured against the corrected package, np.linalg.lstsq, and the retained direct-SVD control. The helpers reproduce removed internals for evidence; they do not monkeypatch the package. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The custom backend's LAPACK branches are only reached when p / n < 0.001. Decomposing X directly instead of X.T @ X made that branch reachable for tall designs for the first time, and it raises TypeError there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The import bound the scipy.linalg.lapack module to the name get_lapack_funcs, so both LAPACK call sites raised TypeError when reached. Only the numba path was ever exercised, because the previous normal-equations code always passed a square p x p matrix and so never took the p / n < 0.001 branch. Import the real get_lapack_funcs and pass the array so the right precision is selected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Decomposing X directly rather than X.T @ X costs roughly 10 to 30 times more for designs with many more observations than parameters, and allocates one additional array. The docs advertised the method as fast and scalable without noting that this release changes that trade deliberately. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fix three numerical defects and rewrite the diagnostic notebook as legacy-versus-corrected evidence. - FracRidgeRegressorCV no longer preprocesses the whole dataset before GridSearchCV builds folds, so the intercept it copies from best_estimator_ is defined in the same coordinate system predict receives. Its constructor passes fracs=None instead of self. - _do_svd decomposes X directly with one economy SVD for every shape instead of decomposing X.T @ X for tall designs, and divides by singular values only above tol. This trades roughly 10-30x throughput on tall designs for coefficient accuracy; documented in the user guide. - The shrinkage factor is computed through a masked divide, so an exactly-zero singular value no longer poisons the entire result with NaN. - _linalg.py imports the real get_lapack_funcs instead of aliasing the lapack module, fixing a TypeError on the default jit path that decomposing X directly made reachable for tall designs. Every production change followed a committed RED test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
make run-notebook executes the notebook in place with a real Jupyter kernel, layering nbconvert and ipykernel in for that command rather than adding them to the package dependencies. make check-notebook keeps the faster in-process cell check. Drop the two notebook tests that only existed to drive the notebook's construction. test_notebook_is_output_free enforced an empty-output file and test_notebook_preserves_base_serialization asserted no cell carries an id field, which Jupyter adds on every save; together they made executing the notebook fail the suite. The remaining three tests still check that every cell runs, that the maintainer narrative is present, and that the code cells are unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The digest pinned a SHA-256 of the notebook's code cells so a RED test could precede the notebook itself. That scaffolding has served its purpose, and keeping it means every intentional notebook edit fails the suite until someone recomputes the hash. Git already records unintended changes. Two checks remain: every code cell still executes, and the maintainer narrative headings are present. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Each test now carries a two-to-three line docstring saying which defect it guards and why it is written the way it is, so the reason a tolerance, a shape assertion, or a fixture looks unusual is readable at the test rather than only in the commit history. Also move the zero_column_design fixture up with the other fixtures, where it belongs; it had been appended below the tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Add a two-to-three line docstring to each test saying what it guards, plus a module comment explaining the (nn, pp) sweep: tall, wide, and moderately tall designs take different paths through the SVD, and the bb sweep reaches more targets than observations in the wide case. Records two things that were only implicit: make_data's oracle is pinv(X) rather than the normal-equations form because the latter is itself inaccurate for wide designs, and test_benchmark_fracridge asserts nothing, so it cannot fail on speed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
poldrack
marked this pull request as draft
August 4, 2026 23:14
poldrack
marked this pull request as ready for review
August 5, 2026 20:08
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
1 participant
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 includes changes based on an AI-driven code review and refactor. Below is Copilot's summary. Note that the actual production change itself is small: fracridge/fracridge.py +87/−36 and fracridge/_linalg.py +3/−3. A notebook (examples/diagnose_intercept_and_svd_inaccuracy.ipynb) is also included to demonstrate the two major issues (intercept handling and SVD computation), and the test suite was also expanded significantly. Added Python 3.14 to testing matrix and confirmed that it runs.
(sorry for bundling this all together into a big PR!)
Copilot summary
This pull request introduces significant improvements to the numerical accuracy, reliability, and developer experience of the
fracridgepackage. The most important changes include switching the core SVD computation to operate directly on the design matrix for improved numerical stability, fixing intercept handling in cross-validated regression, and greatly expanding the test suite to cover both legacy and new behaviors. Additionally, developer tooling and documentation have been enhanced to support these changes.Core algorithm and numerical accuracy:
fracridgenow decomposes the design matrixXdirectly, rather than the normal matrixX.T @ X, improving numerical stability and ensuring accurate coefficients even for nearly collinear designs. This change is not configurable and is documented as a deliberate trade-off of speed for accuracy. [1] [2] [3]Bug fixes and reliability:
FracRidgeRegressorCVwhere the constructor incorrectly setselfas the base class'sfracsargument, and corrected intercept handling so that predictions and scoring now properly account for the fitted intercept in both single and multi-target cases. [1] [2] [3]Testing and regression coverage:
Developer tooling and documentation:
Makefilewith developer entry points for running, checking, and linting the diagnostic notebook, as well as running the full test suite, improving the developer workflow.Internal code and dependency cleanup:
get_lapack_funcsand improving compatibility. [1] [2]These changes collectively make
fracridgemore robust, accurate, and maintainable, with a strong focus on correctness and reproducibility.