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.varest contains wrong (repeated) coefficients? #1174

Closed
MatthieuStigler opened this issue Oct 4, 2023 · 4 comments · Fixed by #1175
Closed

tidy.varest contains wrong (repeated) coefficients? #1174

MatthieuStigler opened this issue Oct 4, 2023 · 4 comments · Fixed by #1175

Comments

@MatthieuStigler
Copy link
Contributor

tidy.varest for package vars contains redundant coefficients, repeating coefficients of equation 1 for all equations instead of updating them. See below, where tidy(x)$estimate == coef(x) only for the first K variables in equation 1.

The bug comes most likely from the use of index [[1]] instead of [[v]] in https://github.com/tidymodels/broom/blob/main/R/vars-tidiers.R, implemented in #979:

for (v in names(s$varresult)) {
ret[[v]] <- as_tidy_tibble(
s$varresult[[1]]$coefficients,
new_names = c("estimate", "std.error", "statistic", "p.value")
)
ret[[v]]$group <- v
}

Also, are there tidy conventions for the name of the equation variable in the tidy output for multi-response/equations models? I notice tidy.varest uses group whereas tidy.mlm uses response (I guess there are more models with multiple equations)?

library(vars)
#> Loading required package: MASS
#> Loading required package: strucchange
#> Loading required package: zoo
#> 
#> Attaching package: 'zoo'
#> The following objects are masked from 'package:base':
#> 
#>     as.Date, as.Date.numeric
#> Loading required package: sandwich
#> Loading required package: urca
#> Loading required package: lmtest
library(broom)
data("Canada", package = "vars")
mod <- VAR(Canada, p = 1, type = "none")
tidy(mod)$estimate == unlist(purrr::map(coef(mod), ~.[,"Estimate"]))
#>       e.e.l1    e.prod.l1      e.rw.l1       e.U.l1    prod.e.l1 prod.prod.l1 
#>         TRUE         TRUE         TRUE         TRUE        FALSE        FALSE 
#>   prod.rw.l1    prod.U.l1      rw.e.l1   rw.prod.l1     rw.rw.l1      rw.U.l1 
#>        FALSE        FALSE        FALSE        FALSE        FALSE        FALSE 
#>       U.e.l1    U.prod.l1      U.rw.l1       U.U.l1 
#>        FALSE        FALSE        FALSE        FALSE

## coefs are repeated
all.equal(subset(tidy(mod), group=="e")[,-1], 
          subset(tidy(mod), group=="prod")[,-1])
#> [1] TRUE

Created on 2023-10-04 with reprex v2.0.2

@simonpcouch
Copy link
Collaborator

Thank you! Just put a fix in.

Also, are there tidy conventions for the name of the equation variable in the tidy output for multi-response/equations models? I notice tidy.varest uses group whereas tidy.mlm uses response (I guess there are more models with multiple equations)?

Package output is unfortunately not standardized in that case. The modeltests glossaries should give a sense of the set that those column names should fall in, but the divergence of those column names happened early on in the package's development and likely won't be revisited.

@MatthieuStigler
Copy link
Contributor Author

thanks for the fix and additional info!

Where can one consult the "modeltests glossaries "? Thanks!

@simonpcouch
Copy link
Collaborator

Ah, that was a bit obscure, wasn't it👻

Here's the glossary for acceptable column names in tidy() output, from the modeltests package, along with descriptions of what those columns (usually) contain: https://github.com/alexpghayes/modeltests/blob/master/data-raw/columns_tidy.yaml

Copy link

This issue 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 Dec 12, 2023
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 a pull request may close this issue.

2 participants