[ENH] Add TSMixer v2 model - #2387
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2387 +/- ##
=======================================
Coverage ? 88.22%
=======================================
Files ? 199
Lines ? 11142
Branches ? 0
=======================================
Hits ? 9830
Misses ? 1312
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:
|
There was a problem hiding this comment.
Thanks!
Please add docstrings to the public classes and methods
Also, for the test file, i think most of those tests are already handled by the unified test framework no? Can you please de-duplicate the file
Also, can you please add the API reference for the model and pkg class as well?
| class. | ||
| """ | ||
|
|
||
| import torch.nn as nn |
There was a problem hiding this comment.
I’m using it for nn.MSELoss() within the test. Would it be preferable to remove this case, or perhaps move the import to the top of the file?
There was a problem hiding this comment.
Thanks, I missed it the last time
| from pytorch_forecasting.models.base._tslib_base_model_v2 import TslibBaseModel | ||
|
|
||
|
|
||
| class TSMixerBlock(nn.Module): |
There was a problem hiding this comment.
Can you please add docstrings here?
There was a problem hiding this comment.
Definitely, have added docstrings to the TSMixerBlock class and its forward function in the latest commit.
| metadata=metadata, | ||
| ) | ||
|
|
||
| warnings.warn( |
There was a problem hiding this comment.
I think the warning is already handled by BaseModel no? See here
There was a problem hiding this comment.
Thanks @phoeenniixx, have removed the redundant warning now.
| assert model.n_quantiles is None | ||
|
|
||
|
|
||
| def test_tsmixer_forward(sample_dataset): |
There was a problem hiding this comment.
I think it is already being tested by the test framework
There was a problem hiding this comment.
Yes, it isn't adding a meaningful test, have removed it now. Thanks @phoeenniixx.
8277cc0 to
9b7ab27
Compare
9b7ab27 to
d2e9470
Compare
|
Hi @phoeenniixx, Thank you for the review. Have attempted to address the four comments left. Additionally, the generic tests have been removed and TSMixer has been added to the API documentation. |
|
Hi @phoeenniixx, The failure appears to be due to a network/DNS issue whilst downloading |
| from pytorch_forecasting.models.base._tslib_base_model_v2 import TslibBaseModel | ||
|
|
||
|
|
||
| class TSMixerBlock(nn.Module): |
There was a problem hiding this comment.
should this be added to layers/_blocks? Sorry i didnt notice this earlier
|
|
||
|
|
||
| @pytest.fixture | ||
| def sample_dataset(): |
There was a problem hiding this comment.
you can use examples.load_toydata as well here and then pass that to TimeSeries
phoeenniixx
left a comment
There was a problem hiding this comment.
Thanks! i think it is almost ready!
Just a few nit-picks
Reference Issues/PRs
For issue #1992 which requires implementing "models to the v2 interface". This PR adds the TSMixer implementation for the v2 interface.
#1992
What does this implement/fix? Explain your changes.
Implements TSMixer model for the v2 interface, including:
QuantileLosssupportWhat should a reviewer concentrate their feedback on?
The implementation is complete as an initial functional TSMixer v2 integration.
Quantile forecasting with multiple targets and inputs without target history will raise explicit errors. Feedback on methodology for resolving these cases without errors would be welcome if their support is required.
Did you add any tests for the change?
Yes, PR has corresponding tests implemented therein, covering:
Any other comments?
Only single-target quantile forecasting is currently supported, consistent with the current DLinear implementation.
PR checklist
pre-commit install.To run hooks independent of commit, execute
pre-commit run --all-files