[ENH] softs_v2 Model added - #2232
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2232 +/- ##
=======================================
Coverage ? 87.65%
=======================================
Files ? 180
Lines ? 10299
Branches ? 0
=======================================
Hits ? 9028
Misses ? 1271
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:
|
phoeenniixx
left a comment
There was a problem hiding this comment.
Thanks!
i would suggest adding detailed docstrings with clear descriptions of all the params, so that it is easy to understand the methods and the architecture in general.
Is this code taken from somewhere or is it completely implemented from scratch? I think if it is taken from somewhere, it would be better to add the original authors to the authors tag.
Also, please add the references (like paper, repo etc) to the docstrings as well of the model.
|
|
||
| class SoftsEncoderLayer(nn.Module): | ||
| """ | ||
| Single Encoder layer for SOFTS. |
There was a problem hiding this comment.
Can you please add more detailed docstrings for all methods and classes?
| """Get the underlying DataModule class.""" | ||
| from pytorch_forecasting.data._tslib_data_module import TslibDataModule | ||
|
|
||
| return TslibDataModule |
There was a problem hiding this comment.
Why are we using this datamodule? I think this data module is mainly for tslib models, I have no issue with using this, but pls have a look at EncoderDecoderDataModule as well, maybe that would also be helpful.
I have not looked at the architecture, so I have a question: is the model encoder-decoder based model?
There was a problem hiding this comment.
We are using TslibDataModule because SOFTS is a direct-projection MLP model. It only consumes historical input sequences and maps them directly to the forecast window, using EncoderDecoderModule felt unnecessary.
From the perspective of data input we can call it encoder-only model.
Code Ref : Code
There was a problem hiding this comment.
I think this can be seen as a encoder-decoder model with decoder being a identity layer?
I think we should use the TslibDataModule only if we are interfacing the model from the tslib package. Otherwise if it fits encoder-decoder model type, we should use EncoderDecoderDataModule
There was a problem hiding this comment.
You need to update this data module as well then
| return TslibDataModule | ||
|
|
||
| @classmethod | ||
| def _get_test_datamodule_from(cls, trainer_kwargs): |
There was a problem hiding this comment.
I dont think we need this anymore
| def get_test_train_params(cls): | ||
| params = [ | ||
| {}, | ||
| dict(hidden_size=128, n_layers=1, use_revin=True), |
There was a problem hiding this comment.
Can you try adding some more params to cover more scenarios?
| class Softs(TslibBaseModel): | ||
| """ | ||
| SOFTS: Efficient Multivariate Time Series Forecasting with Series-Core Fusion. | ||
| """ |
phoeenniixx
left a comment
There was a problem hiding this comment.
I think there are geniune failures, please take a look at that
|
@phoeenniixx, I think there are no longer failures. Can you please re-review? (quick question, do we need to add the model to the docs, or is that automatic now?) |
We still need to add the API reference, but the table generation is automatic now! |
phoeenniixx
left a comment
There was a problem hiding this comment.
Thanks! I think this one is almost ready. Just few nitpicks.
Please add the API reference of the model here: http://github.com/sktime/pytorch-forecasting/blob/main/docs/source/m_layer_v2.rst#api-reference
| from pytorch_forecasting.models.base._tslib_base_model_v2 import TslibBaseModel | ||
|
|
||
|
|
||
| class Softs(TslibBaseModel): |
There was a problem hiding this comment.
Can the BaseModel not work here? I mean we mainly use TslibBaseModel for tslib models only
There was a problem hiding this comment.
The model is actually part of the tslib model family that's why I used that. Is there any particular reason to use the base model instead ?
There was a problem hiding this comment.
I thought tslib model family was from here: https://github.com/thuml/Time-Series-Library
But this model was taken from here: https://github.com/Secilia-Cxy/SOFTS/
That is why I said it is not a tslib model. Am I missing something here?
There was a problem hiding this comment.
Yeah, you're right I just misinterpreted thanks for letting me know. I'll adjust that and use the baseClass.
| hidden_size=64, | ||
| n_layers=1, | ||
| use_revin=False, | ||
| loss=MAE(), |
There was a problem hiding this comment.
It would be good if we could add other point prediction losses here as well - just to increase the coverage
There was a problem hiding this comment.
Thanks for letting me know I've updated that
| """ | ||
|
|
||
| _tags = { | ||
| "info:name": "SOFTS", |
There was a problem hiding this comment.
I think the name should be same as the class here: Softs in place of SOFTS?
There was a problem hiding this comment.
I think it should be SOFTS all over the place as the actual model name is SOFTS not Softs and also the convention also follows that across other models.
There was a problem hiding this comment.
Then you would have to update the class name. The tag name should be exactly same as the class name
|
Hi @phoeenniixx, |
phoeenniixx
left a comment
There was a problem hiding this comment.
Please use EncoderDecoderTimeSeriesDataModule in place of tslib one, if we are using the BaseModel and not the tslib one, then we need to use the corresponding data module as well. Although the tests are passing, but both the data modules represent different ideologies, and with time as we add more feats will diverge from each other, so we shouldn't use them interchangeably
| return x + dispatch_out | ||
|
|
||
|
|
||
| class SOFTSEncoderLayer(nn.Module): |
There was a problem hiding this comment.
should it go to _encoders folder?
There was a problem hiding this comment.
I considered this during implementation. Looking at the current _encoders/ contents, both Encoder and EncoderLayer are TimeXer-specific (they take cross, tau, delta params and have global-token logic).
Whereas SOFTSEncoderLayer has a fundamentally different interface it takes a 4D tensor (B, C, L, D) and uses STAD instead of attention, so there's no shared contract between them. _blocks/ currently houses model-specific building blocks like ResidualBlock (for DSIPTs), and SOFTSEncoderLayer + STADModule follow the same pattern self-contained blocks specific to one model. Should I move it to a new _softs/ subfolder under layers/ to make the model association clear.
There was a problem hiding this comment.
I see, but we can have multiple implementations of encoder layers, no? and that is why we created an _encoders folder that can host multiple implementations. It is not necessary that this encoder layer is just used by SOFTS, what if we see some new model that is derived from SOFTS, that could also use this layer. THe name - SOFTSEncoderLayer already makes the association pretty clear.
There was a problem hiding this comment.
Yes, it can be possible that in the future we'd reuse it. I've adjusted that.
|
|
||
| return {"prediction": out} | ||
|
|
||
| def predict_step( |
There was a problem hiding this comment.
why do we need a predict_step here? can't the BaseModel's implementation enough?
And if not, why do have not added other step functions like test_step?
There was a problem hiding this comment.
Yes, I thoroughly looked into the implementation. The BaseModel's implementation is enough.
|
Hi @phoeenniixx,
Kindly have a look |
Fixes #2231
Hi @fkiraly , @phoeenniixx, @PranavBhatP !
I have implemented the SOFTS (Star Aggregate-Dispatch for Time Series Forecasting) model within the PyTorch Forecasting v2 architecture.
Could you please review the PR?
Here is a summary of the changes made:
(pytorch_forecasting/layers/_blocks/_softs_block.py): Implemented the core neural network components, specifically the novel STADModule (Star Aggregate-Dispatch mechanism) and the SoftsEncoderLayer, completely isolated from the base estimating logic to strictly adhere to the project's v2 architectural standards.(pytorch_forecasting/models/softs/): Created the main Softs estimator wrapper which inherits from TslibBaseModel. It effectively handles dynamic feature input alignments (history_cont and history_target), optionally applies RevIN scaling, and routes the forward pass through the STAD-based encoders.(_softs_pkg_v2.py): Established the Softs_pkg_v2 class representing model metadata (capability tags, compute requirements) and defined automated testing configurations (get_test_train_params) tightly integrated with the underlying TslibDataModule.__init__.pyfiles across the layers and models directories to properly expose Softs, Softs_pkg_v2, and the STAD blocks to the broader PyTorch Forecasting ecosystem.Thank you!