export functions for setting init values #18
Conversation
theta can take the form "(low,,up)", with optional white space between the two commas. parse_theta_paren() botches its "has white space?" check because, when at the first comma, it looks for a sequence of two white space elements (something the parser would never produce) rather than a white space element followed by a comma. This leads to an init option that uses the up value. Look for a comma in the second position, as intended.
The values(diag,odiag) form for omega and sigma is stored as an option
with "(diag,odiag)" as the value. For the upcoming set_{omega,sigma}
feature, we need to be able to set diag and odiag individually.
Parse values(...) into a nested option with diag and odiag options.
These will be used in the upcoming set_{omega,sigma} feature, which
needs to map values from input matrices to the init values in the
parsed records.
The upcoming parameter indexing feature is going to need the core logic of get_record_option() in a few spots, but for internal code there's no reason to go through the name resolution. Pull the logic out into an internal function.
The new function create_param_index() will be used by the upcoming
set_{theta,omega,sigma} functions.
One of the original motivations for nmrec was to support a feature in
bbr where initial estimates for a new model are inherited from the
final estimates of a previous (typically parent) model.
To make that possible, add set_theta(), set_omega(), and set_sigma()
functions that accept a full parameter value---which is the form that
would come from calling bbr::get_{theta,omega,sigma} on the parent
model---and then map each of those values to the corresponding option
in the parsed records.
Two constraints simplify things from the design standpoint:
* only support updating parameters that are explicitly written in the
control stream
This makes it much easier to stick close to the original form
because we don't have to drastically alter the control stream and
instead are (mostly) working with options that are already there.
And that also means we don't need complex logic for determining if
two representations are the same, and deciding what's best to use
in a given context.
* only accept the _full_ parameter
Even though we only update the explicit values, we always require
the full parameter to be passed, so we don't need provide an
explicit interface for selecting a sub-part of vector or matrix.
Note, however, that the caller can still effectively update a
subset of the explicit values by setting some input values to NA.
Closes #9.
|
@kyleam I think we may want different handling when keeping the bounds. If two values are found, I would think we'd want to add a middle value as opposed to replacing the higher value: Keeping Bounds or adding new initial value> test_case$input_nmrec
$PROBLEM theta bounds - maintain bounds; add starting value if not present
$THETA
(0, 2) ; KA
(0, 3) ; CL
(0, 10) ; V2
(0.02) ; RUVp
(1) ; RUVa
(0, 0.5, 1) ; F
(-10, -3.92E+00, 10)
> test_case$replacement
[1] 1 2 3 4 5 6 7
> nmrec::set_theta(test_case$input_nmrec, test_case$replacement)
> test_case$input_nmrec
$PROBLEM theta bounds - maintain bounds; add starting value if not present
$THETA
(0, 1) ; KA
(0, 2) ; CL
(0, 3) ; V2
(4) ; RUVp
(5) ; RUVa
(0, 6, 1) ; F
(-10, 7, 10)Referring to the above example, I think we'd want to return the second record: Discarding BoundsAdditionally, it would be ideal if we could maintain parentheses and any other formatting when discarding the bounds (second record in image below again): > test_case$input_nmrec
$PROBLEM theta bounds - replace with single value
$THETA
(0, 2) ; KA
(0, 3) ; CL
(0, 10) ; V2
(0.02) ; RUVp
(1) ; RUVa
(0, 0.5, 1) ; F
(-10, -3.92E+00, 10)
> # Copy Thetas
> nmrec::set_theta(test_case$input_nmrec, test_case$replacement, bounds = "discard")
> test_case$input_nmrec
$PROBLEM theta bounds - replace with single value
$THETA
1 ; KA
2 ; CL
3 ; V2
(4) ; RUVp
(5) ; RUVa
6 ; F
7
|
|
I looked over this a fair amount, but alone and with @barrettk , and I think it's looking great. I like how the test cases are set up, and they all made sense to me. @barrettk is going to take a look at the test cases he was using before to see if there are any others we might consider adding. He is also going to begin wiring this up to I had two other thoughts to discuss as well:
Thanks again. Great work on this. |
The two-value form with one comma is
Why you think that would be ideal? This behavior (dropping parens if we've reduced a multi-value options down to one value) comes with ab7fab1. As mentioned in that message, this applies to cases where we are dropping things at the request of a non-default option, so I think it's reasonable to simplify the result. Given that either way we go in some cases the result may or may not be consistent with neighboring single-value items, I lean toward giving the simplified result. However, if you and @seth127 prefer retaining the parens in these cases, I'm okay with ejecting ab7fab1 from this series. |
Sure, that sounds good to me.
Were those examples NONMEM code from projects, or the examples that ship with NONMEM/bbr? Is there a reason why someone would prefer the use the old form instead of the more informative names? I don't think there is, in which case my view (84a591b) is that it's not worth the added complication and potential for bugs.
Even accepting that we can rely on prior records, that seems like non-trivial logic that we'd be better off not adding unless there was a strong reason why someone shouldn't just use the informative names. (However, as far as I understand, a PRIOR subroutine can be used instead of a PRIOR record, so looking at the PRIOR record doesn't cover everything.) If someone wants to use |
Ahh I think I misunderstood this specification, though it sounds familiar from years ago when I was first learning about NONMEM. I was thinking the bounds were (low, hi) if two values were provided, but I confirmed the two-form you mentioned.
I would have said it would make the tests nicer/easier using the framework we made in
Im slightly conflicted here. In terms of where I saw the examples involving the older method of specifying priors, I saw a notable number of examples. There are also cases where your error would trigger, but priors are not used. Here is one such example, and related test we had. This is because the second |
Simplification is referring to reducing From a user standpoint, if you want to say
Given we're touching the value anyway, I think it makes sense to go with the simplified form (but I'm just repeating myself). If others share your preference, we can drop ab7fab1.
I'd rather remove the "drop parens" simplification entirely than bloat the interface for a cosmetic detail. |
In real project code or NONMEM examples?
I'm not convinced that it's a problem to tell them to switch to the the informative names if they want to use the "inherit inits" feature. |
|
With regards to the parentheses simplification, I'll defer to Seth. I see the argument for and against it, and would honestly lean towards what Seth thinks scientists would prefer/expect (but dont think it's significant enough to require scientist feedback)
Both. Im not sure I could back track to see where I got specific examples from, as I just went through the repos I had to find example cases I had seen before. I think could be something worth asking @callistosp or someone about to get a better feel for how frequent this is seen in:
Im of the opinion that if we determined/ballparked ~10% of older models and 20% of client models use the older method, it's worth supporting the older method.
I think if scientists ran into it somewhat frequently, it would be better to support it. I know we develop these packages with mostly our scientists in mind, but given that we've given |
|
My two cents:
|
|
@callistosp Thanks so much for your feedback! Discussing with Seth later today and that will be helpful in coming to a decision. |
Control streams can use additional $THETA, $OMEGA, and $SIGMA records
to define priors rather than the more informative record names (e.g.,
$THETAP and $THETAPV). In this case the set_{param} functions will
fail when fed a correctly sized parameter.
create_param_index() could look at the $PRIOR record and discard some
records when indexing, though that would still miss the case where
PRIOR subroutine is used instead of $PRIOR record.
Supporting the use of overloaded $THETA, $OMEGA, and $SIGMA records
isn't worth the complication given that using the informative record
names seem to be best practice and transitioning to them is easy.
Instead just mention this known incompatibility in the error message.
The values for the set_* functions will typically be floating point numbers (often the final estimates from a previous NONMEM run). The default as.character() behavior uses 15 significant digits, which is too many in the context of initial estimates. For this reason, param_set_value() formats the value with "%.3G". That's probably good for most cases, but expose the format string as an option to let callers override it.
A common source of values for the set_* function will be the final estimates from another model. Regardless of how the model for that run specified OMEGA/SIGMA, NONMEM outputs variances and covariances, so care must be taken to not feed those values into a control stream that specifies something other than variance/covariance. Add an option that lets the caller tell set_omega() and set_sigma() that options for non-default representations should be ejected from the modified records/options. Go with two modes rather than a boolean option because it's not clear whether we'll end up needing to support more variants.
Go with two modes rather than a boolean option because it's not clear whether we'll end up needing to support more variants.
Both discarding theta bounds ("(low,init,up)" => "(up)") and dropping
alternative representations from matrix options (e.g., "(SD 1)" =>
"(1)") can lead to a result that no longer needs parentheses.
In general, the set_* functions try to retain all of the original
control stream formatting when updating the init values. However, in
the cases above, we're already dropping values at the request of a
non-default option, so it seems reasonable to simplify the result.
Note that the parentheses as left as is when updating a single initial
estimate surrounded by parentheses.
The motivation for "getting things in and out of higher-level forms"
was the functionality now covered by the new set_{theta,omega,sigma}
functions. Those didn't end up needing to construct higher-level
forms from a record, and instead just rely on a map between parameter
space and the record options.
There was a problem hiding this comment.
This is all looking good to me at this point. I think we have agreement on both points discussed above:
- Don't support old method of specifying priors
- Drop parens when no longer needed
@kyleam @barrettk did either of you have any outstanding comments or anything on this? If not, I think we're good to merge.
|
In terms of next steps here, I think we probably want a release sooner than later here, right? My motivation is that we'll want to gate the |
No, from my standpoint this is ready to merge. (I'll push a dev version bump on top before merging and tag.)
Yes. I'll plan to do a release this week. There are one or two small things I may try to squeeze in (not functional changes), but that won't hold things up.
A version gate is fine with me. Even better in my view would be gating on the presence of one of these functions, but I know we do the version gate plenty.
I'd say you can move forward in terms of getting that CI wired up to test against this version and merging the PR. It's just that a bbr release is held up by this actually being released. |
|
Just to echo what Seth mentioned - this all looks good to me. Feel free to merge whenever |
This series implements the nmrec-side for the bbr "inherit inits" feature (metrumresearchgroup/bbr#582, metrumresearchgroup/bbr#608). It adds three functions for setting parameter init values:
set_theta,set_omega, andset_sigma.Series overview
Patches 1-4 are prep work. Functionality-wise, patch 3 is the most interesting. It adds some general helper functions that will be used for working the OMEGA and SIGMA input and creating the map from matrix indices to the record options.
This adds the main behind-the-scenes logic for "indexing" parameter records (gathering the required info and making the parameter space to option map). The docstring of
create_param_indexis the best place to start to get a high-level overview of what's going on there.Patches 6-11 introduce the user-exported functions and make various extensions. Patch 9 is probably the one that deserves the most attention (and is something we haven't discussed), but of course any comments on the interface overall would be very appreciated.
Housekeeping.
I'm marking this as a draft because I'd like to go over it with fresh eyes (especially the docs, which I haven't read through again) and will likely reroll it with minor changes. However, please feel free to start to take a look and comment. @barrettk it's in a state where I think you can begin wiring it up to bbr whenever you're ready.
cc @seth127
Closes #9.