eddatasci / unrollment_proj

The Unrollment Project: Exploring algorithmic bias in predicting bachelor's degree completion.
5 stars 0 forks source link

Issue 16 #22

Closed btskinner closed 4 years ago

btskinner commented 4 years ago

Init and tested make_cv_df() function. It's a wrapper for the tidymodels resampling functions, taking arguments as needed. You can use NSE or quoted names for the functions.

e.g.

make_cv_df(df, mc_cv)
make_cv_df(df, mc_cv, times = 50)
make_cv_df(df, "mc_cv")
make_cv_df(df, vfold_cv, v = 20)

I've also included a testing framework. You can test the functions either by

## if in the top level directory
testthat::test_dir("./tests/testthat")

## or
setwd("./tests")
source("./testthat.R")
btskinner commented 4 years ago

Adding more tests for utility functions and setting up Travis-CI per #11. When it runs, I will probably just merge unless @wdoyle42, you have an issue with my doing that.

wdoyle42 commented 4 years ago

@btskinner do you think we actually need this above and beyond the existing resample methods from rsample? I know I filed this issue, and I appreciate the time you spent on it.

btskinner commented 4 years ago

What do you mean by this? The function make_cv_df()? Probably not, since it's mostly just a wrapper for the various rsample methods. I just thought that's what you wanted based on #16. I guess the benefit is that it obscures some of the R code munging we might have to do if we wanted to be able to change resampling methods on the fly with a macro. E.g.,

resample_method <- "mc_cv"
make_cv_df(resample_method)

resample_method <- "vfold_cv"
resample_args <- ...
make_cv_df(resample_method, resample_args)

All the Travis check errors right now are b/c Travis doesn't build rstan and rstanarm well. Since these are requirements of tidymodels, I'm getting these errors. I can stop checking for a moment and we can merge w/o issue. But that's if you think the function is useful. If not, can you say more about what you meant?

wdoyle42 commented 4 years ago

@btskinner --I'm basically questioning whether I should have opened the issue in the first place. Based on what you put together, what do you think?

btskinner commented 4 years ago

@wdoyle42, hah! I didn't spend that much time on it. But it's done and contrary to the Travis CI messages, it works. Here's what I see to be the pros and cons:

Pros

Cons

It will not bother me not at all if we ditch this, though I would keep the branch in case we change our minds.

btskinner commented 4 years ago

And now Travis CI works (and checks pass). That said, @wdoyle42, we still don't have to use this function.

btskinner commented 4 years ago

@wdoyle42, any more thoughts on this?

wdoyle42 commented 4 years ago

This works for me-- and this can be integrated into the workflow for #23 .