-
Notifications
You must be signed in to change notification settings - Fork 228
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
Biocro: Suppress "no visible binding" messages in package check output #1620
Conversation
...These ARE no-ops, right? Please check my work carefully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good to me, but I also don't know the origin of the vmax and Rd 'HAAACK' so I'd appreciate a quick check from @dlebauer before I pull this.
} | ||
if ("Sp" %in% trait.names) { | ||
trait.samples <- transform(trait.samples, Sp = udunits2::ud.convert(Sp, "kg/m2", "g/cm2")) | ||
} | ||
if ("vmax" %in% trait.names) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW the following HAAAKs don't don't do anything. They were originally introduced in ecee6fd#diff-fb09778b34d1b5614bf2c73c9b2b0591 to account for bugs in the biocro code by rescaling Rd and vmax. When the bug was resolved I removed the scaling in 4b0340b but should have just removed these lines alltogether.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So OK to merge - thanks for catching this @infotroph!
if (met[, max(relative_humidity) > 1]) { | ||
met[, `:=`(relative_humidity = relative_humidity/100)] | ||
if (met[, max(met$relative_humidity) > 1]) { | ||
met$relative_humidity = met$relative_humidity/100 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW I think 'data.table' way was faster, as would the dplyr mutate function, though this is definitely more standard and readable.
} | ||
newmet <- met[, list(year = lubridate::year(date), | ||
doy = lubridate::yday(date), | ||
hour = round(lubridate::hour(date) + lubridate::minute(date) / 60, 0), | ||
SolarR = ppfd, | ||
Temp = udunits2::ud.convert(air_temperature, "Kelvin", "Celsius"), | ||
RH = relative_humidity, | ||
Temp = udunits2::ud.convert(met$air_temperature, "Kelvin", "Celsius"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the met$
necessary? Why is it used for some variables but not others (like wind_speed and ppfd)?
Thanks for cleaning this up. The only thing I would suggest is to use consistent style - using the PS The data.table author Matt Dowle and Hadley consider this a problem with R CMD Check. Here is a discussion of the issue: https://stackoverflow.com/questions/9439256/how-can-i-handle-r-cmd-check-no-visible-binding-for-global-variable-notes-when |
Description
Fixes an annoying and potentially-problem-obscuring volume of NOTEs about "no visible binding for global variable" in the devtools::check() output. This is the last remaining item to close #1309.
The root cause of these notes is that the check doesn't know about nonstandard evaluation inside data.table calls. In #1309 I claimed the changes needed to fix these would make the code "WAY uglier", but now I don't think this is so bad.
Please check my deletion in acbaa51 -- I can't see any way these tranforms aren't no-ops, but the "HAAAACK" comments make me suspect they were there for a reason. @dlebauer do you remember what these calls were supposed to do?
Motivation and Context
Types of changes
Checklist: