Creation of new model with inital parameter values from final parameter values of prior model

oskarrgh commented 1 year ago

Missing function for creating new model with initial theta/omega/sigma parameter values taken from final estimates of a prior model. Could perhaps be either stand-alone function or addition to copy_model_from function. Think this would add great value!

seth127 commented 1 year ago

Thanks Oskar. We've heard this from a few other users as well. We are definitely considering adding this feature.

One of the primary concerns with this in the past has been how reliably we can parse the parameter blocks from the control stream, given the flexibility that NONMEM allows for defining them. I remember that OMEGA blocks in particular were a concern. @kylebaron do you have an input here? Is there anything you've done in mrgsolve or elsewhere that we could piggy-back off of, in terms of parsing parameter blocks from a NONMEM control stream?

timwaterhouse commented 1 year ago

I agree that this would be a good addition. An alternative to trying to parse the control stream could involve using an .msf file somehow. I'm not sure how tricky that's going to be to feed in info from one model to another, though.

seth127 commented 1 year ago

@barrettk is going to look into this a bit. Kyle, also reach out to @andersone1 because he has some ideas. In particular, whether we want to use/borrow anything from this old function, originally in metrumrg.

seth127 commented 8 months ago

This has come along way since the last comment on this issue. I think we're pretty close to having it implemented, with the bulk of the new code actually living in nmrec (https://github.com/metrumresearchgroup/nmrec/pull/18/ , currently being reviewed).

@oskarrgh I wanted to get some user review at this point, if possible. Would you (and/or anyone else) be willing to take a look at the test cases in the nmrec PR and verify that the replacement is happening the way that you would expect it to? Any comments can be left on https://github.com/metrumresearchgroup/nmrec/pull/18/ .

Some explanation on the test cases

These are testing internal helper functions which a bbr user would not interact with directly. Those functions are defined and documented in nmrec/R/set-param.R, if you are interested in looking at them.

Each test case is set up like so:


list(
  lines = "$omega 1 2 3",
  values = c(
    4,
    0, 5,
    0, 0, 6
  ),
  want = "$omega 4 5 6"
)

lines is a snippet from an "original" NONMEM control stream
values are the final estimates from a parent model (more on this format below)
want is a snippet from the resulting NONMEM control stream, after we have replaced the relevant initial estimates with the final estimates from values

The key feedback we want from users is whether want looks as you would expect it to.

`values` format

For $THETA, values will be a vector, similar to what bbr::get_theta() returns.

For $OMEGA and $SIGMA, values can be either a full matrix, similar to what bbr::get_omega() and bbr::get_sigma() return, or a vector in row-major order for values in the lower triangle. In other words, the following two forms are both valid (and identical):

# matrix
matrix(
  c(
    4, 0, 0,
    0, 5, 0,
    0, 0, 6
  ),
  nrow = 3, byrow = TRUE
)

# row-major vector
c(4, 0, 5, 0, 0, 6)

seth127 commented 8 months ago

Unrelated to this review request in the previous comment: @oskarrgh as currently implemented, this does not change the final estimates at all before passing them on to the child model. However, we have had some other folks comment that they would prefer to "jitter" the estimates slightly (i.e. randomly perturb them by 10-15%) before passing them to the child.

Do you have an opinion on whether that "jitter" is a necessary feature here?

My current thinking is that we may implement and release this as is (no jitter) and then add the jitter feature in another release (hopefully within a month or two). Does that make sense, or do you think we should just save this and release them together?

(I asked @oskarrgh here because he filed the original issue, but I'm very open to anyone's opinion on this.)

oskarrgh commented 8 months ago

Unrelated to this review request in the previous comment: @oskarrgh as currently implemented, this does not change the final estimates at all before passing them on to the child model. However, we have had some other folks comment that they would prefer to "jitter" the estimates slightly (i.e. randomly perturb them by 10-15%) before passing them to the child.

Do you have an opinion on whether that "jitter" is a necessary feature here?

My current thinking is that we may implement and release this as is (no jitter) and then add the jitter feature in another release (hopefully within a month or two). Does that make sense, or do you think we should just save this and release them together?

(I asked @oskarrgh here because he filed the original issue, but I'm very open to anyone's opinion on this.)

I would release it as is, I think it is important enough to be pushed forward asap. Then as you suggest make the jittering a feature in the near future. With users coming from psn-nonmem usage I think this jitter feature is going to be expected so great that it was brought up.

seth127 commented 7 months ago

Thanks for the input @oskarrgh . I think we'll plan to move forward with this and address the jitter/tweak functionality separately.

seth127 commented 6 months ago

@oskarrgh this has been officially released! https://github.com/metrumresearchgroup/bbr/releases/tag/1.8.0

You can see the newly published documentation here.

Thanks again to @kyleam @barrettk for all of the work on this, as well as valuable input from a number of other folks (including Oskar).

metrumresearchgroup / bbr