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

[Feature]: use loadModule #464

Closed
msupernaw opened this issue Sep 27, 2023 · 16 comments
Closed

[Feature]: use loadModule #464

msupernaw opened this issue Sep 27, 2023 · 16 comments
Labels
attribute: low hanging 🍎 Easy to knock off in less than 1 day P2 mid-level priority
Milestone

Comments

@msupernaw
Copy link
Contributor

msupernaw commented Sep 27, 2023

Is your feature request related to a problem? Please describe.

If we declare loadModule(module = "fims", TRUE) in a file named R/FIMSExports.R we can simplify the model building process by not requiring the following:

fims <- Rcpp::Module("fims", PACKAGE = "FIMS")
ewaa_growth <- new(fims$EWAAgrowth)

This simplifies to:

ewaa_growth <- new(EWAAgrowth)

Describe the solution you would like.

declare loadModule(module = "fims", TRUE) in a file named R/FIMSExports.R

Describe alternatives you have considered

NA

Statistical validity, if applicable

NA

Describe if this is needed for a management application

NA

Additional context

No response

@msupernaw msupernaw added the status: triage_needed This is not approved for this milestone, do not work on it yet label Sep 27, 2023
@ChristineStawitz-NOAA
Copy link
Contributor

Thanks for this suggestion - would this mean the module is loaded into the R environment when the package is installed? If so, how much memory in the working directory would be taken up whenever FIMS is installed?

@msupernaw
Copy link
Contributor Author

msupernaw commented Sep 27, 2023 via email

@Andrea-Havron-NOAA
Copy link
Collaborator

Would fims$clear() then just become clear()?

@msupernaw
Copy link
Contributor Author

msupernaw commented Sep 28, 2023 via email

@ChristineStawitz-NOAA ChristineStawitz-NOAA added P2 mid-level priority attribute: low hanging 🍎 Easy to knock off in less than 1 day and removed status: triage_needed This is not approved for this milestone, do not work on it yet labels Oct 4, 2023
@ChristineStawitz-NOAA ChristineStawitz-NOAA added this to the MQ milestone Oct 4, 2023
@k-doering-NOAA
Copy link
Member

We made one commit, although it is still not working as intended. We may need to call the function Module() before calling loadModule() according to the Rcpp:loadModule() documentation:

When the module can be started (at namespace load time), the function Module() returns an environment with a description of the module's contents. Function loadModule() saves this as a metadata object in the package namespace. Therefore multiple calls to loadModule() are an efficient way to extract different objects from the module.

note we added the call to the loadModule() function in the R/zzz.R file, as it is a common convention for R pkgs to put load and unload functions in this file.

@Andrea-Havron
Copy link

Here is the documentation we were following.

It looks like the RcppExports.R file is autogenerated from running:

Rcpp::compileAttributes()

After running this, roxygen will need to be updated by running:
roxygen2::roxygenize(roclets="rd")

@k-doering-NOAA
Copy link
Member

Good to know about the Rcpp::compileAttributes() function!

Perhaps once we get it working, we should add the Rcpp::compileAttributes() function to our document and style workflows so it remains up to date on the main branch?

@Andrea-Havron-NOAA
Copy link
Collaborator

Looking into this more, Rcpp::compileAttributes() is used to export C++ functions with the // [[Rcpp::export]] header.

We currently export our C++ functions from rcpp_interface.hpp by calling them within the RCPP_MODULE(fims) code chunk. I wonder if we need to expose them instead using the // [[Rcpp::export]] header and then calling Rcpp::compileAttributes()

@Andrea-Havron-NOAA
Copy link
Collaborator

Andrea-Havron-NOAA commented Nov 15, 2023

RcppExports.R and RcppExports.cpp need to be generted automatically opposed to manually. I've gotten this to work by moving the inst/include/interface/rcpp_interface.hpp file to the src folder and calling Rcpp::compileAttributes(). The RcppExports.cpp file replaces the init.hpp file found in inst/include/interface.

I have made this change on a local branch but haven't pushed up to github yet as this still doesn't solve the problem of accessing functions/modules without calling fims <- Rcpp::Module("fims", PACKAGE = "FIMS"). This cannot be checked in a test environment because devtools::load_all() makes all C++ modules/functions available in R.

@msupernaw
Copy link
Contributor Author

msupernaw commented Nov 15, 2023 via email

@Andrea-Havron-NOAA
Copy link
Collaborator

Andrea-Havron-NOAA commented Nov 15, 2023

Current commit uses Rcpp::compileAttributes() to generate an RcppExport.cpp file which replaces the init.h file.

Functions/modules are exposed to R using the loadModule function in zzz.R and can be made publically available in R by adding export(name) to the NAMESPACE via add roxygen tag in R/FIMS_package.R

RcppExport.cpp and RcppExport.R are not needed to implement the solution but this is likely because of code specified in init.h. We should decide if we want to leave the code in init.h as is or autogenerate using Rcpp::compileAttributes().

To do:

  • add export Rcpp modules/function tags to FIMS_package.R
  • update FIMS_demo vignette for testing

@msupernaw
Copy link
Contributor Author

msupernaw commented Nov 15, 2023 via email

@Andrea-Havron-NOAA
Copy link
Collaborator

@msupernaw, this would be good to check. I remember reading somewhere that the CALLDEFs might not be required to specify explicity anymore, but I can't find the reference. Some R packages based on TMB don't include them, including WHAM.

@Andrea-Havron-NOAA
Copy link
Collaborator

@msupernaw, I've asked TMB developers. Adding #define TMB_LIB_INIT R_init_FIMS to the top of the FIMS.cpp file will generate the needed CALLDEFs. This is what WHAM is doing.

@Andrea-Havron-NOAA Andrea-Havron-NOAA linked a pull request Nov 16, 2023 that will close this issue
1 task
@Andrea-Havron-NOAA
Copy link
Collaborator

cherry picked changes to new branch: new-464-loadModule to avoid scary merge conflicts 😄

  • Could not call #define TMB_LIB_INIT R_init_FIMS due to error: already defined here
  • Getting error when running TMB model: getParameterOrder not available - this was defined in init.h in theTMB_CALLDEFS
  • -will try adding: TMB_CCALLABLES("FIMS") following RTMB next

@Andrea-Havron-NOAA
Copy link
Collaborator

After looking into this further, RTMB uses a MakeFile to call Rcpp::compileAttributes() and to modify the generated RcppExports.cpp file to include the TMB CALLDEFS. We would likely need to do something similar if we wanted to let Rcpp autogenerate the RcppExports.cpp file. I reverted code so that this PR only adds the loadModule function and exports Rcpp functions and classes using roxygen export tags. I will file a new issue to investigate the Rcpp::compileAttributes() method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
attribute: low hanging 🍎 Easy to knock off in less than 1 day P2 mid-level priority
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants