Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Release 0.3.0.0 #712

Merged
merged 1 commit into from
Jan 3, 2025
Merged

Release 0.3.0.0 #712

merged 1 commit into from
Jan 3, 2025

Conversation

kellijohnson-NOAA
Copy link
Contributor

What is the feature?

  • Version 0.3.0.0

How have you implemented the solution?

  • Fits to length data using an age-to-length-conversion matrix, data1 includes the necessary information needed to fit to both ages and lengths.
  • Adds C++ ParameterVector to allow for the estimation of time-varying parameters.
  • Implements R wrapper functions to facilitate
    • creating the input model specifications with create_default_*(), update_parameters(), and initialize_*();
    • adding -999 to the missing fleet, year, age, length, etc. combinations;
    • running the model with a user-supplied argument of n_of_loops, where the default is three, to restart the optimizer from the previous run of nlmimb;
    • summarizing the output with the FIMSFit() function and class.
  • Implements a switch for global verbosity within FIMS through the use of {cli} messages and warnings.
  • Updates the logging system complete with a vignette about how to use it, the logging system can be used for both R and C++ errors, warnings, and information.
  • Creates the initial infrastructure to implement random effects with density functions.
  • Implements a helper function to get the parameter names from the C++ code and populate the results with those names.
  • Makes lpdf_vec return 0 if data is missing.

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

  • Case studies need updated
  • Additional tasks that need to be thought about
    • Remove code for processR in tests
    • Find TODO statements in documentation and add the documentation
    • think about renaming some of the Rcpp functions
    • cli_* needs to be implemented throughout the package
    • fix(fims-demo): Does not use returned object from fit_fims for plots
    • no metadata object found to revise superClass error with sdreport class
    • Add a standalone test (CI-only) for checking the speed of parallel runs
    • fix parallel tests to avoid comparing run time with serial execution

Copy link
Contributor

github-actions bot commented Jan 1, 2025

Instructions for code reviewer

Hello reviewer, thanks for taking the time to review this PR!

  • Please use this checklist during your review, checking off items that you have verified are complete!
  • For PRs that don't make changes to code (e.g., changes to README.md or Github actions workflows), feel free to skip over items on the checklist that are not relevant. Remember it is still important to do a thorough review.
  • Then, comment on the pull request with your review indicating where you have questions or changes need to be made before merging.
  • Remember to review every line of code you’ve been asked to review, look at the context, make sure you’re improving code health, and compliment developers on good things that they do.
  • PR reviews are a great way to learn, so feel free to share your tips and tricks. However, for optional changes (i.e., not required for merging), please include nit: (for nitpicking) before making the suggestion. For example, nit: I prefer using a data.frame() instead of a matrix because...
  • Engage with the developer when they respond to comments and check off additional boxes as they become complete so the PR can be merged in when all the tasks are fulfilled. Make it clear when this has been reached by commenting on the PR with something like This PR is now ready to be merged, no changes needed.

Checklist

  • The PR is requested to be merged into the appropriate branch (typically main)
  • The code is well-designed.
  • The functionality is good for the users of the code.
  • Any User Interface changes are sensible and look good.
  • The code isn’t more complex than it needs to be.
  • Code coverage remains high, indicating the new code is tested.
  • The developer used clear names for everything.
  • Comments are clear and useful, and mostly explain why instead of what.
  • Code is appropriately documented (doxygen and roxygen).

Copy link

codecov bot commented Jan 1, 2025

Codecov Report

Attention: Patch coverage is 62.90076% with 243 lines in your changes missing coverage. Please review.

Project coverage is 65.73%. Comparing base (f0d4e76) to head (a65e05b).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
inst/include/common/def.hpp 8.33% 93 Missing and 6 partials ⚠️
inst/include/utilities/fims_json.hpp 52.94% 55 Missing and 41 partials ⚠️
...lude/population_dynamics/population/population.hpp 92.15% 0 Missing and 16 partials ⚠️
inst/include/population_dynamics/fleet/fleet.hpp 78.94% 0 Missing and 12 partials ⚠️
inst/include/common/fims_vector.hpp 75.00% 4 Missing and 6 partials ⚠️
..._dynamics/selectivity/functors/double_logistic.hpp 63.63% 4 Missing ⚠️
...population_dynamics/maturity/functors/logistic.hpp 66.66% 2 Missing ⚠️
...ulation_dynamics/selectivity/functors/logistic.hpp 75.00% 2 Missing ⚠️
...clude/population_dynamics/growth/functors/ewaa.hpp 75.00% 0 Missing and 1 partial ⚠️
...dynamics/recruitment/functors/sr_beverton_holt.hpp 50.00% 0 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (f0d4e76) and HEAD (a65e05b). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (f0d4e76) HEAD (a65e05b)
2 1
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #712       +/-   ##
===========================================
- Coverage   78.78%   65.73%   -13.06%     
===========================================
  Files          36       16       -20     
  Lines        1975      715     -1260     
  Branches      141      197       +56     
