Skip to content

Uses distribution in data to set up uncertainty - #1678

Open
kellijohnson-NOAA wants to merge 3 commits into
refactor-export-r-functionsfrom
refactor-data-uncertainty
Open

Uses distribution in data to set up uncertainty#1678
kellijohnson-NOAA wants to merge 3 commits into
refactor-export-r-functionsfrom
refactor-data-uncertainty

Conversation

@kellijohnson-NOAA

@kellijohnson-NOAA kellijohnson-NOAA commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

What is the feature?

  • No longer passes the uncertainty to parameters. The distributional assumption now lives in the data.

How have you implemented the solution?

  • A formula system was created for the "uncertainty" column of FIMSFrame@data. The column takes the form of "~(<central_tendency_label> = <derived_quantity>, <uncertainty_parameter_label> = <user_input>)". For example, "~lognormal(mean = log_landings_expected, sd = 0.001)" would be a viable entry for landings data.
  • parse_data_distribution() parses this out into a list which is then used to set up the data distributions among other things

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

  • I deleted quite a few tests so we will need to check coverage
  • This PR and the parent branch are behind main by a commit and thus still failing R CMD check for linux.

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)

Comment thread data-raw/fims_distributions.R
Comment thread vignettes/fims-demo-projections.Rmd Outdated
Comment thread R/distribution_formulas.R Outdated
list("parameters", "sd"),
.default = NA_real_
)
new_module$log_sd[] <- log(sd_values)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we need to add checks here to ensure sd_values are valid? R won't throw an error if .default = NA_real_. Do we need to check some potential edge cases as well, such as sd_value being 0 or negative?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Great suggestion @Bai-Li-NOAA . I added one to check that sdlog and sd are always positive values. Are there any other checks that you can think of?

Comment thread R/parse_data_distribution.R Outdated
Comment thread R/parse_data_distribution.R Outdated
Comment thread R/parse_data_distribution.R Outdated
Comment thread R/parse_data_distribution.R Outdated
Comment thread R/parse_data_distribution.R
Comment thread data-raw/fims_distributions.R Outdated
Comment thread tests/testthat/test-distribution-formulas.R
Comment thread data-raw/data_big.R Outdated

@Bai-Li-NOAA Bai-Li-NOAA left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think I ended up leaving more non-blocking suggestions that could become follow-up issues. 😉 Let me know if you have any questions.

Bai-Li-NOAA and others added 3 commits August 7, 2026 08:27
No longer passes the uncertainty to parameters. The distributional
assumption now lives in the data.
* increment version and add NEWS for both PRs
@kellijohnson-NOAA
kellijohnson-NOAA force-pushed the refactor-data-uncertainty branch from 4ce1dde to bfd9a0b Compare August 7, 2026 15:28
@kellijohnson-NOAA

Copy link
Copy Markdown
Contributor Author

@Bai-Li-NOAA I did quite a bit of work to this branch since your review. I think that we should merge it into your branch as a single commit and then merge the whole thing into main. I am going to open a PR in r4ss/r4ss and NOAA-FIMS/case-studies to update both of the repos to use this new version and I would like to get approval of those two PRs before we merge this stuff into main. That way people are reviewing the user experience rather than the logic. What do you think?

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

Successfully merging this pull request may close these issues.

[Refactor]: Centralize hardcoded data-type name strings into a package-level object

2 participants