Skip to content

[Bug] Skip sdreport calculation when optimize=FALSE to prevent NaN warning#1102

Merged
kellijohnson-NOAA merged 1 commit into
mainfrom
copilot/fix-warn-sdreport-issue
Jan 15, 2026
Merged

[Bug] Skip sdreport calculation when optimize=FALSE to prevent NaN warning#1102
kellijohnson-NOAA merged 1 commit into
mainfrom
copilot/fix-warn-sdreport-issue

Conversation

Copilot AI commented Dec 30, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor
  • Initial plan created
  • Add optimize parameter to R layer (FIMSFit and fit_fims)
  • Add optimize parameter to C++ layer (to_json and get_report)
  • Skip sdreport when optimize=FALSE
  • Return empty objects for SE-related fields when skipped
  • Add test to verify no warnings with optimize=FALSE
  • Fix grouped_out initialization bug in rcpp_models.hpp
  • Fix NULL error when accessing names of empty grouped_out list
  • Add Doxygen documentation for to_json method
Original prompt

This section details on the original issue you should resolve

<issue_title>[Bug:] fit_fims(optimize = FALSE) throws warning related to error in SE calculations</issue_title>
<issue_description>### Describe the bug

fit_fims(optimize = FALSE) should not run sdreport. The fit_fims R code uses a flag to skip sdreport calculations, however, get_output() runs sdreport internally in FIMS, regardless if optimize is set to TRUE/FALSE.

Describe how to reproduce the bug

devtools::load_all()
load(test_path("fixtures", "integration_test_data.RData"))

# Load sample data
data("data1")
# Prepare data for FIMS model
data_4_model <- FIMSFrame(data1)

# Create parameters
default_parameters <- data_4_model |>
   create_default_configurations() |>
      create_default_parameters(data = data_4_model)

test_fit <- parameters_4_model |>
   initialize_fims(data = data_4_model) |>
      fit_fims(optimize = FALSE)

Screenshots

Image

Which OS are you seeing the problem on?

Linux

Which browser are you seeing the problem on?

No response

Which version of FIMS are you seeing the problem on?

0.7.0@dev

Additional context

Proposed Changes:

  • Add optimize flag as argument to get_output(), e.g. model_output <- input[["model"]]$get_output(optimize), from fimsfit.R. We need to pass the optimize argument from fit_fims() to FIMSFit()

  • In rcpp_models, get_output() in R points to to_json(). The optimize flag needs to pass from this function to the Rcpp function, get_report(). Then in get_report(), the optimize flag can be used to skip the sdreport calculation on line 700 and return empty objects for sdr_summary and mat</issue_description>

Comments on the Issue (you are @copilot in this section)

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI changed the title [WIP] Fix warning in fit_fims when optimize is false Fix: Skip sdreport calculation when optimize=FALSE to prevent NaN warning Dec 30, 2025
@codecov

codecov Bot commented Dec 30, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.67442% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 86.14%. Comparing base (eec4b13) to head (e368ed7).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...nclude/interface/rcpp/rcpp_objects/rcpp_models.hpp 97.61% 1 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##             main    #1102       +/-   ##
===========================================
+ Coverage   70.58%   86.14%   +15.56%     
===========================================
  Files          52       81       +29     
  Lines        1914     7905     +5991     
  Branches      512      517        +5     
===========================================
+ Hits         1351     6810     +5459     
- Misses        496     1087      +591     
+ Partials       67        8       -59

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Andrea-Havron-NOAA

Andrea-Havron-NOAA commented Dec 30, 2025

Copy link
Copy Markdown
Collaborator

@copilot, can you fix the following error from rcmdcheck?

* creating vignettes ... ERROR
Error: --- re-building ‘fims-demo-minimal.Rmd’ using rmarkdown
--- finished re-building ‘fims-demo-minimal.Rmd’
--- re-building ‘fims-demo.Rmd’ using rmarkdown
Quitting from fims-demo.Rmd:250-265 [fit]
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<error/rlang_error>
Error:
! Not compatible with STRSXP: [type=NULL].
---
Backtrace:
    ▆
 1. └─FIMS::fit_fims(...)
 2.   └─FIMS::FIMSFit(...) at FIMS/R/fimsfit.R:568:5
 3.     └─input[["model"]]$get_output(optimize) at FIMS/R/fimsfit.R:461:3
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Error: Error: processing vignette 'fims-demo.Rmd' failed with diagnostics:
Not compatible with STRSXP: [type=NULL].
--- failed re-building ‘fims-demo.Rmd’
--- re-building ‘fims-path-maturity.Rmd’ using rmarkdown
--- finished re-building ‘fims-path-maturity.Rmd’
SUMMARY: processing the following file failed:
  ‘fims-demo.Rmd’
