openpharma / crmPack

Object-Oriented Implementation of CRM Designs
https://openpharma.github.io/crmPack/
20 stars 10 forks source link

Reduce execution time of `.DefaultDASimulations()` #821

Closed Puzzled-Face closed 5 months ago

Puzzled-Face commented 6 months ago

.DefaultDASimulations() has a relatively long execution time. Can this be improved?

startTime <- Sys.time()
.DefaultDASimulations()
endTime <- Sys.time()

endTime - startTime
Time difference of 56.84297 secs

The same is true, to a lesser extent, of DefaultSimulations().

startTime <- Sys.time()
.DefaultSimulations()
endTime <- Sys.time()

endTime - startTime
Time difference of 2.711638 secs
Puzzled-Face commented 6 months ago

Most likely due to the dose grid in DefaultDADesign():

  emptydata <- DataDA(
    doseGrid = c(0.1, 0.5, 1, 1.5, 3, 6, seq(from = 10, to = 80, by = 2)),
    Tmax = 60
  )

Hmmm. changing the dose grid to

doseGrid = c(0.1, 0.5, 1, 1.5, 3, 6, seq(from = 10, to = 80, by = 10))

reduces the execution time to 55 seconds. So that appears not to be the issue.

Puzzled-Face commented 5 months ago

@danielinteractive I think a potential solution would be to mock .DefaultDASimulations() during tests so that rather than creating a new instance every time the default constructor is called, we simply load the data from disk. Reading the Rds file should be effectively instant, so that close to a minute would be saved every time the default constructor is called.

To do this.

The mocked function would be

local_mocked_bindings(
  .DefaultDASimulations = function(...){
    readRDS(testthat::test_path("testdata", "DefaultDASimulations.Rds")
  }
)

or similar.

The major advantage of this approach would be that there are no changes necessary to any package code and all test results would remain unchanged.

See here for more details on mocking tools.

.DefaultDASimulations is not called explicitly in any test. It is (probably) called implicitly by tests in

Puzzled-Face commented 5 months ago

We could even modify .DefaultDASimulations() itself to load a static .Rds file as well.

This could be created by usethis::use_data(internal = TRUE). See section 7.2 of this for more information.

danielinteractive commented 5 months ago

Yeah good idea, I think this kind of caching makes sense