search

stan-dev / projpred

Projection predictive variable selection
https://mc-stan.org/projpred/
Other
109 stars 25 forks source link

Please undo any changes in future::plan() to what it was before #397

Closed HenrikBengtsson closed 1 year ago

HenrikBengtsson commented 1 year ago

I see that you use:

https://github.com/stan-dev/projpred/blob/d6d9744b7ee84b9c14cffb846fc057195588f026/tests/testthat/test_parallel.R#L84

in an attempt to "undo" the future plan you have set earlier in:

https://github.com/stan-dev/projpred/blob/d6d9744b7ee84b9c14cffb846fc057195588f026/tests/testthat/test_parallel.R#L13-L21

Note that the user might have had another plan than the sequential set before, which means calling your function may change what the user have intended. A better solution is outlined in Section 'For package developers' of https://future.futureverse.org/reference/plan.html;

oplan <- plan(new_set_of_strategies)
on.exit(plan(oplan), add = TRUE)

Could you please update your code to do that instead?

HenrikBengtsson commented 1 year ago

Forgot to say, in the same vein, you can undo your doFuture::registerDoFuture() call using:

oldDoPar <- registerDoFuture()
on.exit(with(oldDoPar, foreach::setDoPar(fun=fun, data=data, info=info)), add = TRUE)

This is explained in Section 'For package developers' of https://dofuture.futureverse.org/reference/registerDoFuture.html.

fweber144 commented 1 year ago

Like #396, your comment sounds like it refers to something inside a function (the mentioned file only belongs to the tests). I don't think I can use on.exit() in a test file (outside of a function), can I?

HenrikBengtsson commented 1 year ago

Oh my, I missed it was a test script. So, yeah, ignore my comment/request. I came to this while troubleshooting a reverse dependency issue with doFuture.