Error: Error: Vignette re-building failed.
Execution halted
Error: Error in proc$get_built_file() : Build process failed
Calls: <Anonymous> ... build_package -> with_envvar -> force -> <Anonymous>
Execution halted
Error: Process completed with exit code 1.

Copilot AI commented Dec 30, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@copilot, can you fix the following error from rcmdcheck?

* creating vignettes ... ERROR

...

The error message appears to be cut off. Could you provide the complete error output from the vignette build? The snippet shows it's quitting from the fit chunk (lines 250-265) but doesn't show the actual error message.

That said, I'll investigate potential issues with the changes to ensure fit_fims(optimize = FALSE) works correctly in the vignette context.

@copilot, can you fix the following error from rcmdcheck?

* creating vignettes ... ERROR

...

Fixed in commit 1ea6de0. The issue was an uninitialized grouped_out list variable in rcpp_models.hpp that caused the vignette build to fail. The variable is now properly initialized to an empty list at declaration.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • https://api.github.com/repos/NOAA-FIMS/FIMS/actions/jobs/59172117440/logs
    • Triggering command: /usr/bin/curl curl -sL -H Accept: application/vnd.github&#43;json -H Authorization: Bearer -H X-GitHub-Api-Version: 2022-11-28 REDACTED (http block)

If you need me to access, download, or install something from one of these locations, you can either:

@Andrea-Havron-NOAA

Andrea-Havron-NOAA commented Dec 30, 2025

Copy link
Copy Markdown
Collaborator

@copilot, here is the error:
Error:
! Not compatible with STRSXP: [type=NULL].

Backtrace:
▆

  1. └─FIMS::fit_fims(...)
  2. └─FIMS::FIMSFit(...) at FIMS/R/fimsfit.R:568:5
  3. └─input[["model"]]$get_output(optimize) at FIMS/R/fimsfit.R:461:3

Copilot AI commented Dec 30, 2025

Copy link
Copy Markdown
Contributor Author

@copilot, here is the error:
Error:
! Not compatible with STRSXP: [type=NULL].
...

Fixed in commit 1eff282. The error occurred because grouped_out.names() returned NULL when called on an empty list (when optimize=FALSE). Added a size check to only access names when the list has elements.

@Andrea-Havron-NOAA

Andrea-Havron-NOAA commented Dec 30, 2025

Copy link
Copy Markdown
Collaborator

@copilot, can you work on this error?:
FAILED: [code=1] CMakeFiles/fims_doxygen /home/runner/work/FIMS/FIMS/build/CMakeFiles/fims_doxygen
cd /home/runner/work/FIMS/FIMS && /usr/local/bin/cmake -E make_directory /home/runner/work/FIMS/FIMS/build && /usr/bin/doxygen /home/runner/work/FIMS/FIMS/build/Doxyfile.fims_doxygen
/home/runner/work/FIMS/FIMS/inst/include/interface/rcpp/rcpp_objects/rcpp_models.hpp:75: error: Member to_json(bool optimize=true) (function) of class FisheryModelInterfaceBase is not documented.

Copilot AI commented Dec 30, 2025

Copy link
Copy Markdown
Contributor Author

@copilot, can you work on this error?:
FAILED: [code=1] CMakeFiles/fims_doxygen /home/runner/work/FIMS/FIMS/build/CMakeFiles/fims_doxygen
cd /home/runner/work/FIMS/FIMS && /usr/local/bin/cmake -E m...

Fixed in commit b17d988. Added Doxygen documentation for the to_json(bool optimize=true) method in the FisheryModelInterfaceBase class.

msupernaw
msupernaw requested changes Jan 5, 2026
View reviewed changes

@msupernaw msupernaw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Andrea-Havron-NOAA I think the "optimize" as a function parameter in get_report function should be something more self describing. "optimize" may give the user the impression that an optimization will be preformed on the model during the get_report call. I think a name like "do_sd_report" may be more appropriate.

