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

new tidiers: vars package / varest objects #979

Merged
merged 4 commits into from
Jan 8, 2021
Merged

new tidiers: vars package / varest objects #979

merged 4 commits into from
Jan 8, 2021

Conversation

vincentarelbundock
Copy link
Contributor

The goal of this PR is to address issue #161. It is marked as WIP because I have a couple questions a few notes.

Questions:

  • The vars package does not define a confint method and does not report confidence intervals. Should the tidy method (a) include and document a conf.int argument and print an informative error if users set it to TRUE, or (b) exclude this argument from the function definition?
  • One important information to report in glance is the lag order. Right now, I included a column called "order". Is that a good name? Should I make a PR to modeltests?

Note:

  • varest objects include results for several levels. These are indicated in a group column produced by tidy.
  • It is not clear to me what should be included in the augment output from a VAR model. For now, I do not define this function, but that might be something to consider in the future if an expert can help.
  • Right now, the glance test is commented out until modeltests includes and documents the column name.

@simonpcouch
Copy link
Collaborator

As for your first bullet point, it's fine to exclude the conf.* level arguments and document the behavior in @details. :-)

Generally, when it comes to modeltests, feel free to file a PR there with the entries you need and link here. I'll merge there and broom can temporarily depend on the Remotes: version of modeltests until we push a release out to CRAN!

It's fine to not define an augment method, feel free to uncomment the glance method once the modeltests PR is merged.

@vincentarelbundock
Copy link
Contributor Author

Thanks for the info.

I think there's an argument to be made for including conf.* and exiting with an error instead of sinking it in .... This would be more explicit, which I prefer, but I'm not dogmatic enough to argue if you prefer I remove the argument and mention this in docs.

@simonpcouch
Copy link
Collaborator

I'm on board for a warning in that case!

@vincentarelbundock vincentarelbundock changed the title [WIP] new tidiers: vars package / varest objects new tidiers: vars package / varest objects Jan 6, 2021
@vincentarelbundock
Copy link
Contributor Author

Removing the [WIP] tag because I think this looks pretty good now. The remaining error should be fixed when modeltests is updated by this PR: alexpghayes/modeltests#35

== Failed tests ================================================================
-- Failure (test-vars.R:29:3): glance.vars -------------------------------------
length(unacceptable) == 0 is not TRUE

`actual`:   FALSE
`expected`: TRUE 
Output column names not in the column glossary: lag.order

[ FAIL 1 | WARN 25 | SKIP 3 | PASS 3886 ]
Error: Error: Test failures
Execution halted

fix return_glance call, spaces before/after operators, clarify warning
@simonpcouch simonpcouch merged commit 1d10ae1 into tidymodels:master Jan 8, 2021
@simonpcouch
Copy link
Collaborator

Much appreciated! :-)

@vincentarelbundock
Copy link
Contributor Author

Ooof, this one wasn’t so clean. Thanks for the review!

@github-actions
Copy link

github-actions bot commented Mar 6, 2021

This pull request has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants