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

roll_lm checks nrow(x) > width rather than nrow(x) > min_obs #3

Closed
rrichmond opened this issue Jul 19, 2017 · 2 comments
Closed

roll_lm checks nrow(x) > width rather than nrow(x) > min_obs #3

rrichmond opened this issue Jul 19, 2017 · 2 comments

Comments

@rrichmond
Copy link

Right now roll_lm throws the error when you try to run a rolling regression on data that is shorter than width:

Error in eval(substitute(expr), envir, enclos) : 
  value of 'width' must be between one and number of rows in 'x' and 'y'

It seems more appropriate to check the nrow(x) and nrow(y) are just greater than min_obs. Here's an example:

library(roll)
n_vars <- 10
n_obs <- 50
x <- matrix(rnorm(n_obs * n_vars), nrow = n_obs, ncol = n_vars)
y <- matrix(rnorm(n_obs), nrow = n_obs, ncol = 1)
# Rolling regressions
result <- roll_lm(x, y, width=100, min_obs = 10)
@jasonjfoster
Copy link
Owner

Thanks for the feedback. Fyi, I typically use the min_obs argument when there are many NA’s in a data set, yet I’m comfortable with a certain threshold of missing values. For example, rolling regressions with a 252-day window and min_obs = 63 days, i.e. a quarter. My sense is that most users don’t change the default min_obs = width argument though.

Anyway, in your example, I could just use width = min(100, nrow(x)) instead of width = 100 to satisfy the condition; however, I understand the latter is easier to write and, if a user has determined a reasonable min_obs threshold, then he/she should understand the concept of an expanding window and wants to get the desired result.

My concern is when users, who don’t change min_obs, see an error message about min_obs. Something like the following message could be confusing if the default arguments remain untouched: value of 'min_obs' must be between one and number of rows in 'x' and 'y'

In general, maybe a better solution is dependent on whether the min_obs is changed from the default or not. That is, if a user does not specify min_obs, then the error message stays the same as it is currently, otherwise a min_obs specific error message is returned like above. And then you can get the result, like in your example, if satisfied. What do you think?

@jasonjfoster
Copy link
Owner

I have simplified checks for the width and min_obs arguments in the development version:

# install.packages("devtools")
devtools::install_github("jjf234/roll")

Specifically, if the min_obs argument is different than the width argument, then there is a check whether the value of min_obs is between one and the minimum of either the number of rows in data or width.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants