-
Notifications
You must be signed in to change notification settings - Fork 909
Updated training window error assertion for relative units #728
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #728 +/- ##
=========================================
Coverage ? 97.64%
=========================================
Files ? 118
Lines ? 10247
Branches ? 0
=========================================
Hits ? 10006
Misses ? 241
Partials ? 0
Continue to review full report at Codecov.
|
jeremyliweishih
left a comment
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.
LGTM. What do you think @rwedge?
jeremyliweishih
left a comment
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.
Would it be possible to add testing for edge cases like leap years? Otherwise LGTM!
jeremyliweishih
left a comment
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.
LGTM. @rwedge
| assert feature_matrix_2.index.all([1, 2, 3, 4]) | ||
| assert feature_matrix_3.index.all([2, 3, 4]) | ||
| assert feature_matrix_4.index.all([2, 3, 4]) | ||
| assert feature_matrix_4.index.all([2, 3]) |
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 don't think these assertions are working as intended. assert feature_matrix_4.index.all([2, 3, 4]) and assert feature_matrix_4.index.all([2, 3]) are both evaluating as True
rwedge
left a comment
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.
Looks good!
training_windowindfs()not support 'month'? #719