-
Notifications
You must be signed in to change notification settings - Fork 8
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
Comments
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? |
@christine Stawitz - NOAA Federal ***@***.***> Yeah, the
module gets loaded when the package is called. It uses the same amount of
memory, it just negates the need for those annoying s4 'fims$' calls every
time new is called for an object.
…On Wed, Sep 27, 2023 at 5:40 PM Christine Stawitz - NOAA < ***@***.***> wrote:
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?
—
Reply to this email directly, view it on GitHub
<#464 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABFUSEBGKKGPBUHEO336QNTX4SMLVANCNFSM6AAAAAA5JNBDFQ>
.
You are receiving this because you authored the 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
|
Would |
Yes, it does. Or FIMS::clear().
…On Thu, Sep 28, 2023 at 2:47 PM Andrea-Havron-NOAA ***@***.***> wrote:
Would fims$clear() then just become clear()?
—
Reply to this email directly, view it on GitHub
<#464 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABFUSECTT2F3MQ2HNZ4QH5LX4XA4RANCNFSM6AAAAAA5JNBDFQ>
.
You are receiving this because you authored the 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
|
We made one commit, although it is still not working as intended. We may need to call the function
note we added the call to the |
Here is the documentation we were following. It looks like the RcppExports.R file is autogenerated from running:
After running this, roxygen will need to be updated by running: |
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? |
Looking into this more, 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 |
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 |
I'm not sure yet why the loadModule isn't working. This works just fine in
other packages, so it may be an issue with the NAMESPACE file...I'm looking
into this.
…On Wed, Nov 15, 2023 at 12:45 PM Andrea-Havron-NOAA < ***@***.***> wrote:
RcppExports.R and RcppExports.cpp need to be generted automatically
opposed to manually. I've gotten this to work by moveing 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.
—
Reply to this email directly, view it on GitHub
<#464 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABFUSEDU7CIT4Q7Q62WVJUTYET5RXAVCNFSM6AAAAAA5JNBDFSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJSHE4DKOBSGA>
.
You are receiving this because you authored the 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
|
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:
|
Does replacing the init.h file cause problems with TMB? There are several
callbacks that are defined under TMB_CALLDEFS.
…On Wed, Nov 15, 2023 at 3:56 PM Andrea-Havron-NOAA ***@***.***> wrote:
Current commit uses Rcpp::compileAttributes() to generate an
RcppExport.cpp file which replaces the init.h file.
Functions/modules can be exposed to 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.
To do:
- add export Rcpp modules/function tags to FIMS_package.R
- update FIMS_demo vignette for testing
—
Reply to this email directly, view it on GitHub
<#464 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABFUSEC7KMDQ2IAIIFS7A23YEUT5TAVCNFSM6AAAAAA5JNBDFSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJTGI2DSMBWGU>
.
You are receiving this because you authored the 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
|
@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. |
@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. |
cherry picked changes to new branch: new-464-loadModule to avoid scary merge conflicts 😄 |
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. |
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
The text was updated successfully, but these errors were encountered: