Skip to content

[MRG] MNT: Use GEMV in enet_coordinate_descent (Pt. 1) #11896

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 1 commit into from
Aug 27, 2018
Merged

[MRG] MNT: Use GEMV in enet_coordinate_descent (Pt. 1) #11896

merged 1 commit into from
Aug 27, 2018

Conversation

jakirkham
Copy link
Contributor

Make use of the BLAS GEMV operation for non-transposed arrays in enet_coordinate_descent instead of using DOT in a for-loop. They are both semantically equivalent, but the former is likely multithreaded in BLAS implementations while here it is merely a serial loop. We avoid GEMV for transposed arrays as the reference BLAS included has a bug for this type of operation. When we either fix the reference BLAS or drop it, we can use GEMV for transposed arrays as well.

@jakirkham
MNT: Use GEMVN in enet_coordinate_descent
3a90bb2 
Make use of the BLAS GEMV operation for non-transposed arrays in
`enet_coordinate_descent` instead of using DOT in a `for`-loop. They are
both semantically equivalent, but the former is likely multithreaded in
BLAS implementations while here it is merely a serial loop. We avoid
GEMV for transposed arrays as the reference BLAS included has a bug for
this type of operation. When we either fix the reference BLAS or drop
it, we can use GEMV for transposed arrays as well.
@jakirkham jakirkham changed the title MNT: Use GEMV in enet_coordinate_descent (Pt. 1) [MRG] MNT: Use GEMV in enet_coordinate_descent (Pt. 1) Aug 23, 2018
@jakirkham jakirkham mentioned this pull request Aug 23, 2018
Merged
@jakirkham
Copy link
Contributor Author

Thoughts on this?

rth
rth approved these changes Aug 26, 2018
View reviewed changes
Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

Sound like a nice improvement, thanks @jakirkham !

Minor comment below.

