From 6d200d07f65a0c5a20a42f03984787880191a543 Mon Sep 17 00:00:00 2001
From: Rudi Engelke <9340110+rengelke@users.noreply.github.com>
Date: Thu, 4 Jul 2024 16:51:02 +0200
Subject: [PATCH] Fix simulation id validation (#138)
* extract simulation ids assuming up to two levels of piParameters depth
* ensure simulation ids are identical
* update snapshots due to new error bar caps size in ospsuite::DefaultPlotConfiguration
* improve error message for missing simulation IDs
* Add tests for multi-path ID verification
* run styler:::style_active_pkg()
---
R/ParameterIdentification.R | 2 +-
R/messages.R | 15 ++++--
R/utilities-simulation.R | 31 +++++++++---
.../after-estimation.svg | 24 ++++-----
.../before-estimation.svg | 24 ++++-----
.../custom-parameter.svg | 24 ++++-----
tests/testthat/helper-for-tests.R | 41 +++++++++++++++
.../testthat/test-parameter-identification.R | 50 ++++++++++++++++++-
.../test-utilities-object-functions.R | 3 +-
9 files changed, 161 insertions(+), 53 deletions(-)
diff --git a/R/ParameterIdentification.R b/R/ParameterIdentification.R
index dd2951a..883c0ab 100644
--- a/R/ParameterIdentification.R
+++ b/R/ParameterIdentification.R
@@ -263,7 +263,7 @@ ParameterIdentification <- R6::R6Class(
costControl$scaling <- private$.outputMappings[[idx]]$scaling
ospsuite.utils::validateIsOption(
options = costControl,
- validOptions = ObjectiveFunctionSpecs
+ validOptions = ObjectiveFunctionSpecs
)
costSummary <- calculateCostMetrics(
df = obsVsPredDf,
diff --git a/R/messages.R b/R/messages.R
index f625f47..ef9f180 100644
--- a/R/messages.R
+++ b/R/messages.R
@@ -32,9 +32,14 @@ messages$hessianEstimation <- function() {
"Post-hoc estimation of Hessian matrix."
}
-messages$errorSimulationIdMissing <- function(id) {
- paste0(
- "Mismatch or missing ID detected: ", id,
- ". Ensure each Simulation ID matches with corresponding PIParameter and OutputMapping IDs."
- )
+messages$errorSimulationIdMissing <- function(simulationIds, piParamIds, outputMappingIds) {
+ message <- capture.output(cat(
+ "Mismatch or missing ID detected.\n",
+ "Ensure each Simulation ID matches with corresponding PIParameter and OutputMapping IDs.\n",
+ "Simulation IDs: ", paste(simulationIds, collapse = ", "), "\n",
+ "PIParameter IDs: ", paste(piParamIds, collapse = ", "), "\n",
+ "OutputMapping IDs: ", paste(outputMappingIds, collapse = ", ")
+ ))
+
+ return(paste(message, collapse = "\n"))
}
diff --git a/R/utilities-simulation.R b/R/utilities-simulation.R
index e97b0e8..15dc1c4 100644
--- a/R/utilities-simulation.R
+++ b/R/utilities-simulation.R
@@ -31,19 +31,34 @@
#' the mismatch or absence of IDs.
#' @keywords internal
.validateSimulationIds <- function(simulationIds, piParameters, outputMappings) {
- # Extract unique IDs from piParameters
- piParamIds <- lapply(piParameters, function(param) .getSimulationContainer(param$parameters[[1]])$id)
+ # Extract unique IDs from piParameters assuming up to two levels of list depth
+ piParamIds <- lapply(piParameters, function(param) {
+ if (is.list(param$parameters)) {
+ return(lapply(param$parameters, function(sub_param) {
+ .getSimulationContainer(sub_param)$id
+ }))
+ } else {
+ return(.getSimulationContainer(param$parameters[[1]])$id)
+ }
+ })
piParamIds <- unique(unlist(piParamIds))
# Extract unique IDs from outputMappings
- outputMappingIds <- lapply(outputMappings, function(mapping) .getSimulationContainer(mapping$quantity)$id)
+ outputMappingIds <- lapply(outputMappings, function(mapping) {
+ .getSimulationContainer(mapping$quantity)$id
+ })
outputMappingIds <- unique(unlist(outputMappingIds))
- # Validate that each simulationId is present in both piParamIds and outputMappingIds
- for (id in simulationIds) {
- if (!(id %in% piParamIds && id %in% outputMappingIds)) {
- stop(messages$errorSimulationIdMissing(id))
- }
+ # sort IDs before comparison
+ simulationIds <- sort(unique(unlist(simulationIds)))
+ piParamIds <- sort(piParamIds)
+ outputMappingIds <- sort(outputMappingIds)
+
+ # Validate that simulationId is identical with piParamIds and outputMappingIds
+ if (!identical(simulationIds, piParamIds) || !identical(simulationIds, outputMappingIds)) {
+ stop(messages$errorSimulationIdMissing(
+ simulationIds, piParamIds, outputMappingIds
+ ), call. = TRUE)
}
return()
diff --git a/tests/testthat/_snaps/parameter-identification/after-estimation.svg b/tests/testthat/_snaps/parameter-identification/after-estimation.svg
index 2f54167..c0575b8 100644
--- a/tests/testthat/_snaps/parameter-identification/after-estimation.svg
+++ b/tests/testthat/_snaps/parameter-identification/after-estimation.svg
@@ -133,12 +133,12 @@
-
-
-
-
-
-
+
+
+
+
+
+
@@ -390,12 +390,12 @@
-
-
-
-
-
-
+
+
+
+
+
+
diff --git a/tests/testthat/_snaps/parameter-identification/before-estimation.svg b/tests/testthat/_snaps/parameter-identification/before-estimation.svg
index 38e895c..a6e1aa6 100644
--- a/tests/testthat/_snaps/parameter-identification/before-estimation.svg
+++ b/tests/testthat/_snaps/parameter-identification/before-estimation.svg
@@ -133,12 +133,12 @@
-
-
-
-
-
-
+
+
+
+
+
+
@@ -388,12 +388,12 @@
-
-
-
-
-
-
+
+
+
+
+
+
diff --git a/tests/testthat/_snaps/parameter-identification/custom-parameter.svg b/tests/testthat/_snaps/parameter-identification/custom-parameter.svg
index 87e0c51..2f4fadf 100644
--- a/tests/testthat/_snaps/parameter-identification/custom-parameter.svg
+++ b/tests/testthat/_snaps/parameter-identification/custom-parameter.svg
@@ -133,12 +133,12 @@
-
-
-
-
-
-
+
+
+
+
+
+
@@ -390,12 +390,12 @@
-
-
-
-
-
-
+
+
+
+
+
+
diff --git a/tests/testthat/helper-for-tests.R b/tests/testthat/helper-for-tests.R
index 5059dfc..fe379a8 100644
--- a/tests/testthat/helper-for-tests.R
+++ b/tests/testthat/helper-for-tests.R
@@ -87,6 +87,47 @@ getTestOutputMapping <- function() {
testOutputMapping <- getTestOutputMapping()
+# PI multiple simulations and parameter paths
+
+sim_250mg <- loadSimulation(
+ system.file("extdata", "Aciclovir.pkml", package = "ospsuite"))
+sim_500mg <- loadSimulation(
+ system.file("extdata", "Aciclovir.pkml", package = "ospsuite"))
+
+piParameterLipo <- PIParameters$new(parameters = list(
+ getParameter(path = "Aciclovir|Lipophilicity", container = sim_250mg),
+ getParameter(path = "Aciclovir|Lipophilicity", container = sim_500mg)
+))
+piParameterCl_250mg <- PIParameters$new(
+ parameters = getParameter(
+ path = "Neighborhoods|Kidney_pls_Kidney_ur|Aciclovir|Renal Clearances-TS|TSspec",
+ container = sim_250mg
+ )
+)
+piParameterCl_500mg <- PIParameters$new(
+ parameters = getParameter(
+ path = "Neighborhoods|Kidney_pls_Kidney_ur|Aciclovir|Renal Clearances-TS|TSspec",
+ container = sim_500mg
+ )
+)
+
+# outputMapping_250mg <- testOutputMapping()
+# outputMapping_500mg <- testOutputMapping()
+
+simOutputPath <- "Organism|PeripheralVenousBlood|Aciclovir|Plasma (Peripheral Venous Blood)"
+outputMapping_250mg <- PIOutputMapping$new(
+ quantity = getQuantity(path = simOutputPath, container = sim_250mg)
+)
+outputMapping_500mg <- PIOutputMapping$new(
+ quantity = getQuantity(path = simOutputPath, container = sim_500mg)
+)
+outputMapping_250mg$addObservedDataSets(
+ testObservedData()$`AciclovirLaskinData.Laskin 1982.Group A`
+)
+outputMapping_500mg$addObservedDataSets(
+ testObservedData()$`AciclovirLaskinData.Laskin 1982.Group A`
+)
+
# Variables
testQuantity <- ospsuite::getQuantity(
diff --git a/tests/testthat/test-parameter-identification.R b/tests/testthat/test-parameter-identification.R
index c450589..d8c95eb 100644
--- a/tests/testthat/test-parameter-identification.R
+++ b/tests/testthat/test-parameter-identification.R
@@ -36,6 +36,50 @@ test_that("ParameterIdentification correctly throws an error upon missing Simula
)
})
+test_that("ParameterIdentification verifies simulation IDs with multiple simulations and parameters correctly", {
+
+ # no error with multiple simulations and parameter paths
+ expect_no_error(
+ ParameterIdentification$new(
+ simulations = list(sim_250mg, sim_500mg),
+ parameters = list(piParameterLipo, piParameterCl_250mg, piParameterCl_500mg),
+ outputMappings = list(outputMapping_250mg, outputMapping_500mg),
+ configuration = piConfiguration
+ )
+ )
+
+ # error missing simulation ID
+ expect_error(
+ ParameterIdentification$new(
+ simulations = list(sim_250mg),
+ parameters = list(piParameterLipo, piParameterCl_250mg, piParameterCl_500mg),
+ outputMappings = list(outputMapping_250mg, outputMapping_500mg),
+ configuration = piConfiguration
+ ),
+ "Mismatch or missing ID detected"
+ )
+ # error missing parameter ID
+ expect_error(
+ ParameterIdentification$new(
+ simulations = list(sim_250mg, sim_500mg),
+ parameters = list(piParameterCl_250mg),
+ outputMappings = list(outputMapping_250mg, outputMapping_500mg),
+ configuration = piConfiguration
+ ),
+ "Mismatch or missing ID detected"
+ )
+ # error missing output mapping ID
+ expect_error(
+ ParameterIdentification$new(
+ simulations = list(sim_250mg, sim_500mg),
+ parameters = list(piParameterLipo, piParameterCl_250mg, piParameterCl_500mg),
+ outputMappings = list(outputMapping_500mg),
+ configuration = piConfiguration
+ ),
+ "Mismatch or missing ID detected"
+ )
+})
+
test_that("ParameterIdentification returns an infinite cost structure if the
simulation is NA", {
piTask <- createPiTask()
@@ -72,12 +116,14 @@ test_that("ParameterIdentification$run() errors on invalid objective function op
piTask <- createPiTask()
piTask$configuration$objectiveFunctionOptions$objectiveFunctionType <- "invalidType"
expect_error(piTask$run(),
- regexp = "Value\\(s\\) 'invalidType' not included in allowed values: 'lsq, m3'")
+ regexp = "Value\\(s\\) 'invalidType' not included in allowed values: 'lsq, m3'"
+ )
piTask <- createPiTask()
piTask$configuration$objectiveFunctionOptions$linScaleCV <- 10
expect_error(piTask$run(),
- regexp = "Value\\(s\\) out of the allowed range: \\[1e-09, 1\\]")
+ regexp = "Value\\(s\\) out of the allowed range: \\[1e-09, 1\\]"
+ )
})
# Test BOBYQA Algorithm (Default)
diff --git a/tests/testthat/test-utilities-object-functions.R b/tests/testthat/test-utilities-object-functions.R
index 763ffbb..7dcb4ee 100644
--- a/tests/testthat/test-utilities-object-functions.R
+++ b/tests/testthat/test-utilities-object-functions.R
@@ -1,7 +1,8 @@
## context(".calculateCensoredContribution")
obsVsPredDf <- readr::read_csv(getTestDataFilePath("Aciclovir_obsVsPredDf.csv"),
- show_col_types = FALSE)
+ show_col_types = FALSE
+)
obsDf <- obsVsPredDf[obsVsPredDf$dataType == "observed", ]
predDf <- obsVsPredDf[obsVsPredDf$dataType == "simulated", ]