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

Fix "No Visible Binding for Global Variable" Warnings in lpjguess #3421

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

harshagr70
Copy link
Contributor

This PR focuses on addressing "no visible binding for global variable" warnings specifically within the lpjguess package of the Pecan codebase , these warnings were not specifically from the tidyverse calls . These issues were identified during rcmdcheck::rcmdcheck() runs, and while some changes have been implemented, further guidance is needed for unresolved errors. , fixes a part of the issue #2758

Summary of Changes:

Scoped Variables:
Passed the ZZ variable as an argument in the relevant functions to resolve scoping issues flagged by R CMD check.

Namespace Updates:
Added required imports to the NAMESPACE file as suggested by rcmdcheck.

Log File Updates:
Updated the .log file to reflect the removal of errors for the issues that were fixed in this PR.

The following warnings remain unresolved and require input from maintainers:

write.insfile.LPJGUESS:
No visible binding for global variable: lpjguess_param_list.
No visible binding for global variable: co2.1850.2020.
Undefined global functions or variables:
co2.1850.2020 and lpjguess_param_list

These warnings may need clarification on:
Whether lpjguess_param_list and co2.1850.2020 should be explicitly declared as global variables or passed as arguments.

Comment on lines 9 to 11
importFrom(utils,data)
importFrom(utils,read.table)
importFrom(utils,write.table)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! Instead of an importFrom directive let's follow PEcAn house style by finding these calls where they appear in the code and adding the utils namespace qualifier, so that write.table(df, filename,...) becomes utils::write.table(df, filename, ...) and so on. (In concept we'd like to do the same for all instances of the ncdf4::nc* functions imported below, but that can be a separate project)

But FYI we do use importFrom for cases that don't work at all in :: form (e.g. rlang's .data pronoun) or that do work but become a hassle (e.g. infix functions like %>%), so feel free to propose an importFrom where it seems like the right move.

However in those cases be aware you'll need to use Roxygen directives (e.g. #' @importFrom utils write.table in the Roxygen comments for the function that uses write.table) rather than edit NAMESPACE directly -- as written Roxygen will overwrite this change the next time it runs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the feedback! I’ll update the code to use utils::write.table and similar namespace qualifiers as per the PEcAn house style. I appreciate the clarification on using importFrom with Roxygen directives when needed—I'll keep this in mind for future contributions. I’ll make the changes and push the updates shortly. Thanks again !!

models/lpjguess/R/readStateBinary.LPJGUESS.R Outdated Show resolved Hide resolved
@@ -226,7 +226,7 @@ readStateBinary <- function(out.path, npft){


##################### Class : Patchpft #####################
getClass_Patchpft <- function(){
getClass_Patchpft <- function(zz,npft){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
getClass_Patchpft <- function(zz,npft){
getClass_Patchpft <- function(zz, npft){

@@ -380,7 +380,7 @@ getClass_Vegetation <- function(){


##################### Class : Individual #####################
getClass_Individual <- function(nind){ # nind <- Vegetation$indv
getClass_Individual <- function(nind,zz){ # nind <- Vegetation$indv
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
getClass_Individual <- function(nind,zz){ # nind <- Vegetation$indv
getClass_Individual <- function(nind, zz){ # nind <- Vegetation$indv

harshagr70 and others added 2 commits January 24, 2025 14:57
improved indentation , added spaces after commas

Co-authored-by: Chris Black <[email protected]>
@harshagr70
Copy link
Contributor Author

@infotroph i have added the changes as suggested !! and also checked for any errors by R CMD check . This fixes warnings in the lpjguess package but there are two more warnings on which i need your guidance ,
Screenshot 2025-01-24 at 11 59 34 PM
Whether lpjguess_param_list and co2.1850.2020 should be explicitly declared as global variables or any other alternative fix for this !!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants