274 Return frequentist MMRM model object

[pengguanya]

#274

Fetch model for condmean. All tests past at local

[gowerc]

Have incorporated the update to use mmrm instead of glmmTMB.

I also decided to convert this from using attributes to instead using environments to store the model object. Whilst it is a bit of an anti pattern I think it simplifies the code quite a bit and avoids the need redefining attributes across multiple locations.

I still need to update the documentation and unit tests but @nociale would you mind skimming the changes to see what you think ?

[nociale]

@gowerc It looks good to me, as if I do drawObj$fit I would get the fit object, and this is exactly what this branch should add. I don't see any problem in your solution! What about tests? Should we add a test that verifies that a mmrm object is actually returned?

[gowerc]

@nociale

Urgh, a big issue with this that I only just realised is that we completely do away with the variable names and also manually expand out dummy variables i.e:

Can't imagine people will be happy getting V1 / V2 / V3...

The amount of code required to map that all bag to meaningful variable names I think might be a huge pain... Not really sure what to do here :(

code has changed since review

[gowerc]

Some notes as to why this is difficult:

• We manually expand out the design matrix before handing it over to mmrm. I think we do this so that we can be confident that the model coeficient order is in the same order that the data will be in when it is expanded out in the impute phase.

• Because we have expanded the matrix by hand we need to re-insert the original key group variables (visit, group & subjid) so in order to avoid name collisions we reset the names of all covariates to Vx. In essence our formula becomes outcome ~ V1 + V2 + V3 + V4 + us(visit | id / group) where the first few variables (i.e. V1-V3 ) are usually the expanded visit columns. I also have a feeling we renamed the variables to avoid issues with special symbols in the variable names like : though I'm struggling to remember the details of this.

We probably could re-write the model to be based on the original variable names and then just assume the expanded design matricies are properly aligned, but this would mean re-writing a non-trivial amount of code + unit tests.

[gowerc]

Some more notes encase we did want to make this change.

the dataframe is expanded into a design matrix in get_mmrm_sample(). It is then handed over to fit_mmrm() where the mmrm::mmrm() is called. The columns are actually renamed twice, once in as_model_df() and again in as_mmrm_df().

If we were to pass the raw dataframe to mmrm::mmrm() I think we could just delete as_mmrm_df(). Main thing would be to ensure the order of the coeficients returned by mmrm::mmrm() is consistant with how the design matrix is created in impute() in particular these two lines:

    dat_pt_mod <- as_model_df(dat_pt, data$formula)
    dat_ref_mod <- as_model_df(dat_ref, data$formula)

I don't think as_model_df() is used anywhere else. Looking at this the biggest issue I think by far would be updating the unit tests to account for all the refactoring of the model functions.

I can't decide if this is worth the hassle or not at the moment :s

[nociale]

@gowerc That was a really good point. If I blind myself from considering the amount of work needed to complete this task, I would say that if we return the MMRM object from the base imputation model, we should keep the same variable names as in data. From a user perspective, looking at the coefficient of "V1", "V2" etc.. is quite meaningless. So, if it is possible to ensure that the order of the variables is the same as in impute, then I would go for it. However since this new feature is not of primary importance for the package, I would also evaluate how much this is worth (maybe also based on feedbacks from user experiences etc..).

In short, I would personally add this feature only once the name of the variables is the same as in the input dataset.

[nociale]

Plus, please see #382 which also applies here in principle..