===========================================
- Hits         1556      470     -1086     
+ Misses        376      160      -216     
- Partials       43       85       +42     

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

@kellijohnson-NOAA kellijohnson-NOAA force-pushed the dev branch 2 times, most recently from 1980a6c to c3dd7ad Compare January 2, 2025 16:30
* Fits to length data using an age-to-length-conversion matrix, `data1`
  includes the necessary information needed to fit to both ages and lengths.
* Adds C++ ParameterVector to allow for the estimation of time-varying
  parameters.
* Implements R wrapper functions to facilitate
  * creating the input model specifications with `create_default_*()`,
    `update_parameters()`, and `initialize_*()`;
  * adding -999 to the missing fleet, year, age, length, etc. combinations;
  * running the model with a user-supplied argument of n_of_loops, where the
    default is three, to restart the optimizer from the previous run of nlmimb;
  * summarizing the output with the `FIMSFit()` function and class.
* Implements a switch for global verbosity within FIMS through the use
  of {cli} messages and warnings.
* Updates the logging system complete with a vignette about how to use it, the
  logging system can be used for both R and C++ errors, warnings, and
  information.
* Creates the initial infrastructure to implement random effects with density
  functions.
* Implements a helper function to get the parameter names from the
  C++ code and populate the results with those names.
* Makes lpdf_vec return 0 if data is missing.

R ----

doc(FIMSFrame): Adds documentation to accessors and increased consistency
doc(ParameterVector): Adds time-varying example to the vignette that is
  turned off by default but users can experiment
doc(fims-demo): Adds example of a model that is built but not fit
doc: Shares argument documentation across functions

feat(create_default_*()): Creates default values for FIMS parameters
feat(update_parameters()): Updates default values for FIMS parameters
feat(initialize_*()): Uses methods::new() to set up FIMS modules
feat(FIMSFit): Created FIMSFit as an S4 Class with [create_FIMSFit()]
  so people can also create a FIMSFit object without having to use
  [fit_fims()] and so the object returned by [fit_fims()] looks the
  same no matter if you want the uncertainty calculated or not via
  `get_sd`. Objects are not added to things to obj, opt, and report and
  instead they are explicitly returned as a slot in the S4 FIMSFit class
feat(estimates): In FIMSFit
feat(verbose): Uses cli_inform cli_abort. Users can now set the global
  verbose level through `options(rlib_message_verbosity = "quiet")` to stop
  cli::cli_inform() messages from coming through. This allows for the
  verbose argument to be removed from all functions. And,
  [is_fims_verbose()] can check internally in functions to see if FIMS
  is currently verbose.

fix(data1): Renamed from data_mile1 and adds length data
fix(fims-demo): Updates vignette doi Bai et al.
fix(FIMSFrame): Allows for subsetting of multiple fleets with %in%
fix(plot.FIMSFrame): Adds y and ... for plot to be properly documented
  where previously we only had x, which led to scales not being seen as a
  package that was needed. The methods proposed in #672 was a work around
  where this commit fixes the problem.
  fix(Rcpp_Parameter*): Moves to zzz.R because when in operators it was
  leading to warnings when compiling in R. The movement allows this file
  to be ran before the others and it does not matter if Rcpp_Parameter or
  Rcpp_ParameterVector is exported/defined. I also worked on the
  formatting of those functions as well as combining the documentation
  rather for similar, i.e., methods.

refactor(cli): Updates error message to use cli formatting
refactor(DESCRIPTION): Adds packages
  * Adds purrr, tidyr, tibble, stats
  * Removes tidyverse
  * Depends on R 4.1.0 because of the use of the native pipe, which is
    available in R version 4.1.0. Status-quo dependency was R 4.0.
refactor(tests): convert tests to testthat format in
  test-unit-rcpp-interface-variable-vector.R

C++ ----

doc: Remove authors, email from file headers, some cleanup to cpp file
  header with consistent use of @file, @brief, @details, and @copyright
  for every file in inst/include and notes.
