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.

Sign up for GitHub

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

Get rid of warnings and notes in R CMD check #338

Merged
merged 5 commits into from
Jul 22, 2022
Merged

Get rid of warnings and notes in R CMD check #338

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0067297
Get rid of warnings and notes in R CMD check
IndrajeetPatil Jul 18, 2022
bff2c22
Merge branch 'develop' into 162_get_rid_of_warnings
IndrajeetPatil Jul 18, 2022
9ef0511
Merge branch 'develop' into 162_get_rid_of_warnings
IndrajeetPatil Jul 19, 2022
d0a0ad5
Merge branch 'develop' into 162_get_rid_of_warnings
IndrajeetPatil Jul 19, 2022
af9bb6c
Merge branch 'develop' into 162_get_rid_of_warnings
IndrajeetPatil Jul 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion DESCRIPTION
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ Imports:
ggplot2 (>= 3.3.0),
R6,
jsonlite,
ospsuite.utils,
ospsuite.utils (>= 1.3.0),
patchwork
Depends:
R (>= 3.6)
Expand All @@ -42,6 +42,8 @@ Roxygen: list(markdown = TRUE)
Suggests:
knitr,
rmarkdown,
scales,
shiny,
testthat (>= 3.0.3),
vdiffr (>= 1.0.0)
VignetteBuilder: knitr
Expand All @@ -61,6 +63,7 @@ Collate:
'ddiratio-datamapping.R'
'error-checks.R'
'font.R'
'global-vars.R'
'histogram-datamapping.R'
'label.R'
'messages.R'
Expand Down
17 changes: 17 additions & 0 deletions R/global-vars.R
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# defining global variables and functions to appease R CMD Check

utils::globalVariables(
names = c(
".",
"color",
"fill",
"fillLength",
"linetype",
"linetypeLength",
"newPlotObject",
"shape",
"size"
),
package = "tlf",
add = FALSE
)
Copy link
Member

Choose a reason for hiding this comment

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

I was trying to understand where those global varibales are defined - and failed.
E.g. "fillLength":

TLF-Library/R/plot-timeprofile.R

Lines 104 to 109 in 87f42c2

# fillLength defined in .parseUpdateAestheticProperty
fillValues <- .getAestheticValues(
n = fillLength,
selectionKey = plotConfiguration$ribbons$fill,
aesthetic = "fill"
)

The comment says "fillLength defined in .parseUpdateAestheticProperty" - but there is no such definition in .parseUpdateAestheticProperty - and also not elsewhere in the code.

TLF-Library/R/aaa-utilities.R

Lines 84 to 98 in 87f42c2

.parseUpdateAestheticProperty <- function(aestheticProperty, plotConfigurationProperty) {
c(
parse(text = paste0(aestheticProperty, 'Variable <- gsub("`", "", mapLabels$', aestheticProperty, ")")),
parse(text = paste0(aestheticProperty, "Length <- length(unique(mapData[, ", aestheticProperty, "Variable]))")),
# Update the property using ggplot `scale` functions
parse(text = paste0(
"suppressMessages(plotObject <- plotObject + ggplot2::scale_", aestheticProperty, "_manual(",
"values=.getAestheticValues(n=", aestheticProperty, "Length,",
"selectionKey=plotConfiguration$", plotConfigurationProperty, "$", aestheticProperty,
',aesthetic = "', aestheticProperty, '")))'
)),
# remove the legend of aesthetic if default unmapped aesthetic
parse(text = paste0("if(isIncluded(", aestheticProperty, 'Variable, "legendLabels")){plotObject <- plotObject + ggplot2::guides(', aestheticProperty, " = 'none')}"))
)
}

I am very confused

Copy link
Member Author

@IndrajeetPatil IndrajeetPatil Jul 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think that comment is outdated.

As mentioned in this NOTE:

plotTimeProfile: no visible binding for global variable 'fillLength'

This unquoted symbol or name appears in the code, but it is not defined anywhere, and so R CMD Check complaints.

This is a common problem with non-standard evaluation, and I am using the standard solution.

Copy link
Member Author

Choose a reason for hiding this comment

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

For example,

df <- data.frame(x = 1, y = 2)

df[, "x"]

dplyr::select(df, x) # no visible binding for global variable 'x'

Copy link
Member Author

Choose a reason for hiding this comment

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

Ref: https://r-pkgs.org/check.html#sec-check-checks

image

Copy link
Member

@Yuri05 Yuri05 Jul 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Well, maybe it's just my very limited understanding of R, but I just don't get how you can use undefined variable? It must be defined somewhere, or not?

Where does the value of fillLength come from in the piece of code above?

Copy link
Member

Choose a reason for hiding this comment

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

ok, thx anyway.
@pchelle Can you explain?

Copy link
Member Author

Choose a reason for hiding this comment

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

IINM, he is on vacation this week.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry for the delay.
The variable fillLength is created during the call of .parseUpdateAestheticProperty when input argument is "fill"
To prevent potential copy/paste issues of very similar lines of code, most of the functions from aaa-utilities.R directly output R code as character strings (which are then transformed into expressions to be evaluated as actual R code using parse(text) and eval(expression).

Below shows the expression returned when the function .parseUpdateAestheticProperty is called with the arguments "fill" and ribbons.

> .parseUpdateAestheticProperty("fill", "ribbons")

expression(
fillVariable <- gsub("`", "", mapLabels$fill),
fillLength <- length(unique(mapData[, fillVariable])),
suppressMessages(
plotObject <- plotObject + ggplot2::scale_fill_manual(
values = .getAestheticValues(n = fillLength, selectionKey = plotConfiguration$ribbons$fill, aesthetic = "fill")
)
), 
if (isIncluded(fillVariable, "legendLabels")) {
plotObject <- plotObject + ggplot2::guides(fill = "none")
}
)

When the expression is wrapped by eval(), this code is evaluated and

  • Creates the variable fillVariable, column name in the data linked to the fill property
  • Creates the variable fillLength, number of unique values for the fill property (used for selecting the right number of colors)
  • Update the fill properties of the plot plotObject
  • Remove the legend if no mapping was actually defined

Note that changing "fill" by any other property name such as "color" or "alpha" would directly create an evaluated R code with all the property names changed to "color" or "alpha"

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the warning in R-CMD-Check is from the function plotTimeProfile which needs to manage between the aesthetic properties of errorbars, scatter points, ribbons and lines while potentially sharing the color aesthetic property.

The plotTimeProfile code was leveraging the creation of the fillLength property created by a previous call of .parseUpdateAestheticProperty to merge the fill property of both simulated and observed data.

Since there is no actual written code of fillLength <- length(unique(mapData[, fillVariable])) (because created and run from the expression), the check flags the variable fillLength as not being created before its use.

Copy link
Member

Choose a reason for hiding this comment

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

ok, now I at least understand how it works :)
I have further questions/thoughts regarding this, but I will create separate issues for that.
Accepting the PR for now.

1 change: 1 addition & 0 deletions man/ObsVsPredDataMapping.Rd
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions man/PKRatioDataMapping.Rd
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.