Skip to content
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.

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

propose to change step_ica() default option to use "method = 'C'" #518

Closed
StefanBRas opened this issue Jun 2, 2020 · 1 comment · Fixed by #522
Closed

propose to change step_ica() default option to use "method = 'C'" #518

StefanBRas opened this issue Jun 2, 2020 · 1 comment · Fixed by #522
Labels
feature a feature request or enhancement next release 🚀

Comments

@StefanBRas
Copy link

Short version

step_ICA using options = list(method = 'C') is always faster than the default method = 'R, with no downsides.
I don't know what the policy is on changing external package defaults in recipes, but it's a 'free' speed gain.

Longer version

step_ica uses fastICA::fastICA which has an argument method. Quoting FastICA's documentation:

if method == "R" then computations are done exclusively in R (default). The code allows the interested R user to see exactly what the algorithm does. if method == "C" then C code is used to perform most of the computations, which makes the algorithm run faster. During compilation the C code is linked to an optimized BLAS library if present, otherwise stand-alone BLAS routines are compiled.

So in almost all cases, you want method = 'C'. The only use for 'R' seem to be the very specific case where you want to see the exact calculations - but then you would have to look in the documentation for step_ica anyway (and see the changed default).

So I propose to change the default option of step_ica to use method = 'C'. I can make a pull request (at some point) if you want.

Below is a small example of the speed gain. Note that it varies but is consistently positive.

library('fastICA')
library('tictoc')
set.seed(1)
X <- matrix(runif(5e6), 5e6 / 20, 20)
tic()
a <- fastICA(X, 20, method = "R")
toc()
#> 4.177 sec elapsed
tic()
b <- fastICA(X, 20, method = "C")
toc()
#> 2.629 sec elapsed

Created on 2020-06-02 by the reprex package (v0.3.0)

@topepo topepo added feature a feature request or enhancement next release 🚀 labels Jun 5, 2020
topepo added a commit that referenced this issue Jun 9, 2020
topepo added a commit that referenced this issue Jun 9, 2020
topepo added a commit that referenced this issue Jun 9, 2020
* sorts the juiced data the same as bake (alphabetically)

* Fixed partial matching on seq args

* first pass at mixOmics calculations

* test cases from mixOmics and old recipes

* pkgdown updates

* updated tidy method for pls

* num_terms -> predictor_prop

* change mixOmics treatment to check with R < 3.5

* still working with mixOmics check issues

* updated news

* small change for step_ica for #518

* install mixomics for pkgdown

* forgotten prep :-(

* pls examples fail for R < 3.5 :-O

* Fixed partial matching on seq args

* Updated NEWS.

Co-authored-by: topepo <mxkuhn@gmail.com>
@github-actions
Copy link

This issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex https://reprex.tidyverse.org) and link to this issue.

@github-actions github-actions bot locked and limited conversation to collaborators Feb 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature a feature request or enhancement next release 🚀
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants