Closed mjskay closed 3 years ago
Thanks for the PR and thorough explanation of changes @mjskay ! Very much appreciated!! I should be able to submit to CRAN before the end of the week (I'll try ASAP)
Awesome, thanks!
Also, looks like I forgot to update the docs after these changes, so I did that :)
@mjskay still some documentation issues, are you able to correct?
Ah yup, that's what I get for not running check locally, should be fixed now :).
@mjskay submitted to CRAN just now. Thank you for being so helpful with the PR. I'll update you when it's accepted.
Regarding fitted_draws()
I'll wait until the next update to decide. I really see the value in delineating with epred_draws()
and linpred_draws()
but my current collaborators will prefer fitted_draws()
... at least at first. I will probably end up supporting both.
Thanks again!
thanks so much!
@mjskay on its way to CRAN, thanks again :smiley:
wonderful, thank you!
Hi! tidytreatment looks like a great package; it's really awesome to see people building on the tidybayes API :)
Some slightly unfortunate news: I am about to submit a new version of tidybayes to CRAN that changes some parts of the tidybayes API that will cause some test failures on the existing version of tidytreatment on CRAN. The good news is this PR will fix those failures.
The changes in tidybayes that are relevant here are:
XXX_draws()
functions (fitted, predicted, etc) is nowobject
instead ofmodel
, as these typically call down to posterior_predict / posterior_epred / posterior_linpred, and those functions also useobject
. In some corner cases this was causing problems (specifically, rstanarm'sposterior_predict
has anm
argument that could not be passed to it viaadd_predicted_draws()
due to partial argument matching thinking the user was specifyingmodel
). This PR renamesmodel
toobject
in your implementations ofresidual_draws
andpredicted_draws
. It does not changefitted_draws
for reasons outlined below.value = ".value"
,residual = ".residual"
,prediction = ".prediction"
) are now all calledvalue
(the default column name is still the same for each function though --- e.g. the default isvalue = ".residual"
for residual_draws). In retrospect having this argument have a different name for each function was confusing. This PR renames the relevant columns tovalue
in your implementations ofresidual_draws
andpredicted_draws
. You shouldn't have to worry if you have existing users using the old column names; thetidybayes
generic for those functions automatically passes through the desired column name to thevalue
argument and gives the user a deprecation warning if they use the old argument name.n
argument is renamed tondraws
. This is not a huge change since you don't implement it. This makes it more consistent with naming schemes in the new posterior package, and also prevents corner cases wheren
is partially matched to thenewdata
argument (really, partial argument matching is just a terrible feature in R).Your test suite passes under this PR if either the CRAN version of tidybayes or the github version is installed.
There's another big change I did not try to address in this PR as it does not currently cause any test failures and I'm not sure how you'd want to handle it: as of the next version,
tidybayes::fitted_draws()
will be deprecated. At least, the default implementation is --- after much experience teaching folks how to use the package, I've come to the realization that "fitted" is a terrible name for the function that only serves to confuse students. Users are now directed to use eitherepred_draws()
(which gives draws from the expected value of the posterior predictive; afterrstantools::posterior_epred()
) orlinpred_draws()
(which gives draws from the linear predictor; afterrstantools::posterior_linpred()
). I'd suggest a similar change here, but of course you're welcome to ignore my advice :). Tidybayes will continue to export thefitted_draws()
generic and the deprecation warning is only hit infitted_draws.default()
, so if you choose to keep using thefitted_draws
name everything should continue to work and your users should not see a deprecation warning.I will be making a submission to CRAN soon (probably later this week). If you're able to submit a new version before I do that would be awesome --- it should save you an angry email from CRAN telling you to update the package due to test failures. Sorry to be the bearer of such news, but I hope having a PR attach to the request will make it easier to deal with :). Happy to try to help / answer questions as needed.