Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Intro vignette #116
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
Intro vignette #116
Changes from 19 commits
c5a2f3d
aefe7da
a8c819f
1bf1bad
2b80ada
b93c2b1
4cbe398
6b51a3c
1765e32
dd93932
4bdaae3
64b4c5f
f4dcd53
2414caa
ca95ffd
0f5c4a5
d007d00
a7b67bf
fe24acf
3b11b42
0bff261
38f3b1d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why
intercept
was removed? Is it because it seems like we would always have the intercept in the ARX model no matter what?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we necessarily have these default
lags
andahead
arguments? The defaults seem specifically defined fortime_type
=day
but there are many other time types for which the defaults make a bit less senseThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do leave the defaults as is, it may be good to briefly mention the defaults somewhere else in
arx_args_list()
aside from the initial list of arguments to emphasize them (under the argument descriptions seems to be natural choice, where I see mention of defaults for some but not all of the arguments).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Intercept, yes, it's always there.
On the
time_type
. Good point. Our implementation always modifies thetime_value
by a day. I added this to the documentation.Actually, can someone create an issue to:
time_unit
argument (days, months, weeks, years) and perform the operations appropriately.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should users know the default of
arx_args_list()
creates 0, 7, 14 periods lagged predictors and 7 day ahead outcomes?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but I don't want to put this in the docs at the moment. Looking at the function would tell you.