Repository navigation
[ENH] Add TimeSeries datatype as per new API - #2410
phoeenniixx wants to merge 10 commits into
Conversation
phoeenniixx
added
enhancement
|
Will stack a PR for |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v2-dev #2410 +/- ##
=========================================
Coverage ? 87.79%
=========================================
Files ? 202
Lines ? 11292
Branches ? 0
=========================================
Hits ? 9914
Misses ? 1378
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
||
| self.save_hyperparameters(ignore=["loss", "logging_metrics", "optimizer"]) | ||
| self.save_hyperparameters( | ||
| ignore=["loss", "logging_metrics", "optimizer", "metadata"] |
There was a problem hiding this comment.
Had to add metadata (which is a dataclass now) to ignore as otherwise it causes issue of UnpicklingError of torch.
|
After talking to @CloseChoice, i will try to break this PR into smaller chunks and try to incorporate @Faakhir30's #2313 in here as well to create a Base data module |
phoeenniixx
changed the title
TimeSeries datatype and update datamodule with the new designTimeSeries datatype as per new API
|
After discussion with @fkiraly, I understood the issue in my mental model - Although So, in the recent commit, |
| ------- | ||
| t : numpy.ndarray of shape (n_timepoints,) | ||
| Time index for each time point in the past or present. Aligned with `y`, | ||
| and `x` not ending in `f`. |
There was a problem hiding this comment.
need to make this docstring better and clearer - also check other docstrings which might not be clear enough
| self.metadata = { | ||
| "cols": { | ||
| all_cols = self._target + self.feature_cols + self._static | ||
| self.metadata = TimeSeriesMetadata( |
There was a problem hiding this comment.
this makes the PR breaking, are breaking changes somewhere documented e.g. in a migration guide?
| ) | ||
|
|
||
|
|
||
| def _check_positive(value, name, allow_none=False): |
There was a problem hiding this comment.
this name is slightly confusing since positive usually means >0, if only ints are handed over it's fine but I don't see type hints here though in the params it is stated that it is int only.
3 participants
This PR adds the new Add
TimeSeriesdatatype and update datamodule with the new design.See #2407, #2398
TimeSeriesis now a standarised datatype. Updated the tests and renamed the file totest_timeseries_v2as this is no more a D1 layer!Also, we now have
metadataas a dataclass, instead of thedictto make it more user friendly.Updated tests also updates some old ones (and removes some tests like
initchecks - which are not needed i think?)This PR is aimed at
v2-devinstead ofmain, so that we dont break current user code until this design is complete and stable. See #2407 for details. So I had to update the branch target in the workflows for the tests to run (which might fail due to the changes)Stack
TimeSeries- [ENH] AddTimeSeriesdatatype as per new API #2410 <- This PRDatamodule- &Faakhir30 [ENH] Update Datamodules according to new API design #2423Forecaster- [ENH] Addforecastersupport and remove_pkgclass for new API #2434Note
The tutorials will fail for sure, will update the notebook in one of the branchs stacked over this PR.
Still need to add the
Forecasterand tests - will add the notebook and docs changes on the upcoming branches as the vignette is incomplete withoutforecasterwhich is not in this PR.Changes made
TimeSeriesfromtest_d1trotest_timeseries_v2TimeSeriesTimeSeriesMetadataas metadata inplace of adictDatasetanymoretargetis the last column ofdata.timeis the first column not already used asgroup,target,staticorweight. If no such column exists,ValueErroris raised - passtimeexplicitly.numandcatare the dtype split over all columns that are notgroup,timeorweightto_pandasto create a dffrom_tensorsto create aTimeSeriesclass for predicitons