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

augment.mvgam tidying method added #88

Merged
merged 3 commits into from
Nov 21, 2024

Conversation

swpease
Copy link
Contributor

@swpease swpease commented Nov 8, 2024

Implemented an augment method for mvgam objects, one of the "tidying" methods. Largely diverged from the set of recommendations in generics.

@swpease
Copy link
Contributor Author

swpease commented Nov 8, 2024

A few notes:

  • I used the generics package per the tidymodels suggestion.
  • I put both generics and tibble in Imports, because they are both dependencies of dependencies anyway: sort(tools::package_dependencies(packages = "mvgam", recursive = TRUE)$mvgam).
  • I didn't follow the tidymodels suggestion on implementation very closely.

I just did what seemed sensible to me. In any case, I'll need to actually write some more documentation.

Copy link
Owner

@nicholasjclark nicholasjclark left a comment

Choose a reason for hiding this comment

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

Thanks so much for this @swpease. Overall this looks great but there is one small issue that needs a bit of work: mvgam can handle data that are in a list rather than a dataframe, so the obs_data slot may be in list format as well (see mvgam:::mvgam_example5$obs_data for an example of this). Would you mind editing augment.mvgam to handle list observations? Unfortunately this will mean that the output will also be a list in those special cases, but I think that is fine for now

@swpease
Copy link
Contributor Author

swpease commented Nov 16, 2024

It looks like as_tibble handles lists just fine, e.g.

identical(mvgam_example5$obs_data, as.list(tibble::as_tibble(mvgam_example5$obs_data)))

, but do you explicitly want a list returned if the data was input as a list? Maybe there's some edge case I'm missing.

I see also that the covariates can be a matrix, which as_tibble does handle, but each column name is like "cov[, 1]", "cov[, n]". Example:

miss_dat <- list(outcome = rnorm(10),
  series = factor('series1',
  levels = 'series1'),
  time = 1:10)
miss_dat$cov <- matrix(rnorm(50), ncol = 5, nrow = 10)

out = mvgam(outcome ~ cov,
  data = miss_dat,
  family = gaussian(),
  backend = 'cmdstanr')

x = augment(out)

This isn't the first time I've wished R was statically typed :neckbeard:

@nicholasjclark
Copy link
Owner

Thanks @swpease. I think in general it would be better if a list is returned whenever data was supplied as a such, just to ensure there are no surprises for the users who will then want to use those data with the added fits and residuals to make diagnostic plots. As long as this is explicitly documented in the function docs then that would be better in my opinion

Implemented an `augment` method for mvgam objects, one of the "tidying" methods. Largely diverged from the set of recommendations in [generics](https://web.archive.org/web/20240520145739/https://www.tidymodels.org/learn/develop/broom/#implementing-the-augment-method).
@swpease
Copy link
Contributor Author

swpease commented Nov 19, 2024

Alright, method operates on lists. I'm not sure what would be useful tests, so I've got a couple simple ones. Can't think of anything that doesn't seem brittle and unhelpful. Also, unsurprisingly I messed up my git workflow, so I kinda want to just make a new branch with a single commit and copy over the changes.

@nicholasjclark nicholasjclark merged commit 55a5505 into nicholasjclark:master Nov 21, 2024
7 of 8 checks passed
@nicholasjclark
Copy link
Owner

Excellent thanks @swpease. I've merged this now and added you as a contributor to the DESCRIPTION. Please check and let me know if more details should be added (i.e. ORCID id or email address). I really appreciate the help! Will need to think about adding glance and tidy support soon as well for the whole package

@swpease
Copy link
Contributor Author

swpease commented Nov 22, 2024

Here's my shiny new ORCID ID: 0009-0006-8977-9285

I had initially looked at implementing glance, since it's first in the list, until I scrolled and scrolled through summary, and thought, let's see how augment goes.

@nicholasjclark
Copy link
Owner

Thanks @swpease. Yes completely agree, that function in particular needs a major overhaul first. I've added your ORCID now, really appreciate the help

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