search

metrumresearchgroup / bbr

R interface for model and project management
https://metrumresearchgroup.github.io/bbr/
Other
23 stars 2 forks source link

get_* helpers for returning omega/sigma matrices and labeled theta values #515

Closed barrettk closed 2 years ago

barrettk commented 2 years ago

Proposed functionality:

Single model summary

sum <- MOD1 %>% model_summary()
> sum %>% get_theta()
     THETA1      THETA2      THETA3      THETA4      THETA5 
  2.3412300  58.7268000 529.6200000  -0.0807956   4.1189400 

> sum %>% get_omega()
           OMEGA(1,1) OMEGA(1,2)
OMEGA(1,1)   0.104694   0.000000
OMEGA(2,1)   0.000000   0.159368

> sum %>% get_sigma()
           SIGMA(1,1)
SIGMA(1,1)          1

With multiple model summaries

mods <- list(MOD1, mod)
sums <- mods %>% model_summaries()
> sums %>% get_theta()
[[1]]
     THETA1      THETA2      THETA3      THETA4      THETA5 
  2.3171600  54.6151000 462.5140000  -0.0820082   4.1795900 

[[2]]
     THETA1      THETA2      THETA3      THETA4      THETA5 
  2.3412300  58.7268000 529.6200000  -0.0807956   4.1189400 

> sums %>% get_omega()
[[1]]
           OMEGA(1,1) OMEGA(1,2)
OMEGA(1,1)  0.0985328   0.000000
OMEGA(2,1)  0.0000000   0.156825

[[2]]
           OMEGA(1,1) OMEGA(1,2)
OMEGA(1,1)   0.104694   0.000000
OMEGA(2,1)   0.000000   0.159368

> sums %>% get_sigma()
[[1]]
           SIGMA(1,1)
SIGMA(1,1)          1

[[2]]
           SIGMA(1,1)
SIGMA(1,1)          1

closes https://github.com/metrumresearchgroup/bbr/issues/442

seth127 commented 2 years ago

I like this approach.

Do we have any models (maybe in complex?) that have off-diagonals for either OMEGA or SIGMA. I just want to make sure we can test against that to make sure we're parsing the matrices correctly.

kylebaron commented 2 years ago

I think the names on the matrices should be refactored; it doesn't really make sense to have both the row and column index in the row name ... the 2nd row is all OMEGA 2 and the 3rd column is all OMEGA 3.

Something like

[[1]]
           OMEGA(1)   OMEGA(2)
OMEGA(1)  0.0985328   0.000000
OMEGA(2)  0.0000000   0.156825

or make it easier to work with in R

[[1]]
           OMEGA_1   OMEGA_2
OMEGA_1  0.0985328   0.000000
OMEGA_2  0.0000000   0.156825
barrettk commented 2 years ago

I like this approach.

Do we have any models (maybe in complex?) that have off-diagonals for either OMEGA or SIGMA. I just want to make sure we can test against that to make sure we're parsing the matrices correctly.

It seems example2_saemimp is the only one we can rely on. acop-fake-bayes and acop-iov do have the necessary info, but do not have any covariance data. Its possible that 1001 and acop-onlysim would based on the ctl files, but they are missing relevant data in the summary calls and cant be used.

> sums %>% get_omega()
[[1]]
           OMEGA(1,1) OMEGA(1,2)
OMEGA(1,1)  0.0985328   0.000000
OMEGA(2,1)  0.0000000   0.156825

[[2]]
             OMEGA(1,1)  OMEGA(1,2)  OMEGA(1,3)   OMEGA(1,4)
OMEGA(1,1)  0.010410500 0.000319542 0.001977200 -0.000189864
OMEGA(2,1)  0.000319542 0.008278020 0.000872168  0.001005720
OMEGA(3,1)  0.001977200 0.000872168 0.013139700  0.004040370
OMEGA(4,1) -0.000189864 0.001005720 0.004040370  0.010987300
barrettk commented 2 years ago

@seth127 wondering if I could do this:

#' @section Examples:
#'
#' ```{r, include = FALSE}
#' knitr::opts_chunk$set(collapse = TRUE, comment = "#>")
#' options('bbr.bbi_exe_path' = read_bbi_path())
#' MODEL_DIR <- system.file("model", "nonmem", "complex",   package = "bbr")
#' mod <- read_model(file.path(MODEL_DIR, "example2_saemimp"))
#' ```
#'
#' ```{r, comment = "#>", collapse = TRUE}
#' sum <- mod %>% model_summary()
#'
#' sum %>% get_omega()
#'
#'
#' sum %>% get_sigma()
#'
#'
#' sum %>% get_theta()
#' ```

This will appease drone, but im worried about how this helper doc would render after someone installed the package on their end (without running use_bbi()). I would add hidden conditional logic that would use that function if not found at that path, but I have a feeling there are reasons we wouldnt want to do that. A lot of this package doesn't have examples (which I think would be nice), but I can see the difficulty in making them for most functions without having bbi installed. Have thought about this for a while, but wondering what your thoughts are (in this case, and having examples relying on bbi in general)

In this case, I could easily paste the tables below as text, and not actually run any code, but moving forward I would be a fan of maybe having some version of bbi installed in inst or something (Im sure thats a terrible idea), that we could rely on for helper docs, but wouldn't be available to actual users without system.file() (which no one would do). Not sure thats the best solution, but I assume its at least better than including use_bbi() in the helper docs, or changing something more dramatic like installing bbi on load of bbr

seth127 commented 2 years ago

Good thoughts there @barrettk . At this point, I think our best move is to just do examples and make them \dontrun{. The main downside of that is that CRAN doesn't like it, but getting bbr on CRAN really isn't a priority because pretty much all likely users are gonna get it from MPN anyway. We already use this pattern in other places and you're right that we should probably add more examples for other functions.

maybe having some version of bbi installed in inst or something (Im sure thats a terrible idea)

Definitely not a terrible idea. I've thought about this a few times, not inst specifically, but somehow having a version of bbi included in bbr. Every time I run through it in my head, I end up deciding it's not a good idea, but I do keep coming back to it. There are a couple big advantages, a couple off the top:

Anyway, something to consider at some point, though I doubt we'll bite it off anytime soon. Lemme know if you want to talk further at some point.