Please undo any changes you do to future::plan()

[HenrikBengtsson]

Congrats to the CRAN release.

Author of Futurverse here. I've got some comments on the parallelization code in:

TaxaNorm/R/norm_func_ZINB.R

Lines 79 to 84 in 2dcf392

	if (is.null(ncores)) {ncores <- max(1, parallel::detectCores() - 1)}
	if (ncores > 1) {
	message(paste0("Setting up parallel computation using ", ncores, " cores... \n" ))
	if (.Platform$OS.type == "windows" | parallelly::supportsMulticore() == FALSE) {
	future::plan(future::multisession, workers=ncores)} else {
	future::plan(future::multicore, workers=ncores)}

Please undo any changes you do to future::plan(). If not, you might break other code that the user runs. This is explained in Section 'For package developers' of ?future::plan. That section also shows how to undo settings. This one is important.

Also, for reasons that I bring up in https://www.jottr.org/2022/12/05/avoid-detectcores/,

1. please don't use parallel::detectCores(), and
2. please don't default to using all cores, or almost all cores. It's also a big no-no on CRAN.

Even if I cannot convince you to agree with me on the above, maybe you could replace:

if (is.null(ncores)) {ncores <- max(1, parallel::detectCores() - 1)} 

with

if (is.null(ncores)) {ncores <- parallelly::availableCores(omit = 1)}

It'll give sysadms a chance to control the number of default CPU cores.

[HenrikBengtsson]

Forgot to mention that in:

if (.Platform$OS.type == "windows" | parallelly::supportsMulticore() == FALSE) {

you want to use || rather than |. You can also use ! parallelly::supportsMulticore(), which is more common.

[wangziyue57]

Thanks a lot for the suggestion! We're working on updating the package and will incorporate this in the newer version.