Skip to content
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

Updates to parlmice + bugfix #107

Merged
merged 3 commits into from
Jun 20, 2018
Merged

Conversation

gerkovink
Copy link
Member

ADDED: check.dataform() and check.m outside of the parallel cluster
CHANGED: updated documentation to conform to mice()
CHANGED: added comments in the code
CHANGED: only necessary objects exported to computing cluster (more efficient)
FIXED: parlmice() arguments now properly parsed to the computing cluster [Thanks Teresa Lwin]

@gerkovink
Copy link
Member Author

gerkovink commented Jun 20, 2018

The below code did not run in mice 3.1.0, but runs as expected after the updates in #107

> set.seed(123)
> df <- boys
> meth <- make.method(df)
> pred <- make.predictorMatrix(df)
> visit <- sample(1:ncol(df))
> imp <- parlmice(df, method = meth, 
+               predictorMatrix = pred, 
+               visitSequence = visit, 
+               n.imp.core = 4, 
+               n.core = 7,
+               maxit = 3,
+               cluster.seed = 123, 
+               cl.type = "FORK")
> imp$pred
    age hgt wgt bmi hc gen phb tv reg
age   0   1   1   1  1   1   1  1   1
hgt   1   0   1   1  1   1   1  1   1
wgt   1   1   0   1  1   1   1  1   1
bmi   1   1   1   0  1   1   1  1   1
hc    1   1   1   1  0   1   1  1   1
gen   1   1   1   1  1   0   1  1   1
phb   1   1   1   1  1   1   0  1   1
tv    1   1   1   1  1   1   1  0   1
reg   1   1   1   1  1   1   1  1   0
> imp$meth
      age       hgt       wgt       bmi        hc       gen       phb        tv       reg 
       ""     "pmm"     "pmm"     "pmm"     "pmm"    "polr"    "polr"     "pmm" "polyreg" 
> imp$visit
[1] "wgt" "phb" "reg" "gen" "hc"  "age" "hgt" "tv"  "bmi"
> imp$chainMean[, , 1]
            1         2         3
age       NaN       NaN       NaN
hgt 90.875000 91.510000 90.705000
wgt 38.225000 37.425000 35.675000
bmi 17.584762 17.400476 17.323810
hc  53.606522 53.643478 54.141304
gen  2.159046  2.089463  2.099404
phb  2.449304  2.250497  2.244533
tv   6.858238  7.086207  6.908046
reg  3.000000  3.000000  3.000000
> imp$m
[1] 28

@stefvanbuuren stefvanbuuren merged commit c6e7c84 into amices:master Jun 20, 2018
@stefvanbuuren
Copy link
Member

Pull request is technically correct and merged. Some questions/suggestions:

  • Future integration with mice will probably be easier if arguments of parlmice and mice are in the same order
  • Some arguments that have defaults in mice do not have defaults in parlmice, which is somewhat inconsistent.
  • There is duplication of arguments and documentation. Is every argument necessary? Perhaps some could go over the dots argument.

@gerkovink
Copy link
Member Author

  1. I opted for the arguments with defaults first and empty arguments second. The help will otherwise follow the usual order, which may be confusing to new users. While experienced users are familiar with the arguments mice may take. Can change if your preference is consistency.
  2. mice has default missing arguments, which are challenging to parse to do.call in the computing cluster. Now all non-default parlmice() arguments are missing, unless otherwise specified by the user; then mice() runs on defaults, unless the user-specifies arguments.
  3. If missing arguments, then also documentation. dots does not work here as there are environments created within the computing cluster. See also (2). I am not sure there is a more elegant solution, but the current solution matches the parlmice call to mice and works like a charm.

@gerkovink
Copy link
Member Author

dots does not work here

I mean; it runs without error, but the arguments are not parsed to the deeper levels. It can be fixed by making the code dependent on the parent.frames() which seems much less elegant and more error-prone to me.

@stefvanbuuren
Copy link
Member

stefvanbuuren commented Jun 20, 2018

  1. What a user would want: replace call to mice(...) by call to parlmice(...), and trust it works. Best bet for that would be that arguments behave the same, as far as possible. Same order would surely help.
  2. If I understand correctly the arguments of parlmice will work in the same way as in mice. That's good.
  3. You could let roxygen2 copy automatically any documentation from mice that you don't specify, so as to reduce duplication. If the documentation needs to be different for parlmice, use something like "As in mice, but with the following changes in behavior: ...".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants