daynefiler / tcpl

Former repository for the tcpl R package project. New repo:
https://github.com/USEPA/CompTox-ToxCast-tcpl
4 stars 5 forks source link

nchar() behavior in R > 3.2.0 causes error in plotFit() #15

Closed krball closed 7 years ago

krball commented 7 years ago

tcplPlotFits() calls plotFit() which in turn calls nchar() in numerous instances (usually inside of the internally defined function spaces()) to build the right output window of assay component and model fitting information for tcpl's various concentration series plot output functions.

When HILL and GNLS models are not fit in the processing pipeline, NA values are returned for many of their parameters. In R 3.2.0 and earlier, nchar(NA) returned 2, in versions > 3.2.0 nchar(NA) returns NA because of an effective change in the default value of keepNA.

According to nchar()'s documentation on the option keepNA: "keepNA

logical: should NA be returned where ever x is NA? If false, nchar() returns 2, as that is the number of printing characters used when strings are written to output, and nzchar() is TRUE. The default for nchar(), NA, means to use keepNA = TRUE unless type is "width". Used to be (implicitly) hard coded to FALSE in R versions <= 3.2.0."

Working example: Run tcplPlotM4ID() in R > 3.3.0 on an assay component endpoint where one or model has not been fit. The function spaces() defined in plotFit() will return an error in the rep() function because the times option in rep() is set to NA.

Solution: All instances of nchar(x) in plotFit() should be replaced with nchar(x,keepNA=FALSE).

ericwatt commented 7 years ago

This was closed as part of #2. Try installing the dev version from github using devtools: devtools::install_github("daynefiler/tcpl")

daynefiler commented 7 years ago

Agreed. We need to figure out when to push a new version to CRAN. Thoughts, @martino165?

ericwatt commented 7 years ago

If there is still work to be done on other parts before a 'big' release, you could always do a patch of just .plotFit() for a CRAN release. Depends on timing and what you want to include.

krball commented 7 years ago

Sorry guys, I didn't go through the closed issues before opening a new one.