Add data_small dataset and associated test and documentation#958
Draft
iantaylor-NOAA wants to merge 15 commits into
Draft
Add data_small dataset and associated test and documentation#958iantaylor-NOAA wants to merge 15 commits into
iantaylor-NOAA wants to merge 15 commits into
Conversation
* set verbosity to `quiet` in `tests/testthat.R` to suppress test output when running `devtools::test()` locally. * set verbosity to `quiet` in `tests/testthat/helper-aaa-quiet-test-output.R` to suppress output when running `testthat::test_package()` locally. This mirrors the "Show testthat output" (`test_check()`) step in the `call-r-cmd-check` GitHub Action workflow. * set `silent = TRUE` in `TMB::MakeADFun()` calls to suppress TMB output during tests. * use `suppressMessages()` to silence output from fims_frame tests and testthat template tests. * don't use `Rcerr` to print a message to the console when the ParameterVector index is out of range, since `throw` already handles the error output.
Bumps [actions/first-interaction](https://github.com/actions/first-interaction) from 1 to 2. - [Release notes](https://github.com/actions/first-interaction/releases) - [Commits](actions/first-interaction@v1...v2) --- updated-dependencies: - dependency-name: actions/first-interaction dependency-version: '2' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
* document(check_fims.R): Add examples of strings passed to ... in
FIMS:::setup_and_run_gtest()
* refactor(fimsfit.R):
* Remove is.FIMSFits() function
* Remove "total" from number_of_parameters; only print it
as the sum of fixed_effects + random_effects
* document(fimsfit.R): Add comment referencing sdmTMB issue #455 to
evaluate whether report should always use last.par.best
* document(initialize_modules.R): Clarify that dims field is necessary
to track dimensions for multivariate input (e.g., MVNORM) and may be
needed for other distributions
* update(data1.R): Remove check for CSV readability in R
* update(helper-integration-tests-setup-function.R): Replace 1:x with
seq_along() in for loop for safer indexing
Co-authored-by: Andrea-Havron-NOAA <Andrea-Havron-NOAA@users.noreply.github.com>
Co-authored-by: k-doering-NOAA <k-doering-NOAA@users.noreply.github.com>
Co-authored-by: kellijohnson-NOAA <kellijohnson-NOAA@users.noreply.github.com>
Co-authored-by: msupernaw <msupernaw@users.noreply.github.com>
Co-authored-by: nathanvaughan-NOAA <nathanvaughan-NOAA@users.noreply.github.com>
Co-authored-by: peterkuriyama-NOAA <peterkuriyama-NOAA@users.noreply.github.com>
* add README.md under the /tests directory as a quick reference for testing. It includes instructions on adding a new test, run tests, and debug tests. * add a C++ test template to inst/templates. * add a new R function `use_gtest_template()` to create a C++ test file using the template and register the test in `tests/gtest/CMakeLists.txt`. * add R unit tests for `use_gtest_template()`. * add a C++ test `tests/gtest/test_FIMSJson_JsonParser_WriteToFile.cpp` to verify error handling instructions outlined in the C++ test template. * update LogisticSelectivity C++ test to use the new template. * fix spelling errors and update WORDLIST. * Place `Rcpp::loadModule(module = "fims", what = TRUE)` inside the `.onLoad` function has been observed to fix R session crashes when running `devtools::load_all()` followed by `devtools::test()` in the package development workflow.
Bumps [actions/checkout](https://github.com/actions/checkout) from 4 to 5. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v4...v5) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: '5' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
- following {tidyr} example described in https://r-pkgs.org/data.html#sec-documenting-data
Contributor
Instructions for code reviewerHello reviewer, thanks for taking the time to review this PR!
Checklist
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #958 +/- ##
==========================================
- Coverage 80.26% 74.55% -5.72%
==========================================
Files 51 82 +31
Lines 1941 7982 +6041
Branches 469 308 -161
==========================================
+ Hits 1558 5951 +4393
- Misses 268 2025 +1757
+ Partials 115 6 -109 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
|
Thanks @iantaylor-NOAA for this PR. I plan on reviewing it today. |
kellijohnson-NOAA
force-pushed
the
dev
branch
2 times, most recently
from
f362d4a to
638ee30
Compare
kellijohnson-NOAA
force-pushed
the
dev
branch
from
4a24251 to
51adea8
Compare
Merged
kellijohnson-NOAA
force-pushed
the
dev
branch
3 times, most recently
from
db3ddf7 to
e0be2a1
Compare
kellijohnson-NOAA
force-pushed
the
dev
branch
from
afee2ed to
1276b24
Compare
kellijohnson-NOAA
force-pushed
the
dev
branch
2 times, most recently
from
b3d15a2 to
343c111
Compare
kellijohnson-NOAA
force-pushed
the
dev
branch
from
4c6b361 to
0b30ee2
Compare
kellijohnson-NOAA
force-pushed
the
dev
branch
from
3bf9e4f to
de9a41e
Compare
kellijohnson-NOAA
force-pushed
the
dev
branch
from
87a3229 to
e1f5808
Compare
Contributor
|
@iantaylor-NOAA thank you for this PR and sorry that I have been so slow to review it. I am going to mark it as draft and I will come back to it after the CIE Review. |
6 tasks
Contributor
Author
|
@kellijohnson-NOAA, the notifications about this PR reminded me that it's out of date relative to the current FIMS. I can commit further changes to update it. But it also looks like I should perhaps cherry pick the original commits related to data_small to separate them from other unrelated commits in August 2025. |
Contributor
|
Sorry about those emails, they came from me deleting the dev branch before changing the base branch of this PR. I think we can just leave things as they are right now and maybe we can decide on a way forward after we get the comments back from the CIE Review? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is the feature?
data_smallto the package, alongside the existingdata1, as discussed in https://github.com/orgs/NOAA-FIMS/discussions/945How have you implemented the solution?
data-raw/data_small.Rwhich generates the datasetdata/data_small.rdacreated by the script above, which runs theusethis::use_data()command at the end.tests/testthat/test-data_small.Rwhich confirms that the dataset can be converted to a FIMSFrame object and used in a model.R/data1.RtoR/data.Rand adds documentation of the second data set in the same file, following the {tidyr} example described in https://r-pkgs.org/data.html#sec-documenting-data (they could be documented in separate files, but that might make keeping them aligned harder)man/data_small.Rdcreated bydevtools::document()running on theR/data.Rfile noted above.Does the PR impact any other area of the project, maybe another repo?
data_smallin testing if it proves helpful.