Improvement/Adds temporal_hidden_size_past and temporal_hidden_size_future hyperparams to TiDEModel #2416
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2416 +/- ##
==========================================
- Coverage 93.81% 93.80% -0.02%
==========================================
Files 139 139
Lines 14694 14684 -10
==========================================
- Hits 13785 13774 -11
- Misses 909 910 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Hi @eschibli, and thanks for this PR.
In order to make it backwards compatible (e.g. loading a TiDEModel save from an older Darts version into the new one), we need to change the default behavior of the hyperparmeters.
| temporal_hidden_size_past: int, | ||
| temporal_hidden_size_future: int, |
There was a problem hiding this comment.
In order for this to be backward compatible (loading a saved TiDEModel in the new Darts version), we need to set a default of None here. If temproal_hidden_size_past/future is None, then we need to use the hidden_size.
| temporal_hidden_size_past: int, | |
| temporal_hidden_size_future: int, | |
| temporal_hidden_size_past: Optional[int] = None, | |
| temporal_hidden_size_future: Optional[int] = None, |
| self.temporal_hidden_size_past = temporal_hidden_size_past | ||
| self.temporal_hidden_size_future = temporal_hidden_size_future |
There was a problem hiding this comment.
From comment above
| self.temporal_hidden_size_past = temporal_hidden_size_past | |
| self.temporal_hidden_size_future = temporal_hidden_size_future | |
| self.temporal_hidden_size_past = temporal_hidden_size_past or hidden_size | |
| self.temporal_hidden_size_future = temporal_hidden_size_future or hidden_size |
| @@ -375,6 +383,8 @@ def __init__( | |||
| hidden_size: int = 128, | |||
| temporal_width_past: int = 4, | |||
| temporal_width_future: int = 4, | |||
| temporal_hidden_size_past: int = 64, | |||
There was a problem hiding this comment.
Same comments here as above
| will bypass feature projection and use the raw feature data. | ||
| temporal_hidden_size_past |
There was a problem hiding this comment.
mention here that by default, it uses the same value as temporal_width_*, but that it is recommended setting a lower value.
Checklist before merging this PR:
Implements #2408
Summary
Adds hyperparameters controlling hidden layer size for the past covariate and future covariate feature encoders in
TiDEModel. Previously these were controlled byhidden_size.Other Information
Defaults were set to 64. In the original google-research implementation the default
temporal_hidden_size_pastwas set to 64, and there was no future covariate encoder.Alternatively, to preserve default behavior, defaults could be set to 128.
Other comments:
Oops I see my rebase failed horribly - should I drop this PR and do it over from a fresh branch?