[ENH] Add v2 interface support for RecurrentNetwork (RNN)#2136
[ENH] Add v2 interface support for RecurrentNetwork (RNN)#2136Meet-Ramjiyani-10 wants to merge 16 commits intosktime:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2136 +/- ##
=======================================
Coverage ? 86.72%
=======================================
Files ? 167
Lines ? 9806
Branches ? 0
=======================================
Hits ? 8504
Misses ? 1302
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
phoeenniixx
left a comment
There was a problem hiding this comment.
Thanks!
Added some comments
| return TslibDataModule | ||
|
|
||
| @classmethod | ||
| def _get_test_datamodule_from(cls, trainer_kwargs): |
There was a problem hiding this comment.
I dont think you need this method anymore? Please have a look at other pkgs (like timexer) for more info
There was a problem hiding this comment.
thanks for pointing. After checking _timexer_pkg_v2.py. this method is not needed . Will remove it
| """Get the underlying DataModule class.""" | ||
| from pytorch_forecasting.data._tslib_data_module import TslibDataModule | ||
|
|
||
| return TslibDataModule |
There was a problem hiding this comment.
Should it be EncoderDecoderDataModule?
There was a problem hiding this comment.
@phoeenniixx After checking _timexer_pkg_v2., it also uses TslibDataModule in get_datamodule_cls. Should I still switch to EncoderDecoderDataModule, or is TslibDataModule correct here?
There was a problem hiding this comment.
Please switch to EncoderDecoderDataModule for models with v1 native implementations. The rule of thumb is to stick to TslibDataModule only for direct migrations of models from thuml(tslib).
There was a problem hiding this comment.
@PranavBhatP
thanks for clarifying!. But it seems EncoderDecoderDataModule doesn't exist in the codebase yet. Should I keep TslibDataModule for now, or is there a different existing class I should use?
There was a problem hiding this comment.
EncoderDecoderTimeseriesDataModule is the exact name of the D2 data module I am referring, it is interchangeably referred to as EncoderDecoderDataModule (u get what i mean?).
There was a problem hiding this comment.
yes @PranavBhatP got it . I was searching for EncoderDecoderDataModule as a single class and couldn't find it , making the changes.. EncoderDecoderTimeSeriesDataModule
| params : list of dict | ||
| Parameters to create testing instances of the class. | ||
| """ | ||
| from pytorch_forecasting.metrics import MAE, SMAPE, QuantileLoss |
There was a problem hiding this comment.
Did you mean to use them in the params?
I think it would be good if we could chekc which type of losses this can support
There was a problem hiding this comment.
i will update get_test_train_params to include MAE, SMAPE, and QuantileLoss directly in the params to verify loss compatibility, as in _timexer_pkg_v2.py
…ate test params with losses
…ani-10/pytorch-forecasting into enh/rnn-v2-interface
…ani-10/pytorch-forecasting into enh/rnn-v2-interface
|
@phoeenniixx and @PranavBhatP assert object_pkg.name == object_class.name + "_pkg_v2", msg object_class.name == "RecurrentNetwork_v2" Could you please advise what naming you’d prefer here? I see two options: Rename the model class back to RecurrentNetwork and keep the package as RecurrentNetwork_pkg_v2 (matching the convention used for the other v2 models), or |
|
Yes this is a known issue, see #2080 by @PalakB09. You can:
The decision is depends on you |
|
okay, for now i will rename the model class to RNN |
Reference Issues/PRs
Closes #2128. Part of #1992.
What does this implement/fix? Explain your changes.
Adds v2 interface support for the RecurrentNetwork (RNN) model.
_rnn_v2.pyimplementingRecurrentNetwork_v2usingTslibBaseModel_rnn_pkg_v2.pywith tags, datamodule, and test parametersI followed the v2 pattern established by DLinear and SAMformer and have made no changes to existing v1 implementation.
What should a reviewer concentrate their feedback on?
Did you add any tests for the change?
Test parameters added in
get_test_train_paramscovering LSTM, GRU, single layer, and multi-layer configurations.Any other comments?
Happy to iterate based on feedback.
PR checklist
[✓ ] The PR title starts with either [ENH], [MNT], [DOC], or [BUG]. [BUG] - bugfix, [MNT] - CI, test framework, [ENH] - adding or improving code, [DOC] - writing or improving documentation or docstrings.
[ ✓] Used pre-commit hooks when committing to ensure that code is compliant with hooks. Install hooks with
pre-commit install.To run hooks independent of commit, execute
pre-commit run --all-files