refactor(create_default_*()): export modular setup helpers for modules by Bai-Li-NOAA · Pull Request #1628 · NOAA-FIMS/FIMS

Bai-Li-NOAA · 2026-07-13T01:19:08Z

What is the feature?

close issues Export create_default_*() functions to users #1578 and Fix distribution_type defaults when recruitment distribution is set to NA #1588
export create_default_*() helpers to users

How have you implemented the solution?

remove create_default_configurations() and tests
change create_default_parameters() to setup_default_parameters()
remove model_family from paramter tibble
export modular setup helpers for fleet, selectivity, data distribution, recruitment, maturity, growth, and population
update docs, pkgdown content, README examples to the new setup_default_*()
replace old snapshots/tests with expanded test coverage for setup_default_* helpers
rename integration test files to clarify with-wrappers vs without-wrappers paths
move stockplotr from Suggests to Imports and align devecontainer package setup

Does the PR impact any other area of the project, maybe another repo?

create_default_configuraitons() and create_default_parameters() were removed; users should migrate to setup_default_parameters and setup_default_*() helpers.

Instructions for code reviewer

👋Hello reviewer👋, thank you for taking the time to review this PR!

Please use this checklist during your review, checking off items that you have verified are complete but feel free to skip over items that are not relevant!
See the GitHub documentation for how to comment on a PR to indicate where you have questions or changes are needed before approving the PR.
Please use standard conventional messages for both commit messages and comments
PR reviews are a great way to learn so feel free to share your tips and tricks. However, when suggesting changes to the PR that are optional please include nit: (for nitpicking) as the comment type. For example, nit: I prefer using a data.frame() instead of a matrix because ...
Engage with the developer. Make it clear when the PR is approved by selecting the approved status, and potentially commenting on the PR with something like This PR is now ready to be merged.

Checklist

The code is well-designed
The code is designed well for both users and developers
Code coverage remains high- [ ] Comments are clear, useful, and explain why instead of what
Code is appropriately documented (doxygen and roxygen)

Bai-Li-NOAA · 2026-07-13T01:23:41Z

Copilot generated the Roxygen examples and unit tests for the modular helpers. I'll review both.

codecov · 2026-07-13T01:24:25Z

Codecov Report

❌ Patch coverage is 96.39175% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.87%. Comparing base (b7783d7) to head (e34f253).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
R/utils.R	68.18%	14 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1628      +/-   ##
==========================================
- Coverage   84.88%   84.87%   -0.01%     
==========================================
  Files         105      105              
  Lines        9473     9303     -170     
  Branches      536      539       +3     
==========================================
- Hits         8041     7896     -145     
+ Misses       1395     1370      -25     
  Partials       37       37

☔ View full report in Codecov by Harness.
📢 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.

kellijohnson-NOAA · 2026-07-16T20:29:31Z

@Bai-Li-NOAA I finished the rebase and I am working on the data/distribution side now. Looks like I will be able to use data as is because all distributional forms are currently hard coded 😮 it is just the uncertainty column that we have to pass through. So, I am going to leave them hardcoded for this PR and just remove the setup_default_data_*() functions and have them use the FIMSFrame directly in setting up the distributions within initialize_fims() because the data is already passed. This will set us up nicely for a future PR but keep this one contained. Anyway, I hope to have this for you to review tomorrow. Monday at the latest because we all know that I am a terrible estimator of time 😉, here is to trying to meet a deadline ❤️.

kellijohnson-NOAA · 2026-07-17T11:51:50Z

@Bai-Li-NOAA I just pushed, it was passing all checks locally and I ran the R formatting stuff. We might need to add some tests back if code coverage goes down because I removed some files.

* remove create_default_configurations() and tests * change create_default_parameters() to setup_default_parameters() * export modular setup helpers for fleet, selectivity, data distribution, recruitment, maturity, growth, and population * update docs, pkgdown content, README examples to the new setup_default_*() * replace old snapshots/tests with expanded test coverage for setup_default_* helpers * rename integration test files to clarify with-wrappers vs without-wrappers paths * move stockplotr from Suggests to Imports and align devecontainer package setup BREAKING CHANGE: * create_default_configuraitons() and create_default_parameters were removed; users should migrate to setup_default_parameters and setup_default_*() helpers.

