-
Notifications
You must be signed in to change notification settings - Fork 2
Iss71 #72
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
base: main
Are you sure you want to change the base?
Iss71 #72
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,25 +4,27 @@ | |
| #' added using add_propensities()) | ||
| #' @param split A logical for if the metric should be calculated separately for | ||
| #' the training/testing split. Defaults to TRUE. | ||
| #' | ||
| #' @param group A list of variable names to group by | ||
| #' | ||
| #' @return A discrimination object with propensities (likely added using | ||
| #' add_propensities()) with discriminator AUC | ||
| #' | ||
| #' @export | ||
| #' | ||
| add_discriminator_auc <- function(discrimination, split = TRUE) { | ||
| add_discriminator_auc <- function(discrimination, group = c(),split = TRUE) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you have an example of using
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't have an example, group = NULL is better! |
||
|
|
||
| if (split) { | ||
|
|
||
| discriminator_auc <- discrimination$propensities %>% | ||
| dplyr::group_by(.data$.sample) %>% | ||
| dplyr::group_by(across(all_of(c(".sample", group))))%>% | ||
|
MortonC78483 marked this conversation as resolved.
|
||
| yardstick::roc_auc(".source_label", ".pred_synthetic") %>% | ||
| dplyr::mutate(.sample = factor(.data$.sample, levels = c("training", "testing"))) %>% | ||
| dplyr::arrange(.data$.sample) | ||
|
|
||
| } else { | ||
|
|
||
| discriminator_auc <- discrimination$propensities %>% | ||
| dplyr::group_by(across(all_of(group)))%>% | ||
| yardstick::roc_auc(".source_label", ".pred_synthetic") %>% | ||
| dplyr::mutate(.sample = factor("overall", levels = "overall")) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,11 @@ | ||
| #' Add pMSE to discrimination object | ||
| #' Add pMSE to discrimination object, with option to assess separately on groups | ||
| #' indicated by a grouping variable | ||
| #' | ||
| #' @param discrimination A discrimination object with propensities (likely | ||
| #' added using add_propensities()) | ||
| #' @param split A logical for if the metric should be calculated separately for | ||
| #' the training/testing split. Defaults to TRUE. | ||
| #' @param group A set of variables to group the pmse by | ||
|
MortonC78483 marked this conversation as resolved.
|
||
| #' | ||
| #' @return A discrimination object with propensities (likely added using | ||
| #' add_propensities()) with a pMSE | ||
|
|
@@ -12,27 +14,30 @@ | |
| #' | ||
| #' @export | ||
| #' | ||
| add_pmse <- function(discrimination, split = TRUE) { | ||
|
|
||
| add_pmse <- function(discrimination, group = c(), split = TRUE) { | ||
|
|
||
| calc_pmse <- function(propensities) { | ||
|
|
||
| # calculate the expected propensity | ||
| prop_synthetic <- propensities %>% | ||
| dplyr::summarize( | ||
| n_synthetic = sum(.data$.source_label == "synthetic"), | ||
| n_total = dplyr::n() | ||
| ) %>% | ||
| dplyr::mutate(prop_synthetic = .data$n_synthetic / .data$n_total) %>% | ||
| dplyr::pull("prop_synthetic") | ||
| dplyr::group_by(across(all_of(group))) %>% | ||
| dplyr::summarize("prop_synthetic" = list(c(sum(.data$.source_label == "synthetic")/dplyr::n()))) %>% | ||
| dplyr::pull(prop_synthetic) | ||
|
Comment on lines
+24
to
+26
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This new code is less clear than the original code imo. Can you just add |
||
|
|
||
| propensities_vec <- propensities %>% | ||
| dplyr::pull(".pred_synthetic") | ||
| dplyr::group_by(across(all_of(group))) %>% | ||
| dplyr::summarise(".pred_synthetic" = list(c(.pred_synthetic))) %>% | ||
| dplyr::pull(.pred_synthetic) | ||
|
Comment on lines
+29
to
+31
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I love nested data structures! This sections needs some comments. This is probably also easier with the |
||
|
|
||
| # function for pmse | ||
| pmse_func <- function(propensities_vec, prop_synthetic){ | ||
| mean((propensities_vec - prop_synthetic)^2) | ||
| } | ||
| # calculate the observed pMSE | ||
| pmse <- mean((propensities_vec - prop_synthetic) ^ 2) | ||
| pmse <- mapply(pmse_func, propensities_vec, prop_synthetic) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a blank line before
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We prefer pmse <- purrr::map2_dbl(
.x = propensities_vec,
.y = prop_synthetic,
.f = pmse_func
)
|
||
|
|
||
| return(pmse) | ||
|
|
||
| } | ||
|
|
||
| if (split) { | ||
|
|
@@ -45,21 +50,47 @@ add_pmse <- function(discrimination, split = TRUE) { | |
| dplyr::filter(.data$.sample == "testing") %>% | ||
| calc_pmse() | ||
|
|
||
| pmse <- tibble::tibble( | ||
| .source = factor(c("training", "testing"), levels = c("training", "testing")), | ||
| .pmse = c(pmse_training, pmse_testing) | ||
| ) | ||
| if (length(group)==0){ # original case | ||
| pmse <- tibble::tibble( | ||
| .source = factor(c("training", "testing"), levels = c("training", "testing")), | ||
| .pmse = c(pmse_training, pmse_testing) | ||
| ) | ||
| } | ||
| else{ # we have passed a list of grouping variables | ||
| groups <- discrimination$propensities %>% | ||
| dplyr::group_by(across(all_of(group))) %>% | ||
| group_keys() | ||
| groups <- rbind(groups, groups) # make 2 copies for train/test | ||
|
|
||
| pmse <- tibble::tibble( | ||
| groups, | ||
| .source = factor(c(rep("training", length(pmse_training)), rep("testing", length(pmse_testing))), levels = c("training", "testing")), | ||
| .pmse = c(pmse_training, pmse_testing) | ||
| ) | ||
| } | ||
|
Comment on lines
+53
to
+70
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code fails and it's unclear. Please rewrite using the tidyverse style guide, clearer spacing, and more comments. |
||
|
|
||
| } else { | ||
|
|
||
| pmse_overall <- discrimination$propensities %>% | ||
| calc_pmse() | ||
|
|
||
| pmse <- tibble::tibble( | ||
| .source = factor("overall", levels = "overall"), | ||
| .pmse = pmse_overall | ||
| ) | ||
|
|
||
| if (length(group)==0){ # original case | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You will need to switch to NULL |
||
| pmse <- tibble::tibble( | ||
| .source = factor("overall", levels = "overall"), | ||
| .pmse = pmse_overall | ||
| ) | ||
| } | ||
| else{ # we have passed a list of grouping variables | ||
| groups <- discrimination$propensities %>% | ||
| dplyr::group_by(across(all_of(group))) %>% | ||
| group_keys() | ||
|
|
||
| pmse <- tibble::tibble( | ||
| groups, | ||
| .source = factor(c(rep("overall", length(pmse_overall))), levels = c("overall")), | ||
| .pmse = pmse_overall | ||
| ) | ||
| } | ||
|
Comment on lines
+78
to
+93
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment as above. |
||
| } | ||
|
|
||
| discrimination$pmse <- pmse | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.