doc(mainpage): Adds mainpage for doxygen as discussed by the
documentation group led by @Bai-Li-NOAA in July (notes [here](
  https://docs.google.com/document/d/1tPCSSanZ7SHaSE5SMkPoYrZh_lsswe7o5gtEyJmA0DU/edit?tab=t.0#heading=h.enmw68u5r5hm)).
doc(Parameter): Fully documents Parameter and ParameterVector matching
  documentation at the function definition with what is shown in Rcpp
  methods::show() inside R.
doc(pos): Uses pos in position for time-varying so it does not seem like
  things can only vary with time.

feat: add variable_map infrastructure
  * adds variable map as an object in information.hpp
  * adds std::vector<uint32_t> key to density_components_base
  * adds unique ID to VariableVector class and get_id function
  * resizes key and pass to densities in rcpp_distributions
feat: Uses .estimated_m and .is_random_effect_m in interface
feat(get_force_scalar): method in fims::Vector to return the first index
  if the vector is size one.
feat(get_parameter_names): Gets parameter names from C++

fix: Sets unique key for all tracked parameter/derived value in Rcpp objects
fix(information): add data error checks in information
fix(rcpp_interface): add get and set id functions to fleet interface
fix(multinomial_lpmf.hpp): make lpdf_vec to return 0 if data is missing
fix(Parameter): Removes estimated_m
fix(Makevars): Removes c++17 flag
  In favor of USE_CXX17 = "yes" and removes -w flag where this fix is
  known to work on Windows.
fix(clear): Removes everything because there was some information from
  random effects that was not being cleared leading to crashes. Many
  functions in the C++ code were also changed to CamelCase such as Clear().
  Refactored function names to match style of other member functions.
  clear() procedure creates a new instance of the information singleton object.
fix: Removes log_sigma_recruit from the Beverton--Holt
  stock--recruitment interface because it is now a part of the
  distribution associated with log_devs
fix(logging): Uses macros
  * Removes old logging
  * pretty format for output
  * added format string for json
  * created a logging vignette
  * Adds information about DFIMS_DEBUG information
  * Adds throw on error
fix: Collapses methods using S4 Group Generic Functions
fix: Defines methods for Ops, Math, and Summary
fix: Updates parameter min/max default values

refactor(model.hpp): Loops over all density modules to sum up joint
  negative log-likelihoods
refactor: Adds i=0 in for loops because R CMD check was complaining
  about significant warnings in compiling the c++ code because i was not
  initialized.
refactor(information): Improves readability
refactor: Uses C++ 17
refactor: Renames SIMULATE_F to FIMS_SIMULATE_F
refactor: Renames REPORT_F to FIMS_REPORT_F

GitHub ----

chore: change actions/checkout@v3 to v4

feat: Adds call-spell-check.yml from {ghactions4r} to spellcheck with WORDLIST

fix: Adds .json to .gitignore
fix: Adds .vscode to .gitignore
fix: Removes parallel tests because they were not helpful
fix: revert sim_num to 100 to simulate 100 sets of test data
  (see tests/testthat/fixtures/simulate-integration-test-data.R)

TODO: ----

* Remove code for processR in tests
* Find TODO statements in documentation and add the documentation
* think about renaming some of the Rcpp functions
* cli_* needs to be implemented throughout the package
* fix(fims-demo): Does not use returned object from fit_fims for plots
* no metadata object found to revise superClass error with sdreport class
* Add a standalone test (CI-only) for checking the speed of parallel runs
* fix parallel tests to avoid comparing run time with serial execution

Co-authored-by: Andrea-Havron-NOAA <[email protected]>
Co-authored-by: Bai-Li-NOAA <[email protected]>
Co-authored-by: ChristineStawitz-NOAA <[email protected]>
Co-authored-by: cmlegault <[email protected]>
Co-authored-by: Cole-Monnahan-NOAA <[email protected]>
Co-authored-by: JaneSullivan-NOAA <[email protected]>
Co-authored-by: iantaylor-NOAA <[email protected]>
Co-authored-by: k-doering-NOAA <[email protected]>
Co-authored-by: kellijohnson-NOAA <[email protected]>
Co-authored-by: KyleShertzer-NOAA <[email protected]>
Co-authored-by: msupernaw <[email protected]>
Co-authored-by: nathanvaughan-NOAA <[email protected]>
Co-authored-by: rklasky <[email protected]>

Part of #170: refactor distributions_base
Part of #388
Part of #573: add normal_lpdf
Part of #581: add lognormal_lpdf
Part of #585: add multinomial_lpmf
Part of #586
Part of #630
Part of #644
Part of #645
Part of #646
Part of #662
Part of #671
Part of #695
Part of #702
Close #672
Close #690
@kellijohnson-NOAA kellijohnson-NOAA merged commit 2338d23 into main Jan 3, 2025
15 of 16 checks passed
@kellijohnson-NOAA kellijohnson-NOAA deleted the dev branch January 3, 2025 03:39
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.

2 participants