Concurrent simulation batch #513

PavelBal · 2021-05-05T12:11:22Z

No description provided.

…y/OSPSuite-R into parallel-run

…tionBatch

…cology/OSPSuite-R into concurrent-simulation-batch

…y/OSPSuite-R into concurrent-simulation-batch

PavelBal · 2021-05-05T13:38:16Z

Here is a proposal how to use simulation batches:

Create SimulationBatch(es), defining the simulation to run and parameter and initial values to vary

sim1 <- loadTestSimulation("simple", loadFromCache = TRUE)
sim2 <- loadTestSimulation("simple2", loadFromCache = TRUE)
parameters <- c("Organism|Liver|Volume", "R1|k1")
molecules <- "Organism|Liver|A"

simulationBatch1 <- createSimulationBatch(simulation = sim1,
    parametersOrPaths = parameters,
    moleculesOrPaths = molecules)
simulationBatch2 <- createSimulationBatch(simulation = sim2,
    parametersOrPaths = parameters,
    moleculesOrPaths = molecules)

Add run values to simulation batches. Currently, only one ran value set can be added per method call - if required, it could be extended to passing multiple parameter and initial values sets. Every added run values set gets an ID that can be later be used to get the right result.

#Ids of run values
ids <- c()
ids[[1]] <- simulationBatch1$addRunValues(parameterValues = c(1, 2), initialValues = 1)
ids[[2]] <- simulationBatch1$addRunValues(parameterValues = c(1.6, 2.4), initialValues = 3)
ids[[3]] <- simulationBatch2$addRunValues(parameterValues = c(4, 2), initialValues = 4)
ids[[4]] <- simulationBatch2$addRunValues(parameterValues = c(2.6, 4.4), initialValues = 5)

Run the batches. The results are grouped by the simulation batches (i.e., the output is a list with a list of SimulationResult per batch). Every entry of SimulationResult is named by the ID of of run values set.

res <- runSimulationBatches(simulationBatches = list(simulationBatch1, simulationBatch2)) resultsOfFirstBatch <- res[[1]] resultsOfSecondBatch <- res[[2]]

Running the batches removes all added run values, so next run would result in no results.

R/error-checks.R

R/messages.R

R/simulation-batch.R

R/utilities-simulation.R

msevestre · 2021-05-05T16:47:44Z

@PavelBal The build is failing
https://ci.appveyor.com/project/open-systems-pharmacology-ci/ospsuite-r

PavelBal · 2021-05-05T18:05:51Z

@msevestre Yes, it is using the old Core... And if I replace the libraries, it fails because of PK-Sim.

msevestre · 2021-05-05T18:29:31Z

ok let me check this out

msevestre · 2021-05-05T18:46:47Z

I am update the lib now to make sure we are using the right version of everything. Let' see

…-Systems-Pharmacology/OSPSuite-R into concurrent-simulation-batch

msevestre · 2021-05-05T19:12:42Z

I was trying to fix this and it was not letting me push lol.. I understand now

codecov-commenter · 2021-05-05T19:46:11Z

Codecov Report

Merging #513 (af34c0a) into develop (46864ad) will increase coverage by 0.06%.
The diff coverage is 90.41%.

@@             Coverage Diff             @@
##           develop     #513      +/-   ##
===========================================
+ Coverage    88.37%   88.43%   +0.06%     
===========================================
  Files           72       72              
  Lines         1359     1418      +59     
===========================================
+ Hits          1201     1254      +53     
- Misses         158      164       +6

