FEAT - Add a fetcher for the electricity forecasting dataset#2013
FEAT - Add a fetcher for the electricity forecasting dataset#2013lisaleemcb wants to merge 14 commits into
Conversation
|
The dataset has been updated on osf at https://osf.io/download/d8ykq |
…tyexample_timeseries
rcap107
left a comment
There was a problem hiding this comment.
Thanks for the PR @lisaleemcb! I left a few comments
Co-authored-by: Riccardo Cappuzzo <7548232+rcap107@users.noreply.github.com>
|
one thing we need to check is the license(s) of the original data, to make sure we can redistribute it like this. |
…lisaleemcb/skrub into issue1844_electricityexample_timeseries
…tyexample_timeseries
rcap107
left a comment
There was a problem hiding this comment.
Looks good to me, thanks a lot @lisaleemcb
updated test_dataset_files() Co-authored-by: Jérôme Dockès <jerome@dockes.org>
…tyexample_timeseries
…lisaleemcb/skrub into issue1844_electricityexample_timeseries
jeromedockes
left a comment
There was a problem hiding this comment.
LGTM besides the little fix needed on the test! thank you @lisaleemcb
| ) | ||
| path = _fetching.fetch_electricity_forecasting() | ||
| downloaded = [f.name for f in Path(path).iterdir() if f.is_file()] | ||
| assert files == set(downloaded) |
There was a problem hiding this comment.
it seems the weather ones are parquet not csv? also it is more common to write assert actual == expected than assert expected == actual so writing it in that order makes the pytest error a little easier to read for people used to that convention
There was a problem hiding this comment.
Hi @jeromedockes , thanks for the tip about the boolean check!
Regarding the files, the folder should have both but I'm not checking for the parquet files. Do you want me to change that?
|
Discussed IRL: there should be only csv files. We also need to upload the new zip version (with csv only) on osf. |
Added a timeseries dataset for a forecasting example.
To do: upload to osf.io
Addresses #1844