-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Get initial estimates and tweak initial theta estimates #646
Conversation
- Returns the initial estimates as defined in the control stream file - additional metadata, including which record the estimate was defined in, are also appended
- only theta records are currently supported, but omega and sigma will be added in a future PR
8211f79
to
c6f3ef0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great overall. I have a bunch of comments, but they're almost all very small things.
The one thing that I want to look into deeper next week is the adjust_theta_bounds
part. See my comment on this part of the docs above
#' --> set to value within bounds (first iteration: `(1-0.5)*0.9)`: (`(0, 0.45, 1)`)
I think we're actually good, but I want to walk through this with you to make sure we're on the same page with the algorithm that's been implemented and the test cases for it.
R/initial-estimates.R
Outdated
initial_est_df <- initial_est_df %>% dplyr::filter(!is.na(.data$init)) | ||
|
||
# Add original matrices as attributes if needed | ||
attr(initial_est_df, "omega_mat") <- suppressWarnings(get_initial_est(.mod)) %>% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor thing here, but why do we call get_initial_est()
again here (and two lines later)? Is there a reason we can't just pluck from the initial_est
that we get on line 28 above?
I'm also a little concerned with the supressWarnings()
wrapper. What's that about? Is there a specific warning we're suppressing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mostly did it as to only attach the matrix in this case (and not all the other attributes). I can rework this as to decrease the number of calls. The suppressWarnings was just to suppress prior
warnings, that would have already been displayed during the first call.
R/initial-estimates.R
Outdated
#' | ||
#' @inheritParams initial_estimates | ||
#' | ||
#' @keywords internal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small thing: let's start moving to using @noRd
instead of @keywords internal
. See message in fd3dedf for more context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds good for here. However, in some cases I do like generating helper docs for internal functions, in the event it makes it easier to use or recall how to specify things. Do you have any thoughts on still generating docs for some internal functions?
- removed one redundant test (the second one covered more, so delete the first) - better commenting for added clarity - order initial estimates based on row, to reflect how `param_estimates()` works - add test for same blocks, and expecting a warning when old non-specific priors are used
- informative --> specific
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great. A couple tiny clean up comments, but I think we're good to merge very soon.
|
||
it("theta bounds - adjust_tweaked_theta()", { | ||
# Note: the comments next to each theta record do not necessarily need to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I follow (and appreciate) the rest of this comment, but the first sentence...
the comments next to each theta record do not necessarily need to be true.
I'm not really sure what you're getting at here. I think we should probably remove this sentence, or reword to make it clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comments I had next to each record were originally true when I was setting the seed and used 3 digits. After making that test a loop those comments were no longer true. I thought they could add some clarity to how the function was intended to work, but I agree it sounds confusing/misleading. Opted to remove them.
- Add additional checks when testing the parsing of non-specific priors - removed some comments and added clarity to tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work here.
See previous discussions here: #635
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
If you would like to format OMEGA or SIGMA records as full matrices, they are stored as attributes:
Tweaking initial estimates
Usage
Walkthrough
Starting Record
Tweak values
After Tweaking
closes #632