R cmd notes #692

kellijohnson-NOAA · 2024-12-05T20:58:43Z

What is the feature?

Changes to reduce number of warnings and notes from R CMD check from 3 and 4 to 1 and 2, respectively

How have you implemented the solution?

use x, y, ... instead of just x even though we only need x arguments to the method of plot() for FIMSFrame to match plot.
Moves setMethods() out of operators.R to zzz.R to eliminate warnings that are found in R CMD check
Utilize @Rdname to combine documentation
Format calls to methods::setMethods()
adds i = 0 for rccp stuff to initialize
changes makevars to remove warnings from R on windows when compiling

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

Will close PR #672 because changing the call to plot is more robust than exporting scales.

Big thanks to @arni-magnusson for mentioning how important it is to keep these notes and warnings to a minimum so new contributors are not overloaded with warnings when trying to compile and can see if their changes are making a difference.

@Andrea-Havron-NOAA can you give this a quick glance?

Close #671

For plot to be properly documented and rendered all arguments must be present in the methods, previously we only had x, which led to scales not being seen as a package that was needed. Now that y and ... have been added the package is compiling without the error noted by Part of #671 Close the PR #672, which is no longer needed with this commit. The methods proposed in #672 was a work around where this commit fixes the problem.

github-actions · 2024-12-05T20:58:54Z

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

Andrea-Havron-NOAA · 2024-12-05T21:10:16Z

DESCRIPTION

@@ -79,6 +79,7 @@ Suggests:
    rmarkdown,
    snowfall,
    testthat (>= 3.0.0),
+    tidyr,
    tidyverse,


tidyverse maintainers have asked R package developers to not add tidyverse to Imports, Depends, or Suggests in an R package (see here). The preferred approach is to list specific packages used within the tidyverse instead.

Thanks @Andrea-Havron-NOAA for noticing this, I will remove tidyverse.

Thanks again @Andrea-Havron-NOAA I also noticed that we needed {purrr} so I added that. And, I added a discussion about {processR} which is used in the tests but that does not need to be decided today so I am going to go ahead and merge in this PR.

Andrea-Havron-NOAA · 2024-12-05T21:43:44Z

src/Makevars

 CXX17STD = -std=c++17 -w
-
+USE_CXX17 = "yes"


does this line need to be added to Makevars.win?

The R CMD check for windows, linux, and mac currently states

* checking C++ specification ... OK Not all R platforms support C++17

with the current configuration so I believe the files are fine as is.

to remove the R CMD check WARNING that * checking for unstated dependencies in 'tests' ... WARNING Warning: '::' or ':::' import not declared from: 'tidyr' Removes tidyverse and adds purrr. Started a Discussion about processR which is no longer on CRAN. Part of #671

To fix the WARNING of get_expected_name: missing arguments not allowed in calls to 'c'

To remove the "no visible binding for global variable" warning Part of #671

In favor of USE_CXX17 = "yes" and removes -w flag where this fix is known to work on Windows.

m_weight_at_age and Rcpp_ParameterVector needed help on the documentation, where none of the methods were documented for the latter. They still are not exported, just the m_* functions are. Moves Rcpp_Parameter* 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. Part of #671

R CMD check was complaining about significant warnings in compiling the c++ code because i was not initialized. Close #690

kellijohnson-NOAA · 2024-12-05T23:15:02Z

Thank you @Andrea-Havron-NOAA for the quick review, super helpful as I am trying to get each new branch merged in daily for these small trivial changes so people can branch off of dev daily without coming into merge conflicts at the end of the day.

[Edit]: We still have

Warning: Undocumented code objects:

and

* checking compiled code ... NOTE
File ‘FIMS/libs/FIMS.so’:
  Found ‘_ZSt4cerr’, possibly from ‘std::cerr’ (C++)
    Object: ‘FIMS.o’
  Found ‘_ZSt4cout’, possibly from ‘std::cout’ (C++)
    Object: ‘FIMS.o’

where the latter is going to be fixed by Matthew by not using std::cout in the rcpp files. And the former can be fixed just has not been yet.
This last one may not be able to be fixed

* checking installed package size ... NOTE
  installed size is 62.6Mb
  sub-directories of 1Mb or more:
    R      1.1Mb
    doc    3.5Mb
    libs  57.3Mb

kellijohnson-NOAA requested a review from Andrea-Havron-NOAA December 5, 2024 20:58

Andrea-Havron-NOAA reviewed Dec 5, 2024

View reviewed changes

Andrea-Havron-NOAA approved these changes Dec 5, 2024

View reviewed changes

kellijohnson-NOAA added 6 commits December 5, 2024 15:03

fix(get_expected_name): Removes trailing comma

3ad5e29

To fix the WARNING of get_expected_name: missing arguments not allowed in calls to 'c'

chore: Adds utils::globalVariables

e3de2b2

To remove the "no visible binding for global variable" warning Part of #671

fix(Makevars): Removes c++17 flag

6cf10c3

In favor of USE_CXX17 = "yes" and removes -w flag where this fix is known to work on Windows.

chore: Adds i=0 in for loops

5def7e4

R CMD check was complaining about significant warnings in compiling the c++ code because i was not initialized. Close #690

kellijohnson-NOAA force-pushed the r-cmd-notes branch from 9738bc1 to 5def7e4 Compare December 5, 2024 23:11

kellijohnson-NOAA merged commit 4354ccd into dev Dec 5, 2024
10 checks passed

kellijohnson-NOAA deleted the r-cmd-notes branch December 5, 2024 23:27

kellijohnson-NOAA mentioned this pull request Dec 5, 2024

[feature:] Warnings and Notes from R CMD check #671

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R cmd notes #692

R cmd notes #692

kellijohnson-NOAA commented Dec 5, 2024

github-actions bot commented Dec 5, 2024

Andrea-Havron-NOAA Dec 5, 2024

kellijohnson-NOAA Dec 5, 2024

kellijohnson-NOAA Dec 5, 2024

Andrea-Havron-NOAA Dec 5, 2024

kellijohnson-NOAA Dec 5, 2024

kellijohnson-NOAA commented Dec 5, 2024 •

edited

Loading

R cmd notes #692

R cmd notes #692

Conversation

kellijohnson-NOAA commented Dec 5, 2024

What is the feature?

How have you implemented the solution?

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

github-actions bot commented Dec 5, 2024

Instructions for code reviewer

Checklist

Andrea-Havron-NOAA Dec 5, 2024

Choose a reason for hiding this comment

kellijohnson-NOAA Dec 5, 2024

Choose a reason for hiding this comment

kellijohnson-NOAA Dec 5, 2024

Choose a reason for hiding this comment

Andrea-Havron-NOAA Dec 5, 2024

Choose a reason for hiding this comment

kellijohnson-NOAA Dec 5, 2024

Choose a reason for hiding this comment

kellijohnson-NOAA commented Dec 5, 2024 • edited Loading

kellijohnson-NOAA commented Dec 5, 2024 •

edited

Loading