Impacted Files	Coverage Δ
R/get-net-task.R	`100.00% <ø> (ø)`
R/utilities-pk-parameter.R	`100.00% <ø> (ø)`
R/error-checks.R	`92.72% <62.50%> (-3.57%)`	⬇️
R/simulation-batch.R	`88.23% <77.77%> (-11.77%)`	⬇️
R/utilities-simulation.R	`97.81% <96.22%> (-1.03%)`	⬇️
R/messages.R	`100.00% <100.00%> (ø)`
R/utilities-units.R	`93.58% <0.00%> (+0.08%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 99e01f9...af34c0a. Read the comment docs.

msevestre

I think the API is nice. I am wondering about a few things

1-the ptr
2-the dispose

msevestre · 2021-05-06T18:31:20Z

R/error-checks.R

  if (is.null(object)) {
+    if (nullAllowed) {


return (nullAllowed)

Hah, nice shortcut.

R/error-checks.R

msevestre · 2021-05-06T18:46:45Z

R/error-checks.R

  if (is.null(value)) {
+    if (nullAllowed) {


Why did you change this? Is this more readable to have two ifs instead of one? Not sure.
Also we have the same conde in ValidateIsInteger. I always try to exit early. Here, you are adding complexity I believe because of the imbricated ifs

It took me some time to understand why I did it... I try to avoid testing for nullAllowed if value is not NULL. Probably completely insignificant, and actually much better to test

if (is.null(value) && nullAllowed)

Will do it this way.

UPDATE:: Actually no, I tried to avoid testing for is.null(value) twice.

@msevestre You decide which approach to take.

R/simulation-batch.R

R/utilities-simulation.R

msevestre · 2021-05-06T19:36:50Z

R/utilities-simulation.R

+  for (simBatchIdx in seq_along(simulationBatches)) {
+    simBatch <- simulationBatches[[simBatchIdx]]
+    # Put the simulation into sim pointer id <-> sim map
+    simId <- deparse(rClr::clrGetExtPtr(simBatch$ref))


WHAT IS THIS? LEt's please have a batch return an id or sthg.

:D Yes then we need to add an ID field to OSPSuite.R.Services.SettingsForConcurrentRunSimulationBatch in core.

ok I added one and it's being used now. Please review

msevestre · 2021-05-06T19:38:52Z

R/utilities-simulation.R

+    simulationResults[which(resultsIdSimulationIdMap == simId)]
+  })
+  # Not sure this is required
+  rClr::clrCall(simulationRunner, "Dispose")


there is an issue here. If this object is returned via a Task, it might be singleton and should NOT Be disposed.

Ok, I don't have experience with this. Removing the dispose.

we might need this. A task should not be disposable. It might be something I need to discuss with @abdelr

in short,if this is disposable, we should get a new instance everytime.

Ok I checked. Dispose is fine and should be call in that case explicitly. Not a big deal if not but it will release stuff immediately.

R/utilities-simulation.R

PavelBal

@msevestre There are some open questions left - e.g. SimBatch should return an ID must be first implemented in Core. Please see my answers.

PavelBal · 2021-05-06T19:51:38Z

R/error-checks.R

  if (is.null(object)) {
+    if (nullAllowed) {


Hah, nice shortcut.

PavelBal · 2021-05-06T19:58:27Z

R/error-checks.R

  if (is.null(value)) {
+    if (nullAllowed) {


It took me some time to understand why I did it... I try to avoid testing for nullAllowed if value is not NULL. Probably completely insignificant, and actually much better to test

if (is.null(value) && nullAllowed)

Will do it this way.

UPDATE:: Actually no, I tried to avoid testing for is.null(value) twice.

PavelBal · 2021-05-06T20:02:15Z

R/error-checks.R

  if (is.null(value)) {
+    if (nullAllowed) {


@msevestre You decide which approach to take.

PavelBal · 2021-05-06T20:04:54Z

R/simulation-batch.R

+    #' ids[[1]] <- simulationBatch1$addRunValues(parameterValues = c(1, 2), initialValues = 1)
+    #' ids[[2]] <- simulationBatch1$addRunValues(parameterValues = c(1.6, 2.4), initialValues = 3)
+    #' ids[[3]] <- simulationBatch2$addRunValues(parameterValues = c(4, 2), initialValues = 4)
+    #' ids[[4]] <- simulationBatch2$addRunValues(parameterValues = c(2.6, 4.4), initialValues = 5)


Was just an example with multiple batches. But as it is dontrun anyways, I update the example with two different simulations.

R/utilities-simulation.R

PavelBal · 2021-05-06T20:10:34Z

R/utilities-simulation.R

+  for (simBatchIdx in seq_along(simulationBatches)) {
+    simBatch <- simulationBatches[[simBatchIdx]]
+    # Put the simulation into sim pointer id <-> sim map
+    simId <- deparse(rClr::clrGetExtPtr(simBatch$ref))


:D Yes then we need to add an ID field to OSPSuite.R.Services.SettingsForConcurrentRunSimulationBatch in core.

PavelBal · 2021-05-06T20:14:10Z

R/utilities-simulation.R

+    simulationResults[which(resultsIdSimulationIdMap == simId)]
+  })
+  # Not sure this is required
+  rClr::clrCall(simulationRunner, "Dispose")


Ok, I don't have experience with this. Removing the dispose.

R/utilities-simulation.R

…tionBatch in comments.

PavelBal · 2021-05-10T08:56:14Z

@msevestre OK I think this is ready to merge.

PavelBal added 16 commits March 24, 2021 10:28

Methods for concurrent running of simulations.

3906ecb

Merge branch 'develop' of https://github.com/Open-Systems-Pharmacolog…

4a4f8fc

…y/OSPSuite-R into parallel-run

Dimensions utilities

c839af2

-WIP

118912a

Merge branch 'develop' of https://github.com/Open-Systems-Pharmacolog…

1bba754

…y/OSPSuite-R into parallel-run

Implement runSimulationsConcurrently

fa80ad9

Remov obsolete code

66c25f6

Run styler

6bf9635

Build options

321a41e

Docu update

6cf8594

Project change

d99ee24

SimulationBatch is OSPSuite.R.Services.SettingsForConcurrentRunSimula…

0a2da8a

…tionBatch

Merge branch 'parallel-run' of https://github.com/Open-Systems-Pharma…

759fbf1

…cology/OSPSuite-R into concurrent-simulation-batch

Working on concurrent batch runner

bcfeae5

Added function to run simulation batches concurrently

494d8d1

Merge branch 'develop' of https://github.com/Open-Systems-Pharmacolog…

74540b3

…y/OSPSuite-R into concurrent-simulation-batch

PavelBal requested a review from msevestre May 5, 2021 12:31

Add example for concurrent batch runner

ef17834