ENH precompute residual and column norm of X in coordinate descent - #34572
Merged
Conversation
lorentzenchr
force-pushed
the
cd_colnorm_precompute
branch
from
July 27, 2026 06:08
93d481b to
255204c
Compare
Member
Author
Small benchmark on leukemia dataset
Branch mainBranch PRThis corresponds to a -54%, -37% and -12% fit time reduction. CodeDetailsimport time
import numpy as np
import polars as pl
import seaborn as sns
import sklearn
from sklearn.datasets import fetch_openml
from sklearn.linear_model import Lasso, LassoCV
from sklearn.preprocessing import StandardScaler, LabelEncoder
X, y = fetch_openml("leukemia", version=1, return_X_y=True)
n_samples, n_features = X.shape
# Because we apply a penalty, we standarize X (zero mean and unit variance).
X = StandardScaler().fit_transform(X)
X = np.asfortranarray(X) # Lasso prefers Fortran-contiguous
y = LabelEncoder().fit_transform(y)
print(f"{n_samples=} {n_features=}")
n_alphas = 100
tols = [1e-2, 1e-3, 1e-4]
timing = np.zeros(len(tols))
for tol_ix, tol in enumerate(tols):
t0 = time.time()
m = LassoCV(alphas=n_alphas, tol=tol).fit(X, y)
timing[tol_ix] = time.time() - t0
df = pl.DataFrame({"tolerance": tols, "time": timing, "version": sklearn.__version__})
df |
OmarManzoor
reviewed
Jul 27, 2026
OmarManzoor
left a comment
Contributor
There was a problem hiding this comment.
@lorentzenchr Thank you for the PR
OmarManzoor
approved these changes
Jul 28, 2026
OmarManzoor
left a comment
Contributor
There was a problem hiding this comment.
LGTM. Thank you @lorentzenchr
| excluded_set = np.empty(n_features, dtype=np.uint8) | ||
|
|
||
| if Qw is None: | ||
| # initial value "Q w" which will be kept of up to date in the iterations |
Contributor
There was a problem hiding this comment.
Suggested change
| # initial value "Q w" which will be kept of up to date in the iterations | |
| # initial value "Q w" which will be kept up to date in the iterations |
Member
Author
There was a problem hiding this comment.
I ended up with b50b4ff to align the docstring of the CD routines.
Downside: much larger diff.
virchan
approved these changes
Jul 29, 2026
virchan
left a comment
Member
There was a problem hiding this comment.
Thank you for the PR, @lorentzenchr!
Overall, LGTM. Just have a minor question before merging.
Co-authored-by: Virgil Chan <virchan.math@gmail.com>
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.
Reference Issues/PRs
None.
This PR needed #34514 as preparation.
What does this implement/fix? Explain your changes.
This PR adds precomputing of
R = y - X @ coefnorm2_cols_X = np.sum(X ** 2, axis=0)in the coordinate descent solvers (Cython). The main impact is when computing paths with
enet_pathorElasticNetCVbecause then, except for the very first path value, the initialization (memory) and computation of residuals and column norms is avoided.Note the at the end of a coordinate descent algo, residuals are always up to date.
AI usage disclosure
None
Any other comments?
No public API change.