New 464 load module

[Andrea-Havron-NOAA]

What is the feature?

• Refactoring from fims <- Rcpp::Module("fims", PACKAGE = "FIMS") to Rcpp::loadModule("fims", TRUE)
• Addresses issue [Feature]: use loadModule #464

How have you implemented the solution?

• Rcpp::loadModule("fims", TRUE) has been added to zzz.R
• Rcpp classes and functions are made publically available by adding export tags to FIMS-package.R
• tests and vignettes are changed so that Rcpp modules and functions can be called directly without referencing a fims module first

Does the PR impact any other area of the project?

• Rcpp classes and functions can now be accessed directly after calling library(FIMS)

How to test this change

• This change cannot be tested in a test environment as devtools::load_all() automatically loads all Rcpp classes and functions into the R environment
• This change can be tested with the fims-demo.Rmd, which calls Rcpp classes and functions without first specifying a fims module.

Developer pre-PR checklist

• I relied on GitHub actions to 🧪 things for me while I sat on the 🛋️.

[github-actions]

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).

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (acc80b0) 74.68% compared to head (c617577) 74.92%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #526      +/-   ##
==========================================
+ Coverage   74.68%   74.92%   +0.24%     
==========================================
  Files          38       38              
  Lines        2042     2042              
  Branches      136      136              
==========================================
+ Hits         1525     1530       +5     
+ Misses        476      471       -5     
  Partials       41       41              

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

[nathanvaughan-NOAA]

I did a find in files search and found Rcpp::Module calls are still being made in test-fims-estimation.R, test-fleet-interface.R, test-recruitment-interface.R, and fims-path-maturity.Rmd which I think should probably be removed along with associated fims$ calls? This is also being used in fims_concurrent but given that those are still in development I don't know if they are worth changing at for this pull request? EDIT: went back through the commits and realized that you had reverted the fims$ module changes in test-fims-estimation.R to get the tests to pass, strangely I have 5 tests failing when I run it locally so not sure what is happening here.

[nathanvaughan-NOAA]

Calls to fims$ or Fims$ are also used in fims-path-maturity.Rmd vignette which should also be changed if this file is still in use.

[nathanvaughan-NOAA]

There are also lots of calls to fims$ throughout the rcpp files and looking over them they seem to be all in documentation so these should also be changed.

[Andrea-Havron-NOAA]

Calls to fims$ or Fims$ are also used in fims-path-maturity.Rmd vignette which should also be changed if this file is still in use.

I removed these, thanks!

[Andrea-Havron-NOAA]

@nathanvaughan-NOAA, thanks for your review!

• I kept the calls to Rcpp::Module in test-fims-estimation and fims_concurrent. I think it is trickier to call loadModule when working with different R environments.
• All other references to Rcpp::Module and fims$, Fims$ have been removed, including the vignettes
• Christine added a fix to the failing tests in test-fims-estimation in the fims-vector branch

[msupernaw]

@Andrea Havron - NOAA Federal ***@***.***> Shouldn't loadModule be called automatically in different environments when library(FIMS) is called?

…

On Mon, Nov 27, 2023 at 7:55 PM Andrea-Havron-NOAA ***@***.***> wrote: @nathanvaughan-NOAA <https://github.com/nathanvaughan-NOAA>, thanks for your review! - I kept the calls to Rcpp::Module in test-fims-estimation and fims_concurrent. I think it is trickier to call loadModule when working with different R environments. - All other references to Rcpp::Module and fims$, Fims$ have been removed, including the vignettes - Christine added a fix to the failing tests in test-fims-estimation in the fims-vector branch — Reply to this email directly, view it on GitHub <#526 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABFUSEHRYMBXEV5QX5KGLJLYGUY7TAVCNFSM6AAAAAA7QQ2EHCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRYHA4DSMRWGU> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

-- Matthew Supernaw *Scientific Software Developer* *National Oceanic and Atmospheric Administration* *Office Of Science and Technology* *NOAA Fisheries | *U.S. Department of Commerce Phone 248 - 396 - 7797

[Andrea-Havron-NOAA]

@Andrea Havron - NOAA Federal @.> Shouldn't loadModule be called automatically in different environments when library(FIMS) is called?
…
On Mon, Nov 27, 2023 at 7:55 PM Andrea-Havron-NOAA @.> wrote: @nathanvaughan-NOAA https://github.com/nathanvaughan-NOAA, thanks for your review! - I kept the calls to Rcpp::Module in test-fims-estimation and fims_concurrent. I think it is trickier to call loadModule when working with different R environments. - All other references to Rcpp::Module and fims$, Fims$ have been removed, including the vignettes - Christine added a fix to the failing tests in test-fims-estimation in the fims-vector branch — Reply to this email directly, view it on GitHub <#526 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABFUSEHRYMBXEV5QX5KGLJLYGUY7TAVCNFSM6AAAAAA7QQ2EHCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRYHA4DSMRWGU . You are receiving this because you are subscribed to this thread.Message ID: @.***>
-- Matthew Supernaw Scientific Software Developer National Oceanic and Atmospheric Administration Office Of Science and Technology *NOAA Fisheries | *U.S. Department of Commerce Phone 248 - 396 - 7797

The issue is that library(FIMS) is called in the tests/testthat.R file rather than in the individual tests. It doesn't appear that the Rcpp functions and modules are available in the new R environment when test_env <- new.env(parent = emptyenv()) is called in test-fims-estimation.R. I tried adding the following after creating test_env without success:

Rcpp::loadModule(module = "fims", what = TRUE, env = "test_env")

@Bai-Li-NOAA, any ideas?

[nathanvaughan-NOAA]

Just pushed up a couple more documentation changes to remove fims$ references. Unless Bai has a fast solution to the test environment issue I think this is good to go and we can fix that in the future.

[nathanvaughan-NOAA]

Other than the remaining test environment issue (which can be corrected in a future pull request) this looks good to go.

[Bai-Li-NOAA]

I currently don't have a quick solution for the test environment issue. Please go ahead and submit a new issue, and I can investigate it when I'm back from leave.

[nathanvaughan-NOAA]

Thanks @Bai-Li-NOAA enjoy your holiday break!