Improve convergence handling in fit_fims by wrapping nlminb calls with tryCatch#1385
Improve convergence handling in fit_fims by wrapping nlminb calls with tryCatch#1385aman-raj-srivastva wants to merge 2 commits intoNOAA-FIMS:devfrom
Conversation
|
Thank you for contributing to FIMS and opening your first PR here! We are happy to have your contributions. Please ensure that the PR is made to the dev branch and let us know if you need any help! Also, we encourage you to introduce yourself to the community on the introduction thread in our Discussions. |
🎨 Chore: code formatting workflowOur automated workflows cannot run on forks because of permission issues, and thus, we ask that you run the following code locally and push any changes that are created to your feature branch. You will only be reminded of this once per PR. Thank you! Format C++ code
Format R code
styler::style_pkg() # Style R code
roxygen2::roxygenise() # Update documentation
styler::style_pkg() # Style R code again
roxygen2::roxygenise() # Update documentation again
usethis::use_tidy_description() # Style DESCRIPTION filePush changes
|
|
Hi, thanks again for assigning this issue. |
kellijohnson-NOAA
left a comment
kellijohnson-NOAA
left a comment
There was a problem hiding this comment.
Thanks for the speedy PR. I have some inline comments and I am also wondering about the location of "convergence", where I am assuming this is a typical item in opt so we just recreate that but with a non-converged value of 1.0? Also, should it be 1L to force it to be an integer?
R/fimsfit.R
Outdated
| }, | ||
| error = function(e) { | ||
| cli::cli_warn(c( | ||
| "!" = paste("nlminb failed:", e$message) |
There was a problem hiding this comment.
{cli} allows one to use inherit functionality of {glue} so you do not have to use paste, e.g.,
| "!" = paste("nlminb failed:", e$message) | |
| "!" = "nlminb failed: {e$message}" |
There was a problem hiding this comment.
I will replace paste() with glue-style formatting as suggested.
R/fimsfit.R
Outdated
| ) | ||
|
|
||
| if (is.null(opt)) { | ||
| cli::cli_warn("Optimization failed. Returning partial results.") |
There was a problem hiding this comment.
| cli::cli_warn("Optimization failed. Returning partial results.") |
I added this to the vector of warnings above.
R/fimsfit.R
Outdated
| }, | ||
| error = function(e) { | ||
| cli::cli_warn(c( | ||
| "!" = paste("nlminb failed during loop:", e$message) |
There was a problem hiding this comment.
Same comment as above.
| ) | ||
|
|
||
| if (is.null(opt)) { | ||
| break |
There was a problem hiding this comment.
Should this just break or should it return the same style of non-converged item that is returned above? If this loop doesn't converge does it go back to the previous object that did converge? I am confused on what the outcome of break leads to here.
There was a problem hiding this comment.
You are right, using break with a NULL opt can lead to ambiguity. I will update the logic to retain the last successful optimization result and use that as a fallback if a failure occurs during the loop. This ensures that the returned object remains consistent.
|
Thanks for the feedback! I have addressed the comments and pushed the updates. |
2 participants
This PR improves convergence handling in fit_fims by wrapping nlminb calls in tryCatch blocks.
Changes include:
This addresses issue #1370 and improves robustness and user feedback during optimization.
Instructions for code reviewer
👋Hello reviewer👋, thank you for taking the time to review this PR!
nit:(for nitpicking) as the comment type. For example,nit:I prefer using adata.frame()instead of amatrixbecause ...This PR is now ready to be merged.Checklist