sklearn/linear_model/cd_fast.pyx
@@ -205,8 +209,11 @@ def enet_coordinate_descent(floating[::1] w,

with nogil:
# R = y - np.dot(X, w)
for i in range(n_samples):
R[i] = y[i] - dot(n_features, &X[i, 0], n_samples, &w[0], 1)
copy(n_samples, &y[0], 1, &R[0], 1)
Copy link
Member

Choose a reason for hiding this comment

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

Is it not possible to copy 2 memoryviews without using BLAS functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had some issues getting that to pass on CI with some older versions of Cython. Also saw other functions in this file were using BLAS to do the copy. So figured this was preferred. Though we can look into this more closely if you prefer.

@jakirkham
Copy link
Contributor Author

Thanks for the feedback. Responded inline.

@rth rth merged commit 8d7ce8e into scikit-learn:master Aug 27, 2018
@rth
Copy link
Member

rth commented Aug 27, 2018

Thanks @jakirkham !

@jakirkham jakirkham deleted the opts_cd_fast_a branch August 27, 2018 13:27
yarikoptic added a commit to yarikoptic/scikit-learn that referenced this pull request Sep 10, 2018
@yarikoptic
Merge tag '0.20rc1' into releases
f3aff2d 
* tag '0.20rc1': (1109 commits)
  MNT rc version
  DOC Release dates for 0.20 (scikit-learn#11838)
  DOC Fix: require n_splits > 1 in TimeSeriesSplit (scikit-learn#11937)
  FIX xfail for MacOS LogisticRegressionCV stability (scikit-learn#11936)
  MNT: Use GEMV in enet_coordinate_descent (Pt. 1) (scikit-learn#11896)
  [MRG] TST/FIX stop optics reachability failure on 32bit (scikit-learn#11916)
  ENH add multi_class='auto' for LogisticRegression, default from 0.22; default solver will be 'lbfgs' (scikit-learn#11905)
  MAINT Fix test_logistic::test_dtype_match failure on 32 bit arch (scikit-learn#11899)
  DOC Updated link to Laurens van der Maaten's home page (scikit-learn#11907)
  DOC Remove stray backtick in /doc/modules/feature_extraction.rst (scikit-learn#11910)
  Deprecate min_samples_leaf and min_weight_fraction_leaf (scikit-learn#11870)
  MNT modify test_sparse_oneclasssvm to be parametrized (scikit-learn#11894)
  EXA set figure size to avoid overlaps (scikit-learn#11889)
  MRG/REL fixes /skips for 32bit tests (scikit-learn#11879)
  add durations=20 to makefile to show test runtimes locally (scikit-learn#11147)
  DOC loss='l2' is no longer accpeted in l1_min_c
  DOC add note about brute force nearest neighbors for string data (scikit-learn#11884)
  DOC Change sign of energy in RBM (scikit-learn#11156)
  RFC try to warn on iid less often (scikit-learn#11613)
  DOC reduce plot_gpr_prior_posterior.py warnings(scikit-learn#11664)
  ...
yarikoptic added a commit to yarikoptic/scikit-learn that referenced this pull request Sep 10, 2018
@yarikoptic
Merge branch 'releases' into dfsg
610cc46 
* releases: (1109 commits)
  MNT rc version
  DOC Release dates for 0.20 (scikit-learn#11838)
  DOC Fix: require n_splits > 1 in TimeSeriesSplit (scikit-learn#11937)
  FIX xfail for MacOS LogisticRegressionCV stability (scikit-learn#11936)
  MNT: Use GEMV in enet_coordinate_descent (Pt. 1) (scikit-learn#11896)
  [MRG] TST/FIX stop optics reachability failure on 32bit (scikit-learn#11916)
  ENH add multi_class='auto' for LogisticRegression, default from 0.22; default solver will be 'lbfgs' (scikit-learn#11905)
  MAINT Fix test_logistic::test_dtype_match failure on 32 bit arch (scikit-learn#11899)
  DOC Updated link to Laurens van der Maaten's home page (scikit-learn#11907)
  DOC Remove stray backtick in /doc/modules/feature_extraction.rst (scikit-learn#11910)
  Deprecate min_samples_leaf and min_weight_fraction_leaf (scikit-learn#11870)
  MNT modify test_sparse_oneclasssvm to be parametrized (scikit-learn#11894)
  EXA set figure size to avoid overlaps (scikit-learn#11889)
  MRG/REL fixes /skips for 32bit tests (scikit-learn#11879)
  add durations=20 to makefile to show test runtimes locally (scikit-learn#11147)
  DOC loss='l2' is no longer accpeted in l1_min_c
  DOC add note about brute force nearest neighbors for string data (scikit-learn#11884)
  DOC Change sign of energy in RBM (scikit-learn#11156)
  RFC try to warn on iid less often (scikit-learn#11613)
  DOC reduce plot_gpr_prior_posterior.py warnings(scikit-learn#11664)
  ...
yarikoptic added a commit to yarikoptic/scikit-learn that referenced this pull request Sep 10, 2018
@yarikoptic
Merge branch 'dfsg' into debian
151b85b 
* dfsg: (1109 commits)
  MNT rc version
  DOC Release dates for 0.20 (scikit-learn#11838)
  DOC Fix: require n_splits > 1 in TimeSeriesSplit (scikit-learn#11937)
  FIX xfail for MacOS LogisticRegressionCV stability (scikit-learn#11936)
  MNT: Use GEMV in enet_coordinate_descent (Pt. 1) (scikit-learn#11896)
  [MRG] TST/FIX stop optics reachability failure on 32bit (scikit-learn#11916)
  ENH add multi_class='auto' for LogisticRegression, default from 0.22; default solver will be 'lbfgs' (scikit-learn#11905)
  MAINT Fix test_logistic::test_dtype_match failure on 32 bit arch (scikit-learn#11899)
  DOC Updated link to Laurens van der Maaten's home page (scikit-learn#11907)
  DOC Remove stray backtick in /doc/modules/feature_extraction.rst (scikit-learn#11910)
  Deprecate min_samples_leaf and min_weight_fraction_leaf (scikit-learn#11870)
  MNT modify test_sparse_oneclasssvm to be parametrized (scikit-learn#11894)
  EXA set figure size to avoid overlaps (scikit-learn#11889)
  MRG/REL fixes /skips for 32bit tests (scikit-learn#11879)
  add durations=20 to makefile to show test runtimes locally (scikit-learn#11147)
  DOC loss='l2' is no longer accpeted in l1_min_c
  DOC add note about brute force nearest neighbors for string data (scikit-learn#11884)
  DOC Change sign of energy in RBM (scikit-learn#11156)
  RFC try to warn on iid less often (scikit-learn#11613)
  DOC reduce plot_gpr_prior_posterior.py warnings(scikit-learn#11664)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agramfort agramfort agramfort approved these changes

@rth rth rth approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jakirkham @rth @agramfort