@Andrea-Havron-NOAA Andrea-Havron-NOAA marked this pull request as ready for review January 8, 2026 18:00
@Andrea-Havron-NOAA Andrea-Havron-NOAA changed the title Fix: Skip sdreport calculation when optimize=FALSE to prevent NaN warning [Bug] Skip sdreport calculation when optimize=FALSE to prevent NaN warning Jan 8, 2026
@Andrea-Havron-NOAA Andrea-Havron-NOAA moved this from Backlog to In review in FIMS Code Club Jan 8, 2026
@Andrea-Havron-NOAA

Andrea-Havron-NOAA commented Jan 9, 2026

Copy link
Copy Markdown
Collaborator

@Andrea-Havron-NOAA I think the "optimize" as a function parameter in get_report function should be something more self describing. "optimize" may give the user the impression that an optimization will be preformed on the model during the get_report call. I think a name like "do_sd_report" may be more appropriate.

@msupernaw, I changed "optimize" to "do_sd_report". Any other recommended changes or is this PR ready for approval?

kellijohnson-NOAA
kellijohnson-NOAA reviewed Jan 14, 2026
View reviewed changes
Comment thread inst/include/interface/rcpp/rcpp_interface.hpp Outdated
kellijohnson-NOAA
kellijohnson-NOAA reviewed Jan 15, 2026
View reviewed changes

@kellijohnson-NOAA kellijohnson-NOAA left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Andrea-Havron-NOAA thank you for initiating this PR. I think some of the logic is overcomplicated though. I have made local changes related to all of my comments and I am testing them out before I push. The biggest thing is that FIMSFit does not need optimize because that information can be found in opt object already. Also, if we were to report optimize it would be duplicated with the information provided in max_gradient because if max_gradient = NA then the model was not optimized.

If we decide to keep the reporting for some reason, we will need to add that to the documented return of FIMSFit, which was not done.

No need for you to make any of the suggested changes as I have them all on my local machine and I will push once they pass.

Second, I also added a google test for the virtual function of get_json to ensure that when the class doesn't exist that you get the warning message. This was reported as a line that was not being tested and why this PR was decreasing our code coverage.

Comment thread inst/include/interface/rcpp/rcpp_interface.hpp Outdated
Comment thread inst/WORDLIST
Comment thread R/fimsfit.R Outdated
Comment thread R/fimsfit.R Outdated
Comment thread tests/testthat/helper-integration-tests-setup-function.R Outdated
@kellijohnson-NOAA kellijohnson-NOAA force-pushed the copilot/fix-warn-sdreport-issue branch from fd30236 to db1338d Compare January 15, 2026 18:26
@Andrea-Havron-NOAA @kellijohnson-NOAA
fix(get_output): Adds optimize parameter
e368ed7 
to skip sdreport when optimize = FALSE in fit_fims()

Co-authored-by: Andrea-Havron-NOAA <85530309+Andrea-Havron-NOAA@users.noreply.github.com>
@kellijohnson-NOAA kellijohnson-NOAA force-pushed the copilot/fix-warn-sdreport-issue branch from db1338d to e368ed7 Compare January 15, 2026 18:36
@kellijohnson-NOAA

kellijohnson-NOAA commented Jan 15, 2026

Copy link
Copy Markdown
Contributor

The google test ended up not working out b/c Rcpp was involved so I scratched it. Once the checks pass this will be merged into main and we will do a hotfix release.

@kellijohnson-NOAA kellijohnson-NOAA merged commit ec28733 into main Jan 15, 2026
17 checks passed
@github-project-automation github-project-automation Bot moved this from In review to Done in FIMS Code Club Jan 15, 2026
@kellijohnson-NOAA kellijohnson-NOAA deleted the copilot/fix-warn-sdreport-issue branch January 15, 2026 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@msupernaw msupernaw msupernaw requested changes

@kellijohnson-NOAA kellijohnson-NOAA kellijohnson-NOAA approved these changes

@Andrea-Havron-NOAA Andrea-Havron-NOAA Awaiting requested review from Andrea-Havron-NOAA

Assignees

@Andrea-Havron-NOAA Andrea-Havron-NOAA

Copilot code review Copilot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug:] fit_fims(optimize = FALSE) throws warning related to error in SE calculations

4 participants

@Andrea-Havron-NOAA @kellijohnson-NOAA @msupernaw