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

tidy.mvgam() WIP #100

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

tidy.mvgam() WIP #100

wants to merge 10 commits into from

Conversation

swpease
Copy link
Contributor

@swpease swpease commented Jan 10, 2025

draft method

Some comments on the current status:

  • I haven't included the individual betas for random effects. I'm leaning towards that they should be included.
  • I think if using hierarchical residual correlation via gr, the Sigma matrix should be split out by group, but haven't implemented that yet.
  • Include the changepoints in piecewise trends?
  • I haven't yet considered an option to return the smooths significances, if that might be useful.
  • How to test?
    • The existing mvgam_example[n]s cover a good portion of possible outputs, but this method is supposed to handle every type of model. However, it seems excessive and minimally useful to cover that whole range with additional mvgam_examples.

Sorry this took to long -- I wanted to go over all the model types, and I've been managing eye strain so I can't just stare at text on my monitor for hours on end.

draft method
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 very much for this initial effort @swpease. I'm hugely appreciative, and overall I think this covers most of what users would like

grep always returns a vec, even if you pass it null, so null check is wrong
Decided it would be better to include these than not; can be easily filtered out if undesired.
These models have error terms (sigmas) which were not being included.
"param_type" -> "type" to comply w/ {broom}
Removed `=` assignments
Wrote documentation for method. Also settled on names for the "term" column contents.
covers the majority of main use cases, but is missing a random effects test and a heirarchical correlation test.
The snapshots (note: not snapshot values) record what's printed, and the `check()` call led to a different truncation compared to `test_active_file()`, so I'll just use `expect_snapshot_value()` instead, even though it's harder to read.
called `document()` to put it in the namespace and... documentation.
@swpease
Copy link
Contributor Author

swpease commented Jan 16, 2025

Alright, I think it's mostly there now.

I still want to split out the different covariance matrixes for heirarchical correlation cases, but think it warrants having a new mvgam_example to check against. For tidy, the example model's output/sensibility is irrelevant, so long as it doesn't error, but maybe it would be good to have one that makes sense for other possible tests?

The best tests I could think of were just some snapshot tests. I opted for this unappealing JSON output because the regular output (a print) was giving technically-not-false errors due to varying output truncations. I can't say I've ever had seamless experiences with the testthat snapshotting capabilities.

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