Rebased and fixed some stuff in the diagnosticts Fixed not UTF-8 characters

kellijohnson-NOAA

Great work @Bai-Li-NOAA this was needed and I think users will really like it. I have added a few commits like we talked about but I also have several comments. Some of them I directly suggested code that you can commit directly through this UI if you want. There are some additional things that I would like to see changed that I did not code.

kellijohnson-NOAA · 2026-07-19T00:27:30Z

 deqn
 dlognorm
 dmultinorm
+Ddirichlet


Suggested change

Ddirichlet

I don't see Ddirichlet anywhere in the code base.

kellijohnson-NOAA · 2026-07-19T00:33:27Z

+#' @description
+#' This function generates a tibble with default parameters needed to run a
+#' FIMS model from a [FIMSFrame()] input object. The returned tibble includes
+#' default initial values and estimation settings for each configured module.
+#' You can modify the returned tibble before fitting a model (for example,
+#' updating maturity and selectivity parameter values).
+#'
+#' @details
+#' The function builds module-specific defaults by calling helper
+#' functions for data, fleet, selectivity, recruitment, maturity, growth, and
+#' population components, then combines those defaults into one tibble.


Suggested change

#' @description

#' This function generates a tibble with default parameters needed to run a

#' FIMS model from a [FIMSFrame()] input object. The returned tibble includes

#' default initial values and estimation settings for each configured module.

#' You can modify the returned tibble before fitting a model (for example,

#' updating maturity and selectivity parameter values).

#'

#' @details

#' The function builds module-specific defaults by calling helper

#' functions for data, fleet, selectivity, recruitment, maturity, growth, and

#' population components, then combines those defaults into one tibble.

#' @description

#' Initializing a FIMS model, i.e., `initialize_fims()` requires a `FIMSFrame`

#' object and a tibble of parameters. The parameter tibble can be automatically

#' generated using this function or by building up your own tibble from

#' helper functions used within this function. The resulting tibble will have

#' all of the necessary parameters, specific to your data, to run a FIMS

#' model.

#'

#' @details

#' The function builds module-specific defaults by calling helper

#' functions for data, fleet, selectivity, recruitment, maturity, growth, and

#' population components, then combines those defaults into one tibble.

#' You can modify the returned tibble before fitting a model (for example,

#' updating maturity and selectivity parameter values).

#'

#' To create the default initial numbers at age, this function uses the defaults

#' from `setup_default_Population()` and `setup_default_recruitment()`, which

#' are passed to `setup_default_init_naa()` to calculate initial numbers at age.

kellijohnson-NOAA · 2026-07-19T00:36:01Z

+#'     "fixed_effects", "random_effects").}
+#'   \item{\code{distribution_type}:}{The type of distribution (e.g., "data",
+#'     "process"), where a process distribution can refer to a fixed effect or a
+#'     random effect but it does not fit to data, e.g., recruitment deviation.}


Is the e.g. an example of what it does pertain to or what it doesn't pertain to. The sentence is confusing as written. If you want to have examples, I think you should give examples of each.

kellijohnson-NOAA · 2026-07-19T00:36:48Z

+#' @rdname setup_default_parameters
+#' @keywords setup_default_parameters
+#' @seealso
+#' * [FIMSFrame()]


Suggested change

#' * [FIMSFrame()]

#' * [FIMSFrame()]

#' * [initialize_fims()]

kellijohnson-NOAA · 2026-07-19T00:38:26Z

