-
Notifications
You must be signed in to change notification settings - Fork 7
updates from utils/update.r scripts, adding CI paramteres to ei_iter,… #150
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
Conversation
… enabling parallel results for ei_rxc, and creating visualization functions named rpv_toDF() and rpv_coef_plot()
rachel-carroll
left a comment
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.
Hey Sean! This looks good, just a few comments below. Also did you run devtools::check()? I think it should have updated documentation if you did which I don't see.
R/rpv_coef_plot.R
Outdated
| @@ -0,0 +1,62 @@ | |||
| rpv_coef_plot <- function (rpvDF = NULL, | |||
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.
you need to add the documentation stuff at the beginning. If you look at some other .R function you will see they all start with descriptions of the function overall and the arguments and other parameters. For example @export tag which makes it actually become a seen function of the package for users. Just follow the format in other .R files
…t and rpv_toDF, moved example_rpvDF for test of new functions
rachel-carroll
left a comment
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.
Thanks! looks basically ready, just asked to bring back some of the helpful code comments and some formatting.
R/rpv_toDF.R
Outdated
| #' } | ||
|
|
||
|
|
||
| rpv_toDF <- function (rpv_results = NULL, model = NULL, jurisdiction = "", |
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 looks mostly right, one note on preference is that the formatting (spacing and indentation standards) and comments in the eiExpand function are very helpful to keep. I think the only substantive code change was changing #library(eiExpand) to #library(eiCompare) in the commented out documentation examples. Is that right? The rest can match eiExpand i think
| ci_lower <- 0.025 | ||
| ci_upper <- 0.975 |
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.
not as familiar with the CI here but do these defaults need to change or become arguments?
R/rpv_toDF.R
Outdated
| colnames(rpv_data) <- colnames(rpv_data) %>% stringr::str_to_lower() | ||
| newcols <- gsub("mean", "Estimate", colnames(rpv_data)) | ||
| # Handle both ci_95_lower and ci_lower naming conventions | ||
| newcols <- gsub("ci_95_lower", "Lower_Bound", newcols) |
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.
Can probably drop these since the names will always be ci_lower ci_upper now without the _95, right?
| chains_pr[, race_indices] <- race_pr | ||
| } | ||
|
|
||
| # Get point estimates and standard errors |
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 strongly prefer to keep these comments. The substantive changes will be clreaer and its helpful to have well commented out code
| #' @return A nicely formatted dataframe for printing results | ||
| #' @export | ||
| summary.eiCompare <- function(object, ...) { | ||
| objects <- list(object, ...) |
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.
remove ... argument in function definition
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.
just fyi looked at this error and its right that the ... has to stay bc its a method versus its own function. Thanks for catching that!
rachel-carroll
left a comment
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.
one more comment! then should be ready to go.
| tidyr::pivot_longer( | ||
| cols = grep("\\.", | ||
| ".value"), names_pattern = "(.*)\\.(.*)", names_repair = "unique") | ||
| plotDF$Voter_Race <- gsub("^pct_", "", plotDF$Voter_Race) | ||
|
|
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 looks a little funky some of it got cut off i think. shoudl be
plotDF <- rpv_data %>%
dplyr::mutate(Model = model,
Jurisdiction = jurisdiction,
Election_Type = election_type,
Year = as.numeric(year),
Contest = contest,
Candidate = candidate,
Party = party,
Preferred_Candidate = preferred_candidate
) %>%
tidyr::pivot_longer(
cols = grep("\\.", colnames(rpv_data), value = TRUE),
names_to = c("Voter_Race",".value"),
names_pattern = "(.*)\\.(.*)",
names_repair = "unique"
)
| #' @examples | ||
| #' \donttest{ | ||
| #' #library(eiCompare) | ||
| #' #data("south_carolina") |
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.
darn found one more thing. This data is not in eiCompare its in eiExpand. You can see if you can use something from eiCompared internal data. Or move south carolina over to eiCompare.
R/rpv_toDF.R
Outdated
| #' # race_cols = c('pct_white', 'pct_black'), | ||
| #' # totals_col = "total_vap" | ||
| #' #) %>% | ||
| #' # rpv_normalize( |
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.
now getting an error on this bc rpv_normalize is in eiExpand. I think just remove this part
… enabling parallel results for ei_rxc, and creating visualization functions named rpv_toDF() and rpv_coef_plot()
Thanks for opening a PR!
Please list any Issues this PR addresses
updated the functions in the updates/utils files