Skip to content

ENH speedup enet_coordinate_descent_gram #31880

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Aug 8, 2025
Merged

ENH speedup enet_coordinate_descent_gram #31880

merged 7 commits into from
Aug 8, 2025

Conversation

lorentzenchr
Copy link
Member

Reference Issues/PRs

None

What does this implement/fix? Explain your changes.

This PR speeds up enet_coordinate_descent_gram by saving one call of _axpy (BLAS level 1) in the inner loop.

Any other comments?

Benchmarking is tricky because the bottleneck is usually the data preprocessing (like validating, i.e. copying, and centering of X and y).

@lorentzenchr
ENH speed up enet_coordinate_descent_gram
9b31d63 
* safe one call of _axpy in inner loop
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 4, 2025 •
edited
Loading

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: ddbe2d8. Link to the linter CI: here

@lorentzenchr
Copy link
Member Author

lorentzenchr commented Aug 4, 2025 •
edited
Loading

Benchmarking

See notebook in 8022fc6 (link to notebook) gives timings

image

Results as numbers:

columns: timing, value objective function, mean(|coef - optimal_coef|)

cd_results_main = np.array(
    [
        [5.04482031e-01, 7.57670773e+03, 3.42140309e-01],
       [1.10022402e+00, 7.53403836e+03, 2.89433791e-01],
       [1.97008801e+00, 8.85767885e+03, 3.01503615e-01],
       [2.41216612e+00, 7.01619669e+03, 2.57807888e-01],
       [3.49784660e+00, 8.43652475e+03, 2.73777630e-01],
       [4.55174208e+00, 7.12579667e+03, 2.45977591e-01],
       [5.29429817e+00, 7.68852160e+03, 2.50490130e-01],
       [6.42195415e+00, 8.91115432e+03, 2.75837493e-01],
       [6.97268987e+00, 8.79644301e+03, 2.72195146e-01],
       [9.40416098e+00, 8.65606151e+03, 2.61646490e-01]
    ]
)
cd_results_pr = np.array(
    [
        [4.85033989e-01, 7.57670773e+03, 3.42140309e-01],
        [1.06766486e+00, 7.53403836e+03, 2.89433791e-01],
        [1.75434995e+00, 8.85767885e+03, 3.01503615e-01],
        [2.41252184e+00, 7.01619669e+03, 2.57807888e-01],
        [3.20060396e+00, 8.43652475e+03, 2.73777630e-01],
        [4.12740493e+00, 7.12579667e+03, 2.45977591e-01],
        [5.13423800e+00, 7.68852160e+03, 2.50490130e-01],
        [6.07307792e+00, 8.91115432e+03, 2.75837493e-01],
        [7.16410398e+00, 8.79644301e+03, 2.72195146e-01],
        [7.82733417e+00, 8.65606151e+03, 2.61646490e-01]
    ]
)

Also very interesting, within the debug/profiling commit - indented lines add up to the next unindented line below:

X, y = make_regression(
    n_samples=50_000, n_features=1000, noise=0.1, n_informative=100, random_state=seed
)
X = np.asfortranarray(X)
%time ElasticNet(alpha=0.19, l1_ratio=l1_ratio, precompute=True, verbose=1).fit(X, y);
  Time in fit validation = 0.26387786865234375 s
  Time in fit _pre_fit = 0.5280959606170654 s
Time in fit preprocessing = 0.7926478385925293 s
  Init time = 0.000631
  Time feature loop = 0.00221 s
  Time convergence = 6e-06 s
  Number of convergence checks = 1 in 5 iterations
Time in fit path function = 0.0032510757446289062 s
Time in fit postprocessing = 0.00015115737915039062 s
CPU times: user 3 s, sys: 153 ms, total: 3.15 s
Wall time: 830 ms

and

%time ElasticNet(alpha=0.19, l1_ratio=l1_ratio, precompute=False, verbose=1).fit(X, y);
  Time in fit validation = 0.2920999526977539 s
  Time in fit _pre_fit = 0.08787274360656738 s
Time in fit preprocessing = 0.3801398277282715 s
  Init time = 0.06857699999999989
  Time feature loop = 0.306567 s
  Time convergence = 0.019407 s
    Number of convergence checks = 1 in 5 iterations
Time in fit path function = 0.39502692222595215 s
Time in fit postprocessing = 0.00015878677368164062 s
CPU times: user 854 ms, sys: 153 ms, total: 1.01 s
Wall time: 807 ms

OmarManzoor
OmarManzoor approved these changes Aug 6, 2025
View reviewed changes
Copy link
Contributor

@OmarManzoor OmarManzoor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you @lorentzenchr

@OmarManzoor OmarManzoor added the Waiting for Second Reviewer First reviewer is done, need a second one! label Aug 6, 2025
@lorentzenchr lorentzenchr changed the title ENH speed up enet_coordinate_descent_gram ENH speedup enet_coordinate_descent_gram Aug 6, 2025
ogrisel
ogrisel approved these changes Aug 7, 2025
View reviewed changes
Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Irrespective of the performance improvement, I like the more readable code of the diff.

@ogrisel ogrisel removed the Waiting for Second Reviewer First reviewer is done, need a second one! label Aug 7, 2025
OmarManzoor
OmarManzoor approved these changes Aug 8, 2025
View reviewed changes
Copy link
Contributor

@OmarManzoor OmarManzoor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thank you for the updates @lorentzenchr

@OmarManzoor OmarManzoor merged commit 8525ba5 into scikit-learn:main Aug 8, 2025
36 checks passed
@lorentzenchr lorentzenchr deleted the cd_gram branch August 8, 2025 06:31
@lorentzenchr lorentzenchr mentioned this pull request Aug 12, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ogrisel ogrisel ogrisel approved these changes

@OmarManzoor OmarManzoor OmarManzoor approved these changes

Assignees
No one assigned
Labels
cython module:linear_model Performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lorentzenchr @ogrisel @OmarManzoor