extract_rxc_precinct function #155
Conversation
rachel-carroll
left a comment
rachel-carroll
left a comment
There was a problem hiding this comment.
Hey Sean, Thanks, this is nice functionality! It ran fast and works. I suggested another method thats more simplified using dplyr. LMK what you think about that and thoughts on versioning.
PS "nit" just means its a nit picky small comment thats not really a big deal. learned that from ppl on my team.
R/extract_rxc_precinct.R
Outdated
| #' # cand_cols = c("kemp", "abrams", "metz"), | ||
| #' # race_cols = c("white", "black", "other"), |
R/extract_rxc_precinct.R
Outdated
| #' @param cand_cols Character vector of candidate column names (e.g., c("pct_cand_A", "pct_cand_B")) | ||
| #' @param race_cols Character vector of race column names (e.g., c("pct_black", "pct_white")) | ||
| #' @param dat Original data frame used in ei_rxc() call | ||
| #' @param uniq Column name for precinct identifier (must exist in dat) |
There was a problem hiding this comment.
maybe call this something more intuitive like precint_id_col
R/extract_rxc_precinct.R
Outdated
| #' @param eivote ei_rxc() output object containing stat_objects | ||
| #' @param cand_cols Character vector of candidate column names (e.g., c("pct_cand_A", "pct_cand_B")) | ||
| #' @param race_cols Character vector of race column names (e.g., c("pct_black", "pct_white")) | ||
| #' @param dat Original data frame used in ei_rxc() call | ||
| #' @param uniq Column name for precinct identifier (must exist in dat) |
There was a problem hiding this comment.
nit: in the documentation wrap any code word with `` so the formatting looks good. for example in the first one do ei_rxc() in the second `c("pct_cand_A", "pct_cand_B")`, in the last `dat`. There are a few others. Really whenever you're referring to a specific variable, function, or bit of R code syntax.
README.md
Outdated
| # eiCompare 3.0.6 | ||
|
|
||
| ## New function | ||
|
|
||
| * included extract_rxc_precinct() function to extract precinct level estimates from ei_rxc() | ||
|
|
||
| # eiCompare 3.0.5 | ||
|
|
||
| ## Package changes 10/27/25 | ||
|
|
||
| * added add_rpv_normalize and removed wru dependency | ||
|
|
||
| # eiCompare 3.0.4 |
There was a problem hiding this comment.
I didnt bump the package version for wru and rpv_normalize. Instead can you add rpv_normalize() to the first bullet of the 3.0.4 section? Then also add a bullet for removed wru dependency to that section.
If you want to bump the version for this new function then it can be 3.0.5. You will also need to update the DESCRIPTION file with the new version and rerun devtools::check() so documentation is updated.
Next time we chat we should think through the strategy we want to take for when to bump versions and if/when we want to resubmit to CRAN
|
|
||
| # Extract Beta matrix (MCMC iterations × beta parameters) | ||
| Beta <- eiMD_object$draws$Beta | ||
|
|
There was a problem hiding this comment.
I was messing with using dplyr functionality, which I tend toward when possible over base R matrix stuff. I used this which pretty much does it. format is slightly different but you can tweak as needed.
beta_means <- Beta %>% colMeans()
result_df <- data.frame(
names = gsub('beta.','',names(beta_means)),
vals = beta_means
) %>%
tidyr::separate(
col = names,
into = c('race', 'cand', 'precinct'),
sep = "\\."
) %>%
tidyr::pivot_wider(
names_from = c(race,cand),
values_from = vals
)
It simplifies the code and also makes it so the user doesn't need to specify the race and cands and the other args, it would just be the rxc result object. This assumes that the naming convention will always be beta.<race>.<cand>.<precinct> which I think is right based on your code?
If you want the user to be able to specify which cands and races are in the output you can keep those args and tweak this statement to filter on them. I kind of like it clean and just have it include all and the user can just subset results if they want. But thats really just an operational preference and I defer to you. If there are runs with a lot of cands and races it would mean an output with a ton of columns so I can see wanting to keep the args.
There was a problem hiding this comment.
Hey good suggestions throughout. I think we should defer on the dplyr functionality, just because we did some extensive stress testing/testing for edge case names and want the function to work with messier names. We could stress test your version, but I think we should defer that and get this up and running.
There was a problem hiding this comment.
ok thats ok for now. The next thing I want to do is make a bunch of package tests. Do you have some scripts of the stress testing you did? I can incorporate that in, which is important either way! Then if down the road we want to update this function we will have tests in place to make sure nothing breaks.
rachel-carroll
left a comment
rachel-carroll
left a comment
There was a problem hiding this comment.
just found 1 typo, other wise approved! so when you fix that feel free to merge.
NEWS.md
Outdated
|
|
||
| ## Package changes | ||
|
|
||
| * added add_rpv_normalize() function |
There was a problem hiding this comment.
remove add_, the function is just called rpv_normalize
|
|
||
| # Extract Beta matrix (MCMC iterations × beta parameters) | ||
| Beta <- eiMD_object$draws$Beta | ||
|
|
There was a problem hiding this comment.
ok thats ok for now. The next thing I want to do is make a bunch of package tests. Do you have some scripts of the stress testing you did? I can incorporate that in, which is important either way! Then if down the road we want to update this function we will have tests in place to make sure nothing breaks.
Thanks for opening a PR!
Please list any Issues this PR addresses
-adds a function (extract_rxc_precinct() extracting precinct-level estimates from the ei_rxc() function. It should handle various types of column names.