search

NOAA-FIMS / FIMS

The repository for development of FIMS
https://noaa-fims.github.io/FIMS/
GNU General Public License v3.0
11 stars 7 forks source link

[Developer Issue]: `mkObj()` does not appear to be used in the code base #565

Closed kellijohnson-NOAA closed 3 weeks ago

kellijohnson-NOAA commented 1 month ago

Issue details

I am not sure if mkObj() needs to be in the R code base of FIMS? The documentation for the function says that it is just an example function and I cannot find where it is used in the code within any repository within the FIMS organization. Do we need to keep it? If yes, then the documentation (@Bai-Li-NOAA) should be updated. https://github.com/NOAA-FIMS/FIMS/blob/97281c20ba697c66cbbbf5fe4f01241f79183676/R/io.R#L1-L15

Bai-Li-NOAA commented 1 month ago

Good catch, @kellijohnson-NOAA. @Andrea-Havron-NOAA, I remember you mentioning that the function is for developers to test FIMS by calling MakeADFun() during development. Do we still need this function for testing FIMS now that we have the integration test that calls MakeADFun() in the {testthat} framework?

Andrea-Havron-NOAA commented 1 month ago

I set this up at the very beginning of FIMS development, it is no longer needed.

kellijohnson-NOAA commented 1 month ago

Thanks for the confirmation that the function is not needed. This issue is now a task of #567 and will be completed by the R interface group in m2-r-cleanup.

kellijohnson-NOAA commented 1 month ago

@MOshima-PIFSC do you want to take this on? If yes, please do development in m2-r-cleanup.

MOshima-PIFSC commented 1 month ago

@kellijohnson-NOAA I can do this 👍

MOshima-PIFSC commented 3 weeks ago

@kellijohnson-NOAA I just submitted a pull request to m2-r-cleanup with this change. When I ran devtools::check() I noticed that there was this note about dependencies in the R code. 

  Namespaces in Imports field not imported from:
    'TMB' 'scales'
    All declared Imports should be used.

Should this be something we fix now as well?

Also, the tests actually failed because the R session crashed when running test-fims-estimation.R but I don't think that was caused by the changes I made to the code base here.

kellijohnson-NOAA commented 3 weeks ago

I think that we can move TMB from Imports to Suggests in the Description file because I only see it being used in the tests now. I would not do anything with the scales package in this pull request because it has nothing to do with the code you changed.

Andrea-Havron-NOAA commented 3 weeks ago

I think that we can move TMB from Imports to Suggests in the Description file because I only see it being used in the tests now. I would not do anything with the scales package in this pull request because it has nothing to do with the code you changed.

TMB is being used to compile the model - this call is specified in the src/Makevars file, so it needs to stay in Imports. We can add @importFrom TMB MakeADFun to the R/FIMS-package.R file.