+setup_default_parameters <- function(
+  data
+) {


Suggested change

setup_default_parameters <- function(

data

) {

setup_default_parameters <- function(data) {

kellijohnson-NOAA · 2026-07-19T01:18:34Z

+#' @param distribution A string specifying the distribution type for the stock--recruitment
+#'   deviations. The default is `NA_character_`, which means that the stock--recruitment
+#'   deviations are not estimated.


We do not want the default to be stock--recruitment deviations that are not estimated. The default should be as random effects with a normal distribution.

kellijohnson-NOAA · 2026-07-19T02:07:27Z

+#' normal distribution, i.e., `DnormDistribution`, module.
+#' @param value A real number that is passed to `log_sd`. The default value is
+#'   `0.1`.
+#' @param data An S4 object. FIMS input data.


Same comment as others, I am going to stop commenting on this but it should be checked throughout.

kellijohnson-NOAA · 2026-07-19T02:09:10Z

@@ -0,0 +1,83 @@
+# Internal helper: validate the expected FIMSFrame input class.
+assert_is_fims_frame <- function(data) {


I think the cli comments should have been added to is.FIMSFrame() instead of creating a new function.

kellijohnson-NOAA · 2026-07-19T02:46:14Z

+assert_fleet_in_data <- function(data, fleet) {
+  fleets <- get_fleets(data)
+  if (!fleet %in% fleets) {
+    cli::cli_abort(c(
+      "i" = "{.var fleet} is not present in the {.var data}.",
+      "x" = "Available fleet names are: {.val {fleets}}."
+    ))
+  }
+}


This should be in the FIMSFrame file.

kellijohnson-NOAA · 2026-07-19T02:47:14Z

+}
+
+# Internal helper: check that a requested module type exists for a fleet.
+assert_module_type_available <- function(data, fleet, module_type) {


This function is not used, also it is confusing because you are looking for a data type not a module. If this function stays, it should move to R/fimsframe.R.

This was linked to issues Jul 13, 2026

Fix distribution_type defaults when recruitment distribution is set to NA #1588

Open

Export create_default_*() functions to users #1578

Open

Copilot started work on behalf of Bai-Li-NOAA July 13, 2026 16:25 View session

Copilot finished work on behalf of Bai-Li-NOAA July 13, 2026 16:27

kellijohnson-NOAA force-pushed the refactor-export-r-functions branch from d006625 to e550499 Compare July 17, 2026 11:50

kellijohnson-NOAA force-pushed the refactor-export-r-functions branch from 927f8d3 to 7ac63a2 Compare July 18, 2026 16:29

kellijohnson-NOAA force-pushed the main branch from 7f47765 to eb122ca Compare July 18, 2026 19:25

kellijohnson-NOAA force-pushed the refactor-export-r-functions branch 2 times, most recently from ec6dcbf to f9fcaa4 Compare July 18, 2026 19:36

kellijohnson-NOAA force-pushed the main branch from eb122ca to fd8901b Compare July 18, 2026 19:54

Bai-Li-NOAA and others added 6 commits July 18, 2026 12:56

fix devcontainer and tests

960e918

fix: add Ddirichlet to cspell wordlist

47bdbb8

Moves getting sd from parameters to data

af4a3ec

Rebased and fixed some stuff in the diagnosticts Fixed not UTF-8 characters

Updates cheat sheet

cbe4ef8

Simplify code in README to reduce pipes and increase clarity

e34f253

kellijohnson-NOAA force-pushed the refactor-export-r-functions branch from f9fcaa4 to e34f253 Compare July 18, 2026 19:57

kellijohnson-NOAA requested changes Jul 19, 2026

View reviewed changes

	#' * [FIMSFrame()]
	#' * [FIMSFrame()]
	#' * [initialize_fims()]

		@@ -0,0 +1,83 @@
		# Internal helper: validate the expected FIMSFrame input class.
		assert_is_fims_frame <- function(data) {

Uh oh!

Conversation

Bai-Li-NOAA commented Jul 13, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the feature?

How have you implemented the solution?

Does the PR impact any other area of the project, maybe another repo?

Instructions for code reviewer

Checklist

Uh oh!

Bai-Li-NOAA commented Jul 13, 2026

Uh oh!

codecov Bot commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kellijohnson-NOAA commented Jul 16, 2026

Uh oh!

kellijohnson-NOAA commented Jul 17, 2026

Uh oh!

kellijohnson-NOAA left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Bai-Li-NOAA commented Jul 13, 2026 •

edited by github-actions Bot

Loading

codecov Bot commented Jul 13, 2026 •

edited

Loading