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

nlsBoot fails to converge in v2.1-0 #7

Open
nevrome opened this issue May 8, 2024 · 1 comment
Open

nlsBoot fails to converge in v2.1-0 #7

nevrome opened this issue May 8, 2024 · 1 comment

Comments

@nevrome
Copy link

nevrome commented May 8, 2024

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.

@Florent-Baty
Copy link
Collaborator

Thanks Clemens for reporting this issue!

Indeed, some functions of nlstools including nlsBoot and nlsJack rely on the use of parent environment in order to accommodate with complex models, e.g. the ones including factors where confusion may occur with the names of the starting values. This was the purpose of the last update of the package.

I understand that it unfortunately impacted on your script which cannot be ran anymore. I will try to figure out how to solve this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants