-
Notifications
You must be signed in to change notification settings - Fork 1
Description
Dear nlstools developers - thanks for the work on this great package! @trhermes and I use it for a cool research application.
Unfortunately a seemingly small change in nlstools v2.1-0 breaks the previously reliable curve fitting workflow in one of our scripts here.
Here's a minimal, reproducible example to illustrate the issue:
set.seed(138)
d <- structure(list(X = c(-39.5, -37, -34, -31.3, -28.1, -28.1, -25.2,
-22.5, -20, -17, -14.3, -14.3, -11.8, -8.5), Y = c(-10.718, -8.543,
-6.303, -3.781, -2.593, -2.59, -3.85, -5.25, -7.62, -9.943, -10.322,
-10.385, -8.334, -4.668)), class = "data.frame", row.names = c(NA,
-14L))
low <- list(A = 1, x_0 = -45, z = 10, M = -15)
start <- list(A = 5, x_0 = 25, z = 45, M = -5 )
up <- list(A = 15, x_0 = 45, z = 90, M = 10)
fit_res <- stats::nls(
Y ~ A * cos(2 * pi * ((X - x_0) / z)) + M,
data = d,
lower = low, start = start, upper = up,
algorithm = "port",
control = nls.control(maxiter = 100000)
)
rm(start)
nlstools::nlsBoot(fit_res, niter = 10000)This now fails in v2.1-0 with
Error in nlstools::nlsBoot(fit_res, niter = 10000) :
Procedure aborted: the fit only converged in 1 % during bootstrapping
while it works with the old version of the nlsBoot() function here: https://github.com/lbbe-software/nlstools/blob/1ca55aa35655c8ea44e53b0fa2112d2497bd50b9/R/nlsBoot.R
The crucial line to trigger the failure in the little example above is rm(start). Of course this line is nonsensical like this. But if you look into the script linked above where we apply nlstools, you'll notice how this very situation can indeed occur.
I can hack around the issue and make the new version work by renaming start and setting a global variable (<<-) with the new name, containing the content of start. Pretty hacky and it does not resolve the larger, underlying issue:
I believe nlsBoot() (and really any function in any R package) should not rely on the parent environment to include certain variables. This can easily backfire, as in this particular case. Even R functions should be as pure as possible. If start is necessary for nlsBoot(), then it should just be a clearly documented input argument.