Add weights argument to gr(), for #1719
#1727
Conversation
along with unit tests.
|
Thanks! Looks pretty good already upon first glance. I quickly went over it and added some comments/questions. |
|
Thanks Paul- can you share a link to the added comments/questions? I don't see them anywhere (for example at the "Changed files" list ) after some searching in different places |
| # deduplicate weights vector (so length matches number of groups) | ||
| # and order the weights vector to match groups' assigned indices | ||
| distinct_J_indices <- !duplicated(J) | ||
| group_model_weights <- group_model_weights[distinct_J_indices] |
There was a problem hiding this comment.
Do we ensure that the weights is the same for all values within a group before subsetting?
| @@ -57,11 +67,17 @@ gr <- function(..., by = NULL, cor = TRUE, id = NA, | |||
| cor <- as_one_logical(cor) | |||
| id <- as_one_character(id, allow_na = TRUE) | |||
| by <- substitute(by) | |||
| gr_weights <- substitute(weights) | |||
There was a problem hiding this comment.
Would there be an issue with calling this weights rather than gr_weights throughout?
| ) | ||
| if (has_gr_weights) { | ||
| str_add(out$model_prior) <- glue( | ||
| " target += GMW_{id} * std_normal_{lpdf}(to_vector(z_{id}));\n" |
There was a problem hiding this comment.
Can you quickly elaborate again why this would be the right place to incorporate the weights? Just for me to double check the statistical logic.
| str_add(out$model_prior) <- cglue( | ||
| " target += std_normal_{lpdf}(z_{id}[{seq_rows(r)}]);\n" | ||
| ) | ||
| if (!has_gr_weights) { |
There was a problem hiding this comment.
Can this be an if else statement?
| @@ -295,6 +295,24 @@ data_gr_local <- function(bframe, data) { | |||
| J[is.na(J)] <- match(new_gdata, new_levels) + length(levels) | |||
| } | |||
| out[[paste0("J_", idresp)]] <- as.array(J) | |||
|
|
|||
| group_model_weights <- id_reframe$gcall[[1]]$gr_weights | |||
There was a problem hiding this comment.
this weights variable will not be needed in postprocessing since it only affects the model throught the prior (correct?). Can you check in validate_newdata if it would be possible to autoset weights variables to NA if missing in newdata?
| group_model_weights <- id_reframe$gcall[[1]]$gr_weights | ||
| if (nzchar(group_model_weights)) { | ||
| # extract weights from data as a vector (length equals number of observations) | ||
| group_model_weights <- str2formula(id_reframe$gcall[[1]]$gr_weights) |
There was a problem hiding this comment.
do you do any checks that the weights are non-negative? Or numeric more generally?
|
Sorry forgot to press "submit review" :-D |
This is a PR with draft implementation and tests of a
weightsargument ingr(), for the feature request in #1719. Would you mind reviewing Paul with any suggestions?In the Stan code and data, the group-level weights are denoted
GMW_{id}(for "group model weights")- I wasn't sure what the best way to name them in a way that fits the notation used elsewhere. Theweightsname in the Stan code is already used by the ad termresp | weights(w), and themm()weights use theW_syntax. So I wanted to pick a name for the group-level weights that was clearly different from the other existing names for different types of weights in the Stan code.