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

Adding model.matrix() and terms() methods #330

Merged
merged 33 commits into from
Aug 16, 2023

Conversation

ddsjoberg
Copy link
Contributor

@ddsjoberg ddsjoberg commented Aug 14, 2023

Description

  • Two new methods added for mmrm models: model.matrix() and terms().
  • The model.frame() method has been updated to return a data frame the size of the number of observations utilized in the model for all combinations of the model.frame(include) argument.

Outstanding

  • The default call to model.frame() does not return the outcome variable. This will be unexpected by most users, I would think. Should we update the default?
  • I am using the same structure in model.frame() and model.matrix() to construct the formula passed to terms.formula(). The lme4 package returns the fixed effects and response variable only. Should this be our default as well?

@danielinteractive danielinteractive self-assigned this Aug 14, 2023
Copy link
Collaborator

@danielinteractive danielinteractive left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @ddsjoberg for the PR! Please see first comments below. Please also add yourself to package authors in the DESCRIPTION file

R/tmb-methods.R Outdated Show resolved Hide resolved
R/tmb-methods.R Outdated Show resolved Hide resolved
R/tmb-methods.R Outdated Show resolved Hide resolved
R/tmb-methods.R Outdated Show resolved Hide resolved
R/tmb-methods.R Outdated Show resolved Hide resolved
R/tmb-methods.R Outdated Show resolved Hide resolved
R/tmb-methods.R Show resolved Hide resolved
R/tmb-methods.R Outdated Show resolved Hide resolved
@danielinteractive
Copy link
Collaborator

Thanks @ddsjoberg also for the questions:

  • The default call to model.frame() does not return the outcome variable. This will be unexpected by most users, I would think. Should we update the default?

Yeah we can update, thanks!

  • I am using the same structure in model.frame() and model.matrix() to construct the formula passed to terms.formula(). The lme4 package returns the fixed effects and response variable only. Should this be our default as well?

I just also looked at glmmTMB (we are even closer to that package re: model specification) and in this example:

library(glmmTMB)

m1 <- glmmTMB(count ~ mined + (1|site),
              zi=~mined,
              family=poisson, data=Salamanders)

terms(m1)

they also don't return site but just count and mined so I guess we can follow that pattern, yeah

Thanks!

ddsjoberg and others added 6 commits August 14, 2023 06:29
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
@ddsjoberg

This comment was marked as resolved.

@ddsjoberg
Copy link
Contributor Author

ddsjoberg commented Aug 15, 2023

A few updates:

  • Following the instructions in Tests fail on Mac M1+ #322, the unit testing issue I was experiencing resolved.
  • Our update to model.frame() solved the issue in weights() I originally brought up (the output from weights was the length of the non-missing rows in the fixed covariates data, rather than the length of the obs utilized in the model.)
  • As discussed, I updated model.frame() to include the response column by default to be consistent with other pkgs. That said, for the same reason of maintaining consistency, model.frame()'s default should probably be to return all the columns: see example below and the same is true for lme4 models. Let's discuss when we meet later today?
m1 <- glmmTMB::glmmTMB(count ~ mined + (1|site),
              zi=~mined,
              family=poisson, data=glmmTMB::Salamanders)

model.frame(m1) |> head()
#>   count mined site
#> 1     0   yes VF-1
#> 2     0   yes VF-2
#> 3     0   yes VF-3
#> 4     2    no  R-1
#> 5     2    no  R-2
#> 6     1    no  R-3

Created on 2023-08-15 with reprex v2.0.2

@danielinteractive
Copy link
Collaborator

Cool! Nice, yeah we can return all columns

@ddsjoberg ddsjoberg marked this pull request as ready for review August 15, 2023 18:21
Copy link
Collaborator

@danielinteractive danielinteractive left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, thx a lot @ddsjoberg - mainly code convention comments below, please go through all new code for those (e.g. comments in sentence style also in tests etc.) thx!

DESCRIPTION Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
R/tmb-methods.R Outdated Show resolved Hide resolved
R/tmb-methods.R Outdated Show resolved Hide resolved
R/tmb-methods.R Show resolved Hide resolved
R/tmb-methods.R Outdated Show resolved Hide resolved
tests/testthat/test-tmb-methods.R Show resolved Hide resolved
ddsjoberg and others added 12 commits August 15, 2023 13:58
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
@ddsjoberg
Copy link
Contributor Author

Nice work, thx a lot @ddsjoberg - mainly code convention comments below, please go through all new code for those (e.g. comments in sentence style also in tests etc.) thx!

Thanks for the additional comments (apologies about the points already covered in your contributing guide 😳). I think everything has been addressed!

tests/testthat/test-tmb-methods.R Outdated Show resolved Hide resolved
@danielinteractive danielinteractive merged commit 3d6f004 into openpharma:main Aug 16, 2023
16 checks passed
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.

2 participants