-
Notifications
You must be signed in to change notification settings - Fork 904
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
Feat/Uber TLC Dataset #1003
Feat/Uber TLC Dataset #1003
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1003 +/- ##
==========================================
- Coverage 92.92% 92.58% -0.34%
==========================================
Files 76 76
Lines 7628 7704 +76
==========================================
+ Hits 7088 7133 +45
- Misses 540 571 +31
Continue to review full report at Codecov.
|
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.
Once more, a very nice addition, thanks a lot!
I just have a comment concerning leaving it as an option to return a multivariate series, or multiple series. Let me know if you think that's a good idea.
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.
Cool, only one minor suggestion left then we can merge :)
Co-authored-by: Julien Herzen <j.herzen@gmail.com>
I added the functionality to the electricity dataset too |
Is something else needed, or can we merge this branch? |
I think we can merge soon. The test coverage seems to have decreased. Are you able to have a look why? |
@hrzn It's probably the |
You could maybe add a test that calls |
ele_multi_series_dataset = DatasetLoaderCSV( | ||
metadata=DatasetLoaderMetadata( | ||
"Electricity_test.csv", | ||
uri=_DEFAULT_PATH + "/Electricity_test.csv", |
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.
Could we maybe put this one in datasets/test/Electricity_test.csv
? i.e., we create a test subfolder.
Thanks @gdevos010. I assume the tests should pass once it is merged, right? The only last tiny thing I'd ask is to put the test dataset in a subfolder |
@hrzn Yes, the tests passed locally. I moved the file to |
This will be my last PR for about a month. I will be traveling without a laptop. |
Thanks! I was referring to |
Enjoy :) |
Fixes #.
Summary
New dataset based on the FiveThirtyEight dataset.
Supports both daily and hourly sampling.
Other Information