-
Notifications
You must be signed in to change notification settings - Fork 108
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
tidy.mipo
and tidy.glance
#240
Conversation
|
For those who are interested in the problem of pooling |
Thanks or the reference. Looks interesting. I believe that commit 59f1ad4 wraps things up for this PR. Of course, I'm happy to change stuff if you find anything you dislike during code review. |
Vincent, thanks a lot. Almost there. Found three things:
Could you take a look it these? |
Forgetting to run I fixed problems 1 and 2 and force-pushed to keep the history a bit cleaner. W.r.t. problem 3, the issue seems to be that the imputed data produced by Here's the most minimal example I can produce. First, run this interactively from a fresh session to look at the seeds: imp <- mice::mice(mice::nhanes, maxit = 2, m = 2, seed = 1, print = FALSE)
imp$lastSeedValue[1:5]
#> [1] 10403 141 -705880619 1904110533 1951823516 Then, save this in a file called context("seed issue")
imp <- mice::mice(mice::nhanes, maxit = 2, m = 2, seed = 1, print = FALSE)
test_that('interactive and devtools::test seeds differ',{
interactive <- c(10403L, 141L, -705880619L, 1904110533L, 1951823516L)
expect_equal(imp$lastSeedValue[1:5], interactive)
}) Finally, open a fresh session of R, and run test-seed.R:7: failure: interactive and devtools::test seeds differ
imp$lastSeedValue[1:5] not equal to `interactive`.
2/5 mismatches (average diff: 5003)
[1] 403 - 10403 == -10000
[2] 135 - 141 == -6 This shows that the first 2 elements of imp$lastSeedValue are different when we run interactively in a fresh session or from Note that this problem persists if I checkout and install 9184f07, which is your last commit before I made any changes. I tried to find the problem, but I don't understand the internals of https://github.com/stefvanbuuren/mice/blob/master/R/mice.R#L397
|
context("seed issue")
set.seed(1L)
test_that('interactive and devtools::test seeds differ (2)',{
interactive <- c(10403L, 624L, -169270483L, -442010614L, -603558397L)
expect_equal(.Random.seed[1:5], interactive)
}) Test fails. See also https://stackoverflow.com/questions/56104176/devtoolstest-rngversion-and-sample-yields-different-results. I searched
|
I now found the possible culprit. In # set the random generator to V3.5.0 to ensure that this test
# passes in V3.6.0 and later
# see mail Kurt Hornik, dated 06mar19
# FIXME: consider using the new generator once V3.6.0 is out,
# at the expense of breaking reproducibility of the examples in
# https://stefvanbuuren.name/fimd/
suppressWarnings(RNGversion("3.5.0")) I expect updating this will solve it. No |
RNGs still feel a bit like black magic to me, so I'm glad you were able to track down the problem. Thanks for being so efficient and reactive. This PR was a breeze. And thanks for your work on mice more generally! |
This PR does:
pool.r.rsquared
to make sure we get the same results as before after I modify it.glanced
object tomipo
. This allows us to retrieve information about the original models stored inmira
.mipo
objects inpool.r.squared
. This is step 3 because we need access to the newglanced
object.glance.mipo
andtidy.mipo
methods.I have two technical questions for @stefvanbuuren :
pool.r.squared
, did you intend to hardcode the value "3", or should it bedf.residuals
? https://github.com/vincentarelbundock/mice/blob/tidymipo/R/pool.r.squared.R#L89mipo
objects, I don't have a good way to detect if the original model waslm
. Do you think that checking if there is anr.squared
column isglanced
is sufficient, or should I add a new column of information to glance, such asclass(object)[1]
? Or maybe the call should be saved as an attribute oftidy.mipo(object)
? Here's what I do now: https://github.com/vincentarelbundock/mice/blob/tidymipo/R/pool.r.squared.R#L73