Closed barrettk closed 5 months ago
Examples so far:
source("/data/Projects/package_dev/bbr/tests/testthat/setup-workflow-ref.R", echo=FALSE)
MODEL_DIR_C <- system.file("model", "nonmem", "complex", package = "bbr")
These are the examples that have been tested thus far
.mod <- MOD1
.mod <- read_model(file.path(MODEL_DIR_C, "1001"))
.mod <- read_model(file.path(MODEL_DIR_C, "example2_saemimp"))
Base Model. Bounds must be tabulated so that we can respect them when tweaking the initial estimates (see conversation here)
Model with multiple records: here we create a combined matrix, which will facilitate passing jittered values to set_omega()
. If one of the matrices was FIXED
, we would need additional logic for not overwriting those values
Note: this model as prior blocks, so the output here is technically incorrect, or at least lacking in terms of differentiating between actual records and priors. Additional logic would be needed to filter these out or error if the expected length wasnt found
.mod <- read_model(file.path(MODEL_DIR_C, "example2_saemimp"))
initial_estimates()
Here I was thinking about filtering out initial values with NA
, as this indicates the value was not specified in the control stream file, and would likely reflect the user's expectation more appropriately. Edit: decided to do this for now
Hey @seth127, I just marked you for an intermediate review. Some things that still need to be done or talked about:
nmrec
version in a few functions/tests, along with the drone yaml, once a new version of nmrec
is released
nmrec
sideinitial_estimates
wrapper given the nature of it, but let me know if you think I shouldnmrec
will handle most operations)Im going to spend a little more time on documentation, but after that I was planning on waiting for a first review/some other parts to move along
As @seth127 noted, we will need a separate PR to address these issues. The new tweak_initial_estimates()
function works for THETA
parameters, but is flawed in its handling of matrices. Still needed:
@seth127 something I just thought about. This is related to a conversation @kyleam and I had regarding the handling of prior blocks that follow the old naming convention (see final thoughts).
Basically, prior blocks currently get "grouped" when creating the combined matrix. This would also occur for THETA records:
I was reminded about this conversation when doing some preliminary testing for the positive-definiteness, but now im not sure if we should address this in this PR, given that it also implicates THETA
records.
Im not really sure how to handle this. In the case of inherit_param_estimates()
, Kyle compares the length of the passed-in values (from model_summary
output) to the detected number of record values. We cant do that here though, as the length of the passed-in values is determined by the length of the fetched initial estimates. I dont see a way around parsing the PRIOR
block or allowing priors to be jittered.
Basically, prior blocks currently get "grouped" when creating the combined matrix.
My opinion for how to proceed is what I said in the issue you linked: we should document that using non-informative record names for priors is not supported with this feature.
@kyleam said
we should document that using non-informative record names for priors is not supported with this feature.
and I think I agree with that, in principle. However, in practice, we never know that they're using non-informative priors in this case.
Above, @barrettk said
[
nmrec
] compares the length of the passed-in values (frommodel_summary
output) to the detected number of record values. We cant do that here though, as the length of the passed-in values is determined by the length of the fetched initial estimates.
I've looked through the code a bit and I'm pretty sure that's right. My evidence:
length(values) != len_expected
inherit_param_estimates()
call, values
is ultimately coming from a model_summary()
on a previously executed model (e.g. here). At that point, we know they're all real parameters (and not a non-informative priors), and so the mismatch/error in set_param()
gets triggered.tweak_initial_estimates()
call, the thing that is eventually passed to values
is coming from the nmrec::extract_*
functions (here, which leads to here). And since the extract_*
don't differentiate between real parameters and non-informative priors... we never get the mismatch.Soooooo... that means we can't just leave it as is and expect that error to bubble up.
In effect, this means that the current state actually tweaks non-informative priors too. The big question from me: is it ok for us to tweak the non-informative priors the same way that we tweak the initial parameter estimates?
If so, then we can just document that behavior and move on.
If that's not ok... then I'm not sure what the next step is. I guess I'd just say "let's cross that bridge when we come to it" (and hope we don't come to it).
@curtisKJ @timwaterhouse could either of you weigh in on my "big question" at the top of this comment?
One final note (in favor of leaving as is): anecdotally, in most cases I've seen, the non-informative priors use FIX
. In that case, they wouldn't be tweaked anyway. I'm not sure if that's "standard practice" or not, but I thought it was worth noting.
Thanks @seth127. I haven't reviewed this PR or been involved in many of the design discussions, so it's helpful to see you expand on those details.
@kyleam I think you're saying we should just let this case fall through to this error
In the linked thread, I had assumed that would trigger, but for the latest comment here I took for granted @barrettk's claim that that doesn't get triggered. (Thank you both for pointing that out.)
So, in the comment above, I was just casting my vote for documenting that this feature is not meant to be used with uninformative prior names, accepting that it will jitter them if there (i.e. your "document that behavior and move on").
A few more thoughts:
IIUC the common workflow here is "inherit_param_estimates then tweak", in which case the first step would flag control streams that are using non-informative names. So in practice it seems to me that this problem would often be flagged.
If bbr wanted to give an error in most cases, could it abort if there is a $PRIOR
record and not any informative record names? That'd hopefully not have false positives, and I think it might catch most cases, though it would miss some (e.g. subroutine used instead of prior record).
Or it's perhaps worth considering reworking things so that jittering is coupled to estimates from a previous model. You could still use nmrec::extract_*
to get information to intersect, but the core init values could come from a previous model, and you could reliably error on a shape mismatch. However, I guess that's probably being avoided in anticipation for cases where "tweak estimates" might be used to blow out a series of models for assessing sensitivity of the results to the initial values, in which case requiring a finished model as the starting point would be a bit odd (but maybe not too bad in practice).
It's perhaps worth emphasizing in the docs that these are tools meant to help modelers modify the control stream, but they should review the result very carefully, as if they were editing the model by hand or reviewing changes a person was proposing. (I haven't taken a look at the documentation that exists, so apologies if something along those lines is already there.)
Thanks @kyleam I appreciate those thoughts. You said
IIUC the common workflow here is "inherit_param_estimates then tweak", in which case the first step would flag control streams that are using non-informative names. So in practice it seems to me that this problem would often be flagged.
I actually had this same thought this morning. That's reassuring, and I think will likely cover a lot of cases.
To be clear (for anyone else reading this), we're talking about a pattern like this, which several people have mentioned wanting to be able to do:
mod2 <- mod1 %>%
copy_model_from() %>%
inherit_param_estimates() %>%
tweak_initial_estimates()
In that case, if mod1
was using non-informative priors, the error I mentioned above would be triggered by the inherit_param_estimates()
call, before we even get to the tweak_initial_estimates()
function that we're discussing here.
We're going to discuss with a few scientific folks offline. Unless they object, I'm leaning towards this
documenting that this feature is not meant to be used with uninformative prior names, accepting that it will jitter them if there
I had said
We're going to discuss with a few scientific folks offline. Unless they object, I'm leaning towards
documenting that this feature is not meant to be used with uninformative prior names, accepting that it will jitter them if there
Unfortunately, @timwaterhouse and @curtisKJ do object. Essentially, they said we definitely don't want to tweak the priors. Two things we're going to look into:
FIX
for a parameter record to function as non-informative prior. If the answer is "yes", then this solves our problem, because we don't tweak those values anyway.$PRIOR
block is sufficient and, if not, what else we would need to check.@barrettk I believe you're taking this ^ on, right?
Unfortunately, @timwaterhouse and @curtisKJ do object. Essentially, they said we definitely don't want to tweak the priors.
The proposal isn't that anyone wants to tweak them. It's just accepting that the caller can violate the documented use in a way that leads to them being tweaked. Was the conclusion "we're going to insist that the outcome is severe enough that we really want to prevent that mistake"?
Two things we're going to look into:
Perhaps it's worth considering rethinking the design to always use an executed model as the starting point (my "reworking things so that jittering is coupled to estimates from a previous model" bullet point above). That'd give you a reliable answer to what the shape should be.
Just a few comments, in case it's helpful...
Look at the NONMEM docs to verify whether you need to specify
FIX
for a parameter record to function as non-informative prior. If the answer is "yes", then this solves our problem, because we don't tweak those values anyway.
At least based on the nwpri docs, it looks like that could be the case. I see a lot of "values on these records must be fixed", but perhaps a closer look would show cases where that's not said or perhaps there are other more relevant docs.
[...] about whether the
$PRIOR
block is sufficient
My conclusion that the prior block isn't sufficient is based on the nwpri docs saying "a $PRIOR record may be used instead of a user-written PRIOR subroutine, in which case the input arguments listed above may be specified as options of $PRIOR".
In response to @seth127's comment:
Look at the NONMEM docs to verify whether you need to specify FIX for a parameter record to function as non-informative prior. If the answer is "yes", then this solves our problem, because we don't tweak those values anyway.
Seth and I talked offline about this. It is still something worth looking into, but we decided it isnt really a complete solution, as we want to be able to identify prior records in order to remove them from initial_estimates()
output. This may not be totally feasible, but the motivation for this is strong, and it was determined that we should see what can be done here.
In response to @kyleam's comments:
The proposal isn't that anyone wants to tweak them. It's just accepting that the caller can violate the documented use in a way that leads to them being tweaked. Was the conclusion "we're going to insist that the outcome is severe enough that we really want to prevent that mistake"?
The conclusion is that we needed to guarantee priors wouldnt be altered, even using the old method. It's apparently a large deal so we need some method of determining if priors are being used at the minimum. Identifying the specific records that are priors is ideal, but we need to at least know if they're being used.
Perhaps it's worth considering rethinking the design to always use an executed model as the starting point (my "reworking things so that jittering is coupled to estimates from a previous model" bullet point above). That'd give you a reliable answer to what the shape should be.
The jittering feature has to be separate from the inherit_param_estimates()
function unfortunately, as we want tweak_initial_estimates()
to be usable on its own. Ideally we dont want to require it to be a previously run model. There have been discussions on creating a wrapper around tweak_initial_estimates()
to quickly create a bunch of new models with different initial estimates.
$PRIOR
block, need to look more into the other). I think if we could look for the presence of those two blocks, and make as many inferences as we can (with good documentation on its limitations/warnings), that would be the most preferable option by far.I think if we could look for the presence of those two blocks, and make as many inferences as we can (with good documentation on its limitations/warnings), that would be the most preferable option by far.
It seems to me that that line of thinking is in direct contradiction to claiming you must "guarantee priors wouldnt be altered".
The jittering feature has to be separate from the
inherit_param_estimates()
function unfortunately, as we wanttweak_initial_estimates()
to be usable on its own
I understand that (I think, at least). My suggestion is that you reconsider that design decision due to the desire to reliably know if there is a shape mismatch due to prior use. If we decide not to go that direction, that's of course fine, but here it feels like you're just dismissing the suggestion by reasserting what's already a given.
It seems to me that that line of thinking is in direct contradiction to claiming you must "guarantee priors wouldnt be altered".
Perhaps I didnt provide enough detail here. We need to know if there are priors with certainty (so worst case we could error out and not support it). The line you referenced would be the ideal state, and would handle almost all cases of being able to filter out priors. It's fine if we cant guarantee the identification of specific prior records.
I understand that (I think, at least). My suggestion is that you reconsider that design decision due to the desire to reliably know if there is a shape mismatch due to prior use. If we decide not to go that direction, that's of course fine, but here it feels like you're just dismissing the suggestion by reasserting what's already a given.
It's more that this is not the direction we want to go first. That is something we would consider if parsing priors seems too finicky/unreliable, but we want to attempt that direction first. We could only go that direction if we required a previously executed model run to pull out previous estimates, and there is at least a notable preference to not do that based on my discussions with Seth, Curtis and Tim (easier to have these discussions over zoom). That's something I can discuss with @seth127 next week when he gets back, but for now I have directions to look into the parsing of prior records.
I talked with @barrettk for bit yesterday and I have a proposed path forward that I'd love some feedback on. I'll say upfront that my main motivation here is not wanting to try to identify and filter out the non-informative priors. Given discussion above, and some further research, this seems like a huge pain. I also don't like the idea of making this feature rely on a previously run model. (I'm happy to go into detail on either of those points, if anyone is interested.)
So then, my proposal:
nwpri.utl
page in NM75 manual, which says "these records must be fixed" in three different places.)initial_estimates()
will return them as if they're regular parameters) and suggest they switch to informative priors.Thoughts on that? Anything that I'm missing?
Here is some pseudo-code I threw together with the heuristic I have in mind:
Then we call that in initial_estimates()
(maybe here) with a message something like this:
if (isTRUE(using_old_priors(ctl))) {
rlang::warn(paste(
"This model appears to be using the old 'non-informative' method of specifying priors.",
"That will cause this function to extract and display priors as if they are initial parameter estimates.",
"Consider changing the model to use informative prior record names (such as THETAP and THETAPV)."
))
}
and then we go on with our day...
Still working on some matrix stuff before I commit the latest changes, but here are some examples of getting the initial estimates, based on some of the above discussion (including @seth127's most recent comment):
cc @kyleam to check out this modified function as well as Seth's comment above
@seth127
So then, my proposal: [...] Thoughts on that? Anything that I'm missing?
Sounds like a reasonable path forward to me.
@seth127 Your proposal looks reasonable to me as well. My only suggestion would be to change the language for "informative" and "non-informative" record names, since that has specific meaning for Bayesian priors. I know the NONMEM documentation calls them that, but I think something like this might be more clear:
if (isTRUE(using_old_priors(ctl))) {
rlang::warn(paste(
"This model appears to be using $THETA, $OMEGA, and/or $SIGMA to specify priors.",
"That will cause this function to extract and display priors as if they are initial parameter estimates.",
"Consider changing the model to use the more specific prior record names (such as $THETAP and $THETAPV)."
))
}
@timwaterhouse The only issue with changing the naming convention, is that nmrec
also uses those names. I think we'd want those to be consistent. So if this is important I think we'd want to make the change in both places.
I agree with the suggestion to avoid "non-informative" and will update nmrec.
Getting initial parameter estimates
The goal was to return an object that looked similar to
param_estimates()
. The internal functionget_initial_est()
, returns all values (i.e. the full diagonally concatenatedOMEGA
matrix if multiple records are provided) in a list format, whereas the wrapperinitial_estimates()
, only displays values defined in the control stream file`initial_estimates()` example
```r MOD1 <- read_model( system.file("model", "nonmem", "basic", "1", package = "bbr") ) > initial_estimates(MOD1, flag_fixed = TRUE) # A tibble: 9 × 6 parameter_names record_type init lower_bound upper_bound fixedTweaking initial estimates
The details below are also found in the function docs:
Usage
Walkthrough
### Starting Record ``` ;Initial THETAs $THETA (0,,1) ;[LCLM2] ( 0.7 FIX) ;[LCLM] (0.67, 0.7, 0.72) ;[LCLF] ( 2 ) ;[CLAM] ( 2.0);[CLAF] ( 0.7 ) ;[LV1M] ( 0.7 ) ;[LV1F] ( 2.0 ) ;[V1AM] ( 2.0 ) ;[V1AF] ( 0.7 ) ;[MU_3] ( 0.7 );[MU_4] ( 0.3 ) ;[SDSL] ``` ### Tweak values ```r tweak_initial_estimates( .mod .p = 0.1, tweak = c("theta", "sigma", "omega"), digits = 2 ) ``` ### After Tweaking ``` ;Initial THETAs $THETA (0,,1) ;[LCLM2] # If no estimate is found, treat as fixed (skip) ( 0.7 FIX) ;[LCLM] # skip fixed parameters (0.67, 0.72, 0.72) ;[LCLF] # If tweaked value would be outside of one of the bounds, set to the bound ( 1.9 ) ;[CLAM] # Otherwise tweak the value by the sampled percentage ( 2.1);[CLAF] ( 0.76 ) ;[LV1M] ( 0.67 ) ;[LV1F] ( 2.2 ) ;[V1AM] ( 1.8 ) ;[V1AF] ( 0.73 ) ;[MU_3] ( 0.68 );[MU_4] ( 0.27 ) ;[SDSL] ```closes https://github.com/metrumresearchgroup/bbr/issues/632