-
Notifications
You must be signed in to change notification settings - Fork 917
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
Tide Implementation #1727
Tide Implementation #1727
Conversation
Hi @alexcolpitts96 and thanks for this PR 🚀 If you want you can go ahead with the implementation :) |
Thanks @alexcolpitts96, I'll review next week! |
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.
This is a really nice PR, thanks a lot @alexcolpitts96! 🚀
At the current state I only have minor suggestions and:
- could you improve the TiDE tests? I see that currently it is basically a copy of the N/DLinear model tests which are not all applicable to the TiDE model.
For the next steps it would be ideal to implement the following:
- probabilistic support similar to how it's done in other models. We really want to offer probabilistic support for all TorchForecastingModels
- currently the model only works with future covariates. Can we adapt the model to support the case without future_covariates (e.g. don't add future covariate related part of the network when not using future covs, and adapting the forward accordingly)?
- could we add an embedding for catoegorical static covariates, similar to how we do it in TFTModel? Most of the time, static covariates are categorical and it would be great to have this support here as well.
- could you maybe test the model performance against nlinear/dlinear (not a unittest) and share the results with us? Would be nice to see how it performs. This can be a simply plot of predictions from TiDE against N/DLinear on some time series like AirPassengersDataset
@dennisbader I am happy to make these changes. I haven't setup proper testing yet gradle was breaking for me. I have yet to resolve the issue. It keeps hanging on the formatting portion (black). |
Let me know when it's ready for a second round review |
Btw some tipps for testing / linting / formatting (if you aren't aware of this yet, as you mentioned testing with gradle and the linting issues):
|
@dennisbader I have added tests and probabilistic forecasts. In general TiDE is an OK model, in my opinion. It is more accurate than N/DLinear; however, it requires much more care in terms of training. Without gradient clipping, scheduled learning rates, and good initial learning rates it easily gets stuck in local minima. I also found that it is slower than N/DLinear and may not provide enough of an accuracy improvement to be relevant for short deadline forecasts. |
I COMPLETELY forgot that I was looking at dataset cleanup (#1539) before I started looking at TiDE. I will try to strip out those commits. |
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
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.
Pathing change
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.
Great improvement @alexcolpitts96, thanks for that.
I had a couple of suggestions and a bug when using past covariates and no future covariates.
What I find a bit worrying is that the probabilistic predictions seem much better than the deterministic. In the top image below you see the default deterministic predictions and in the lower one the predicitons using QuantileRegression (the quantile intervals also seem a bit unstable).
From my comments from last time, only the categorical static covariates support is missing.
The unit tests should probably also be extended a bit. I would suggest to lean more towards the TFTModel tests rather than the D/NLinear tests.
Regarding the default hyperparmeters, the model is pretty slow (even slower than NBEATSModel, which shouldn't be the case in my opinion).
For example in the paper, they state that they only used temporal_width=4
in contrast to our temporal_width=128


suggestions made by Loudegaste Co-authored-by: Loudegaste <[email protected]>
Co-authored-by: Dennis Bader <[email protected]>
Co-authored-by: Dennis Bader <[email protected]>
Hey @alexcolpitts96 and thanks for the update. That sounds great, I'm glad that you found a way to improve the model performance :) 🚀 In that case I think we can go for including this in the next Darts release. I can do another round of review later today/tomorrow. One thing that would be nice now that it'll be integrated, is to add a simple example notebook for the model similar to other TorchForecastingModels (darts/examples). Do you think you could add one? |
@dennisbader adding an example shouldn't be too tricky. I essentially built one already for testing it. I would hold off on doing another review until after the RINorm PR is merged and I add it to TiDE. At that point it should have all the features that the original paper has (+probabalistic forecasting) and would be ready for another review. |
@alexcolpitts96, perfect 👍 I just merged the RINorm and updated the TiDE branch. Let me know when this one is ready for another review 🚀 |
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.
Great work @alexcolpitts96, this looks very nice 👍 I also played around a bit more with the model today and it feels more robust now 🚀
I only had minor suggestions and a question about covariates normalization that I'd like to know your opinion on.
From what I see, the only things remaining are:
- TiDE example notebook
- categorical static covariates through embeddings (this one we can also open another PR for, as we can do it at the same time for N/DLinear as well).
@dennisbader assuming the tests pass, this should be good to go🤞 |
add comments to RINorm
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.
Congratulations @alexcolpitts96 on this great PR, it will be a very cool addition to darts :) 🚀 💯
The effort that went into this is really appreciated, thanks a lot!
I updated some last things to make it ready for the merge:
- updated the example notebook (just some slight rephrasing to match the style of other Darts documents).
- Added TiDE model to the model sections of the documentation
- some additions to the documentation
Let me know if you want to have a look at it :) If not, then it's ready to be merged 🚀
I don't have any issues with it. Thanks for all the help getting it cleaned up and improved. 😁 |
Related to #1726
Looking for a sanity check before documenting/testing.