brunobrr / bdc

Check out the vignettes with detailed documentation on each module of the bdc package
https://brunobrr.github.io/bdc
GNU General Public License v3.0
23 stars 7 forks source link

doParallel shouldn't be just a suggestion or parallel should default to false #250

Closed black-snow closed 1 year ago

black-snow commented 1 year ago

I was wondering why I had to load doParallel even though I don't use it in my code. The reason is that bdc_suggest_names_taxadb() has an argument parallel, that defaults to TRUE and given that parallel == TRUE bdc utilizes doParallel: https://github.com/brunobrr/bdc/blob/6932e76d27f2d2a15707739b49e19c897791253b/R/bdc_suggest_names_taxadb.R#L129

I think the default for parallel should either be FALSE or you should move doParallel from suggestions up to imports.

kguidonimartins commented 1 year ago

doParallel is required to register/open connections with cores.

black-snow commented 1 year ago

doParallel is required to register/open connections with cores.

Hi @kguidonimartins what cores and how does this address the issue that it just fails with its defaults?

kguidonimartins commented 1 year ago

Cores on which data processing is distributed. And about the issue you're reporting, well there isn't an apparent problem. The function works as expected. Tests report no issues. Anyway, if you prefer, submit a PR with your suggestions. If both checks and tests pass, we accept your changes.

On Sun, 19 Mar 2023 at 19:57 Ronald @.***> wrote:

doParallel is required to register/open connections with cores.

Hi @kguidonimartins https://github.com/kguidonimartins what cores and how does this address the issue that it just fails with its defaults?

— Reply to this email directly, view it on GitHub https://github.com/brunobrr/bdc/issues/250#issuecomment-1475425583, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB6JBVVY23UXXSKL4XF2IRLW46FN3ANCNFSM6AAAAAAWALYNB4 . You are receiving this because you were mentioned.Message ID: @.***>

black-snow commented 1 year ago

Aight. The easiest fix is to just move it up to the imports: https://github.com/brunobrr/bdc/pull/251