Skip to content

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

Merged
rth merged 1 commit intoscikit-learn:masterfrom
jakirkham:opts_cd_fast_a
Aug 27, 2018
Merged

[MRG] MNT: Use GEMV in enet_coordinate_descent (Pt. 1)#11896
rth merged 1 commit intoscikit-learn:masterfrom
jakirkham:opts_cd_fast_a

Conversation

@jakirkham
Copy link
Copy Markdown
Contributor

@jakirkham jakirkham commented Aug 23, 2018

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
Copy Markdown
Contributor Author

jakirkham commented Aug 24, 2018

Thoughts on this?

rth
rth approved these changes Aug 26, 2018
View reviewed changes
Copy link
Copy Markdown
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
# 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
Copy Markdown
Member

@rth rth Aug 26, 2018

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
Copy Markdown
Contributor Author

@jakirkham jakirkham Aug 26, 2018

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
Copy Markdown
Contributor Author

jakirkham commented Aug 26, 2018

Thanks for the feedback. Responded inline.

@rth rth merged commit 8d7ce8e into scikit-learn:master Aug 27, 2018
@rth
Copy link
Copy Markdown
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

+1 more reviewer

@rth rth rth approved these changes

Reviewers whose approvals may not affect merge requirements

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