diff --git a/NAMESPACE b/NAMESPACE index 639ef73f2..a3797a01b 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -197,6 +197,7 @@ export(.copyUi) export(.expandPars) export(.getLastIdLvl) export(.handleSingleErrTypeNormOrTFoceiBase) +export(.iniGetAppendArg) export(.iniHandleFixOrUnfix) export(.iniHandleLine) export(.malert) diff --git a/NEWS.md b/NEWS.md index f1e0fcde2..2b6947a23 100644 --- a/NEWS.md +++ b/NEWS.md @@ -150,6 +150,20 @@ mu-referencing style to run the optimization. be kept carrying more information with it (for example ordered factors, data frame columns with unit information, etc) +- Piping arguments `append` for `ini()` and `model()` have been + aligned to perform similarly. Therefore `ini(append=)` now can take + expressions instead of simply strings and `model(append=)` can also + take strings. Also model piping now can specify the integer line + number to be modified just like the `ini()` could. Also + `model(append=FALSE)` has been changed to `model(append=NULL)`. + While the behavior is the same when you don't specify the argument, + the behavior has changed to align with `ini()` when piping. Hence + `model(append=TRUE)` will append and `model(append=FALSE)` will now + pre-pend to the model. `model(append=NULL)` will modify lines like + the behavior of `ini(append=NULL)`. The default of `model(line)` + modifying a line in-place still applies. While this is a breaking + change, most code will perform the same. + ## Internal new features - Add `as.model()` for list expressions, which implies `model(ui) <- diff --git a/R/piping-ini.R b/R/piping-ini.R index b923908b8..ffa89fe51 100644 --- a/R/piping-ini.R +++ b/R/piping-ini.R @@ -331,7 +331,8 @@ if (length(.w) != 1) { stop("cannot find parameter '", lhs, "'", call.=FALSE) } else if (!is.character(newLabel) || !(length(newLabel) == 1)) { - stop("the new label for '", lhs, "' must be a character string") + stop("the new label for '", lhs, "' must be a character string", + call.=FALSE) } ini$label[.w] <- newLabel assign("iniDf", ini, envir=rxui) @@ -369,7 +370,8 @@ checkmate::assert_choice(append, choices = ini$name) appendClean <- which(ini$name == append) } else { - stop("'append' must be NULL, logical, numeric, or character", call. = FALSE) + stop("'append' must be NULL, logical, numeric, or character/expression of variable in model", + call. = FALSE) } lhs <- as.character(expr[[2]]) @@ -667,16 +669,59 @@ } expr } +#' This gets the append arg for the ini({}) piping +#' +#' @param f this is the `try(force(append))` argument, +#' @param s this is the `as.character(substitute(append))` argument +#' @return corrected ini piping argument +#' +#' This is exported for creating new ini methods that have the same +#' requirements for piping +#' +#' @export +#' @author Matthew L. Fidler +#' @keywords internal +.iniGetAppendArg <- function(f, s) { + if (inherits(f, "try-error") && + checkmate::testCharacter(s, len=1, any.missing=FALSE, + pattern="^[.]*[a-zA-Z]+[a-zA-Z0-9._]*$", + min.chars = 1)) { + return(s) + } + if (is.null(f)) { + return(NULL) + } else if (checkmate::testCharacter(f, len=1, any.missing=FALSE, + pattern="^[.]*[a-zA-Z]+[a-zA-Z0-9._]*$", + min.chars = 1)) { + return(f) + } else if (is.infinite(f)) { + return(f) + } else if (checkmate::testIntegerish(f, len=1, any.missing=FALSE)) { + if (f < 0) { + stop("'append' cannot be a negative integer", call.=FALSE) + } + return(f) + } else if (checkmate::testLogical(f, len=1)) { + # NA for model piping prepends + if (is.na(f)) return(FALSE) + return(f) + } + stop("'append' must be NULL, logical, numeric, or character/expression of variable in model", + call.=FALSE) +} #' @export #' @rdname ini ini.rxUi <- function(x, ..., envir=parent.frame(), append = NULL) { + .s <- as.character(substitute(append)) + .f <- try(force(append), silent=TRUE) + append <- .iniGetAppendArg(.f, .s) .ret <- rxUiDecompress(.copyUi(x)) # copy so (as expected) old UI isn't affected by the call .iniDf <- .ret$iniDf .iniLines <- .quoteCallInfoLines(match.call(expand.dots = TRUE)[-(1:2)], envir=envir, iniDf= .iniDf) if (length(.iniLines) == 0L) return(.ret$iniFun) lapply(.iniLines, function(line) { - .iniHandleLine(expr = line, rxui = .ret, envir = envir, append = append) + .iniHandleLine(expr = line, rxui = .ret, envir = envir, append=append) }) if (inherits(x, "rxUi")) { .x <- rxUiDecompress(x) @@ -695,6 +740,9 @@ ini.rxUi <- function(x, ..., envir=parent.frame(), append = NULL) { #' @rdname ini #' @export ini.default <- function(x, ..., envir=parent.frame(), append = NULL) { + .s <- as.character(substitute(append)) + .f <- try(force(append), silent=TRUE) + append <- .iniGetAppendArg(.f, .s) .ret <- try(as.rxUi(x), silent = TRUE) if (inherits(.ret, "try-error")) { stop("cannot figure out what to do with the ini({}) function", call.=FALSE) diff --git a/R/piping-model.R b/R/piping-model.R index df4f62812..c6633755f 100644 --- a/R/piping-model.R +++ b/R/piping-model.R @@ -1,6 +1,6 @@ #' @export #' @rdname model -model.function <- function(x, ..., append=FALSE, auto=getOption("rxode2.autoVarPiping", TRUE), +model.function <- function(x, ..., append=NULL, auto=getOption("rxode2.autoVarPiping", TRUE), cov=NULL, envir=parent.frame()) { .modelLines <- .quoteCallInfoLines(match.call(expand.dots = TRUE)[-(1:2)], envir=envir) .ret <- rxUiDecompress(rxode2(x)) @@ -11,7 +11,7 @@ model.function <- function(x, ..., append=FALSE, auto=getOption("rxode2.autoVarP #' @export #' @rdname model -model.rxUi <- function(x, ..., append=FALSE, auto=getOption("rxode2.autoVarPiping", TRUE), +model.rxUi <- function(x, ..., append=NULL, auto=getOption("rxode2.autoVarPiping", TRUE), cov=NULL, envir=parent.frame()) { .modelLines <- .quoteCallInfoLines(match.call(expand.dots = TRUE)[-(1:2)], envir=envir) .ret <- rxUiDecompress(.copyUi(x)) # copy so (as expected) old UI isn't affected by the call @@ -32,7 +32,7 @@ model.rxUi <- function(x, ..., append=FALSE, auto=getOption("rxode2.autoVarPipin #' @export #' @rdname model -model.rxode2 <- function(x, ..., append=FALSE, auto=getOption("rxode2.autoVarPiping", TRUE), +model.rxode2 <- function(x, ..., append=NULL, auto=getOption("rxode2.autoVarPiping", TRUE), cov=NULL, envir=parent.frame()) { .modelLines <- .quoteCallInfoLines(match.call(expand.dots = TRUE)[-(1:2)], envir=envir) x <- as.function(x) @@ -56,7 +56,7 @@ model.rxModelVars <- model.rxode2 #' @return New UI #' @author Matthew L. Fidler #' @export -.modelHandleModelLines <- function(modelLines, rxui, modifyIni=FALSE, append=FALSE, +.modelHandleModelLines <- function(modelLines, rxui, modifyIni=FALSE, append=NULL, auto=getOption("rxode2.autoVarPiping", TRUE), cov=NULL, envir) { checkmate::assertLogical(modifyIni, any.missing=FALSE, len=1) @@ -66,8 +66,34 @@ model.rxModelVars <- model.rxode2 .varSelect$cov <- cov .doAppend <- FALSE rxui <- rxUiDecompress(rxui) + .ll <- length(rxui$lstExpr) if (!is.null(.nsEnv$.quoteCallInfoLinesAppend)) { - .ll <- length(rxui$lstExpr) + if (identical(.nsEnv$.quoteCallInfoLinesAppend, quote(Inf))) { + .nsEnv$.quoteCallInfoLinesAppend <- NULL + append <- TRUE + } else if (identical(.nsEnv$.quoteCallInfoLinesAppend, quote(-Inf))) { + .nsEnv$.quoteCallInfoLinesAppend <- NULL + append <- NA + } else if (identical(.nsEnv$.quoteCallInfoLinesAppend, quote(0))) { + .nsEnv$.quoteCallInfoLinesAppend <- NULL + append <- NA + } else if (checkmate::testIntegerish(.nsEnv$.quoteCallInfoLinesAppend, lower=.ll)) { + .nsEnv$.quoteCallInfoLinesAppend <- NULL + append <- TRUE + } + } + if (!is.null(.nsEnv$.quoteCallInfoLinesAppend)) { + if (checkmate::testIntegerish(.nsEnv$.quoteCallInfoLinesAppend, lower=0, upper=.ll)) { + .nsEnv$.quoteCallInfoLinesAppend <- .getLhs(rxui$lstExpr[[.nsEnv$.quoteCallInfoLinesAppend]]) + } else if (checkmate::testCharacter(.nsEnv$.quoteCallInfoLinesAppend, len=1, any.missing=FALSE, + min.chars = 1)) { + .tmp <- try(str2lang(.nsEnv$.quoteCallInfoLinesAppend), silent=TRUE) + if (inherits(.tmp, "try-error")) { + stop("'append' must refer to a LHS model line when a character", + call. = FALSE) + } + .nsEnv$.quoteCallInfoLinesAppend <- .tmp + } .w <- which(vapply(seq_len(.ll), function(i) { .lhs <- .getLhs(rxui$lstExpr[[i]]) @@ -112,7 +138,7 @@ model.rxModelVars <- model.rxode2 envir=rxui) } .doAppend <- TRUE - } else if (is.logical(append) && length(append) == 1L && is.na(append)) { + } else if (is.logical(append) && length(append) == 1L && (is.na(append) || !append)) { assign("lstExpr", c(modelLines, rxui$lstExpr), envir=rxui) .doAppend <- TRUE } else if (isTRUE(append)) { diff --git a/R/piping.R b/R/piping.R index 93da808b6..b75984aeb 100644 --- a/R/piping.R +++ b/R/piping.R @@ -1,4 +1,4 @@ -#' This copies the rxode2 UI object so it can be modified +#' This copies the rxode2 UI object so it can be modified #' #' @param ui Original UI object #' @return Copied UI object diff --git a/data/rxReservedKeywords.rda b/data/rxReservedKeywords.rda index 310c21c71..80fc13cd1 100644 Binary files a/data/rxReservedKeywords.rda and b/data/rxReservedKeywords.rda differ diff --git a/data/rxResidualError.rda b/data/rxResidualError.rda index 59ed83c8c..df63ee3a5 100644 Binary files a/data/rxResidualError.rda and b/data/rxResidualError.rda differ diff --git a/data/rxSyntaxFunctions.rda b/data/rxSyntaxFunctions.rda index 6adecc8a4..bb46aebcc 100644 Binary files a/data/rxSyntaxFunctions.rda and b/data/rxSyntaxFunctions.rda differ diff --git a/man/dot-iniGetAppendArg.Rd b/man/dot-iniGetAppendArg.Rd new file mode 100644 index 000000000..14d2fd49c --- /dev/null +++ b/man/dot-iniGetAppendArg.Rd @@ -0,0 +1,26 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/piping-ini.R +\name{.iniGetAppendArg} +\alias{.iniGetAppendArg} +\title{This gets the append arg for the ini({}) piping} +\usage{ +.iniGetAppendArg(f, s) +} +\arguments{ +\item{f}{this is the \code{try(force(append))} argument,} + +\item{s}{this is the \code{as.character(substitute(append))} argument} +} +\value{ +corrected ini piping argument + +This is exported for creating new ini methods that have the same +requirements for piping +} +\description{ +This gets the append arg for the ini({}) piping +} +\author{ +Matthew L. Fidler +} +\keyword{internal} diff --git a/man/dot-modelHandleModelLines.Rd b/man/dot-modelHandleModelLines.Rd index e9cb9b76b..a60da6ef4 100644 --- a/man/dot-modelHandleModelLines.Rd +++ b/man/dot-modelHandleModelLines.Rd @@ -8,7 +8,7 @@ modelLines, rxui, modifyIni = FALSE, - append = FALSE, + append = NULL, auto = getOption("rxode2.autoVarPiping", TRUE), cov = NULL, envir diff --git a/man/model.Rd b/man/model.Rd index 02f0ca563..bc081d0a9 100644 --- a/man/model.Rd +++ b/man/model.Rd @@ -12,7 +12,7 @@ \method{model}{`function`}( x, ..., - append = FALSE, + append = NULL, auto = getOption("rxode2.autoVarPiping", TRUE), cov = NULL, envir = parent.frame() @@ -21,7 +21,7 @@ \method{model}{rxUi}( x, ..., - append = FALSE, + append = NULL, auto = getOption("rxode2.autoVarPiping", TRUE), cov = NULL, envir = parent.frame() @@ -30,7 +30,7 @@ \method{model}{rxode2}( x, ..., - append = FALSE, + append = NULL, auto = getOption("rxode2.autoVarPiping", TRUE), cov = NULL, envir = parent.frame() @@ -39,7 +39,7 @@ \method{model}{rxModelVars}( x, ..., - append = FALSE, + append = NULL, auto = getOption("rxode2.autoVarPiping", TRUE), cov = NULL, envir = parent.frame() diff --git a/man/reexports.Rd b/man/reexports.Rd index 365c14ab7..f1310e9e9 100644 --- a/man/reexports.Rd +++ b/man/reexports.Rd @@ -82,4 +82,3 @@ below to see their documentation. \item{rxode2random}{\code{\link[rxode2random:dot-cbindOme]{.cbindOme}}, \code{\link[rxode2random:dot-expandPars]{.expandPars}}, \code{\link[rxode2random:dot-vecDf]{.vecDf}}, \code{\link[rxode2random]{cvPost}}, \code{\link[rxode2random]{invWR1d}}, \code{\link[rxode2random]{phi}}, \code{\link[rxode2random]{rinvchisq}}, \code{\link[rxode2random]{rLKJ1}}, \code{\link[rxode2random]{rxGetSeed}}, \code{\link[rxode2random]{rxGetSeed}}, \code{\link[rxode2random]{rxRmvn}}, \code{\link[rxode2random]{rxSeedEng}}, \code{\link[rxode2random]{rxSetSeed}}, \code{\link[rxode2random]{rxSetSeed}}, \code{\link[rxode2random]{rxSetSeed}}, \code{\link[rxode2random:rxWithSeed]{rxWithPreserveSeed}}, \code{\link[rxode2random]{rxWithSeed}}, \code{\link[rxode2random]{rxWithSeed}}} }} -\value{ Inherited from parent routine } diff --git a/man/rxode2.Rd b/man/rxode2.Rd index 415c922b4..43b14e371 100644 --- a/man/rxode2.Rd +++ b/man/rxode2.Rd @@ -325,7 +325,7 @@ compilation model. \if{html}{\out{
}}\preformatted{mod$simulationModel }\if{html}{\out{
}} -\if{html}{\out{
}}\preformatted{## rxode2 2.0.14.9000 model named rx_9140ece6c151a5d4341598adc0f7f3b6 model (ready). +\if{html}{\out{
}}\preformatted{## rxode2 2.0.14.9000 model named rx_85848c9248e14e8cbf9a9b4a606b2010 model (ready). ## x$state: depot, center ## x$stateExtra: cp ## x$params: tka, tcl, tv, add.sd, eta.ka, eta.cl, eta.v, rxerr.cp @@ -336,7 +336,7 @@ compilation model. mod$simulationIniModel }\if{html}{\out{
}} -\if{html}{\out{
}}\preformatted{## rxode2 2.0.14.9000 model named rx_3696701c79e711bcf4b2c8ac921b3f65 model (ready). +\if{html}{\out{
}}\preformatted{## rxode2 2.0.14.9000 model named rx_40b4a6f44b577c0e14f674ccc10f618e model (ready). ## x$state: depot, center ## x$stateExtra: cp ## x$params: tka, tcl, tv, add.sd, eta.ka, eta.cl, eta.v, rxerr.cp diff --git a/tests/testthat/test-piping-ini.R b/tests/testthat/test-piping-ini.R index b7f655d0a..a0b63fedf 100644 --- a/tests/testthat/test-piping-ini.R +++ b/tests/testthat/test-piping-ini.R @@ -145,6 +145,7 @@ test_that(".iniSimplifyAssignArrow", { }) test_that("piping with ini can update reorder parameters (rxode2/issues#352)", { + mod <- function() { ini({ a <- 1 @@ -157,7 +158,9 @@ test_that("piping with ini can update reorder parameters (rxode2/issues#352)", { b ~ add(addSd) }) } + ui <- rxode2(mod) + # No modification expect_equal(ui$iniDf$name, c("a", "b", "c", "addSd")) # b to the top by number @@ -170,6 +173,9 @@ test_that("piping with ini can update reorder parameters (rxode2/issues#352)", { expect_equal(suppressMessages(ini(ui, b <- 1, append = TRUE))$iniDf$name, c("a", "c", "addSd", "b")) # b to the bottom by name expect_equal(suppressMessages(ini(ui, b <- 1, append = "addSd"))$iniDf$name, c("a", "c", "addSd", "b")) + + expect_equal(suppressMessages(ini(ui, b <- 1, append = addSd))$iniDf$name, c("a", "c", "addSd", "b")) + # b after c expect_equal(suppressMessages(ini(ui, b <- 1, append = "c"))$iniDf$name, c("a", "c", "b", "addSd")) # a and b after c; counter-intuitive: the order of a and b are reversed @@ -180,11 +186,16 @@ test_that("piping with ini can update reorder parameters (rxode2/issues#352)", { regexp = "parameter 'b' set to be moved after itself, no change in order made" ) + expect_error( + ini(ui, b <- 1, append = d/dt(fun)), + "append") + # Invalid parameter is correctly caught expect_error( ini(ui, b <- 1, append = "foo"), "append" ) + }) test_that(".iniAddCovarianceBetweenTwoEtaValues", { @@ -275,9 +286,10 @@ test_that(".iniHandleAppend", { b ~ add(addSd) }) } + expect_error( ini(mod, a <- 1, append=factor("A")), - regexp = "'append' must be NULL, logical, numeric, or character" + regexp = "'append' must be NULL, logical, numeric, or character/expression of variable in model" ) expect_error( ini(mod, q <- 1, append=0), diff --git a/tests/testthat/test-ui-piping.R b/tests/testthat/test-ui-piping.R index 2aac8fe55..0103e6420 100644 --- a/tests/testthat/test-ui-piping.R +++ b/tests/testthat/test-ui-piping.R @@ -1428,6 +1428,18 @@ rxTest({ expect_true("cp1" %in% f2$mv0$lhs) expect_equal(f2$lstExpr[[length(f2$lstExpr)]], quote(cp1 <- cp)) + f <- rxode2(ocmt) + f2 <- f %>% model(cp1 <- cp, append=Inf) + + expect_true("cp1" %in% f2$mv0$lhs) + expect_equal(f2$lstExpr[[length(f2$lstExpr)]], quote(cp1 <- cp)) + + f <- rxode2(ocmt) + f2 <- f %>% model(cp1 <- cp, append=100) + + expect_true("cp1" %in% f2$mv0$lhs) + expect_equal(f2$lstExpr[[length(f2$lstExpr)]], quote(cp1 <- cp)) + f2 <- f %>% model(f2 <- 3 * 2, append=NA) expect_true("f2" %in% f2$mv0$lhs) expect_equal(f2$lstExpr[[1]], quote(f2 <- 3 * 2)) @@ -2037,6 +2049,17 @@ test_that("piping append", { expect_equal(mod5$theta, c(tka = 0.45, tcl = 1, tv = 3.45, add.sd = 0.7)) + mod5 <- mod |> + model({ + PD <- 1-emax*cp/(ec50+cp) + ## + effect(0) <- e0 + kin <- e0*kout + d/dt(effect) <- kin*PD -kout*effect + }, append="d/dt(center)") + + expect_equal(mod5$theta, c(tka = 0.45, tcl = 1, tv = 3.45, add.sd = 0.7)) + mod6 <- mod5 |> model({ emax <- exp(temax) @@ -2057,6 +2080,46 @@ test_that("piping append", { eta.e0 ~ 1 })) + mod6 <- mod5 |> + model({ + emax <- exp(temax) + e0 <- exp(te0 + eta.e0) + ec50 <- exp(tec50) + kin <- exp(tkin) + kout <- exp(tkout) + }, append=FALSE) + + expect_equal( + mod6$omega, + lotri({ + eta.cl ~ 0.3 + eta.v ~ 0.1 + eta.e0 ~ 1 + })) + + expect_equal(mod6$theta, + c(tka = 0.45, tcl = 1, tv = 3.45, add.sd = 0.7, temax = 1, te0 = 1, tec50 = 1, tkin = 1, tkout = 1)) + + mod6 <- mod5 |> + model({ + emax <- exp(temax) + e0 <- exp(te0 + eta.e0) + ec50 <- exp(tec50) + kin <- exp(tkin) + kout <- exp(tkout) + }, append=0) + + expect_equal( + mod6$omega, + lotri({ + eta.cl ~ 0.3 + eta.v ~ 0.1 + eta.e0 ~ 1 + })) + + expect_equal(mod6$theta, + c(tka = 0.45, tcl = 1, tv = 3.45, add.sd = 0.7, temax = 1, te0 = 1, tec50 = 1, tkin = 1, tkout = 1)) + # make sure auto model piping turns off withr::with_options(list(rxode2.autoVarPiping=FALSE),