[ENH] Units_v2 Model added - #2165
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2165 +/- ##
=======================================
Coverage ? 87.57%
=======================================
Files ? 180
Lines ? 10308
Branches ? 0
=======================================
Hits ? 9027
Misses ? 1281
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:
|
|
|
||
| _tags = { | ||
| "info:name": "UniTS", | ||
| "authors": ["Muhammad-Rebaal"], |
There was a problem hiding this comment.
I think we should also credit the actual author of the model? The one who implemented it originally?
There was a problem hiding this comment.
yeah sure, I'll add him as well
| ) | ||
| from pytorch_forecasting.models.tide import TiDEModel | ||
| from pytorch_forecasting.models.timexer import TimeXer | ||
| from pytorch_forecasting.models.units import UniTS_pkg_v2 |
There was a problem hiding this comment.
I think you should add the model (and not the pkg) here?
| """ | ||
| UniTS: Unified Time Series Model. | ||
|
|
||
| Patch-based transformer for multivariate time series forecasting. |
There was a problem hiding this comment.
I think it would be good if you could also add the references (like GH link and the paper) here?
|
Hi @phoeenniixx , Kindly review the PR, I've updated the PR with the requested changes. Thank You. |
| import torch.nn as nn | ||
|
|
||
|
|
||
| class _PatchEmbedding(nn.Module): |
There was a problem hiding this comment.
Yes I think it would make complete sense if we'd place it there in a file called _patch_embedding.py, as the _embeddings directory is already the designated home for embedding abstractions (like _data_embedding.py and _en_embedding.py).
| return emb.mean(dim=1) | ||
|
|
||
|
|
||
| class _PositionalEncoding(nn.Module): |
There was a problem hiding this comment.
it could go to layers/_encoders?
There was a problem hiding this comment.
Actually its just a misinterpretation, it is an embedding not an encoder. We already have a PositionalEmbedding class in _embeddings/_positional_embedding.py doing the exact same math. Instead of duplicating that logic in an _encoder.py file, I created a _PositionalEmbedding child class inside the existing _positional_embedding.py file. It inherits the fixed sinusoidal buffer from the parent and adds the specific dropout and additive forward logic required for UniTS.
| return self.drop(x + self.pe[:, : x.size(1), :]) | ||
|
|
||
|
|
||
| class _TransformerBlock(nn.Module): |
There was a problem hiding this comment.
should we add it to layer/_transforms or something?
There was a problem hiding this comment.
I think creating a new _transforms folder wouldn't be necessary here because it would be confusing for new contributors as transform and transformers are 2 different terminologies. Furthermore, we already have an established layers/_blocks/ directory. A transformer block is standard neural network block logic, so keeping it grouped with other blocks in layers/_blocks/_transformer_block.py works perfectly entirely to our existing layout.
|
Hi @phoeenniixx , |
|
Hi @phoeenniixx, Kindly review this and let me know if we're done with this one so I'd add the |
There was a problem hiding this comment.
what is the use of this folder?
There was a problem hiding this comment.
The model file already imports directly from the canonical locations. I'll remove the _units layer folder entirely.
| ) | ||
|
|
||
|
|
||
| def test_basic_model_initialization(model, basic_metadata): |
There was a problem hiding this comment.
i think test framework is enough for these tests?
There was a problem hiding this comment.
Yes, the extra tests are redundant I've removed that.
phoeenniixx
left a comment
There was a problem hiding this comment.
I think it is almost ready. Just please de-duplicate the tests that can already being tested using the test framework
| "info:name": "UniTS", | ||
| "info:compute": 4, | ||
| "authors": ["Muhammad-Rebaal", "gasvn", "sohamukute"], | ||
| "python_dependencies": ["torch"], |
There was a problem hiding this comment.
why do we need this? isnt torch already a core dep here?
There was a problem hiding this comment.
Actually I implemented this seeing the model extension v2. So I think I its an essential tag to be added torch as dependencies, not knowing the core-dep can't be added. Although I removed that.
| {}, | ||
| { | ||
| "patch_len": 8, | ||
| "stride": 4, |
There was a problem hiding this comment.
please add some loss functions as well here - Is this model only compatible with point prediction losses, or can it also handle quantile and distribution losses?
There was a problem hiding this comment.
In the paper the model it is mentioned only about point prediction losses but we can extend it so I extend it to both quantile and distribution losses.
There was a problem hiding this comment.
why do we have this file? I dont understand, it has just have a few fixtures and no clear tests are written,
Can you explain what this file is exactly doing?
There was a problem hiding this comment.
Actually the model was initially implemented by the use of TslibBaseModel which later converted to EncoderDecoder so I removed those tests manually and new tests yet to be added which I added in my latest commit.
phoeenniixx
left a comment
There was a problem hiding this comment.
Please remove the test file, or add some tests there - currently it feels like an AI hallucination
Changes made in the recent commit:
|
| return EncoderDecoderTimeSeriesDataModule | ||
|
|
||
| @classmethod | ||
| def get_base_test_params(cls): |
There was a problem hiding this comment.
As I mentioned earlier, get_base_test_params is not going to work for now for v2
use get_test_train_params instead.
for that you need to remove pred_type, y_type tags
There was a problem hiding this comment.
looping over all the losses is not a good idea rn for v2
There was a problem hiding this comment.
Aren't these tags pred_type, y_type be removed from here : https://github.com/sktime/pytorch-forecasting/blob/main/extension_templates/v2/model_simple/model_pkg.py if these are part of get_base_test_params ?
looping over all the losses is not a good idea rn for v2
Can you suggest me the preferred ones ? so I'd add those
There was a problem hiding this comment.
Aren't these tags
pred_type,y_typebe removed from here : https://github.com/sktime/pytorch-forecasting/blob/main/extension_templates/v2/model_simple/model_pkg.py if these are part ofget_base_test_params?
I have reservations about this :)
Yes, we can remove this, but we will have to add it again, once the work on updating the test framework is done - which is the next work item on my list. As, we dont have much contributors around at this time, we can manage to ignore this. wdyt?
|
Hi @phoeenniixx ,
Also I add another change:
|
| instance. ``create_test_instance`` uses the first (or only) dictionary in | ||
| ``params``. | ||
| """ | ||
| from pytorch_forecasting.metrics import NormalDistributionLoss, QuantileLoss |
There was a problem hiding this comment.
I am not sure if DistributionLoss works well with v2 rn, does it?
There was a problem hiding this comment.
No I didn't find any implementation as well that's why I asked you above. That's why in my last commit I remove the code implementation as well but forget to remove this dead import sorry for confusion
There was a problem hiding this comment.
Is there any open issue regarding the support of Distribution loss in v2 models ?
Fixes #2158
Hi @fkiraly , @phoeenniixx, @PranavBhatP !
I have implemented the UniTS (Unified Time Series Model) within the PyTorch Forecasting v2 architecture.
Could you please review the PR?
Here is a summary of the changes made:
pytorch_forecasting/layers/_units/): Implemented the core neural network components (_PatchEmbedding,_PositionalEncoding,_TransformerBlock) completely isolated from the standard base estimating logic to strictly adhere to the project's v2 layer standards.pytorch_forecasting/models/units/): Created the mainUniTSwrapper which inherently inherits fromTslibBaseModeland handles feature input alignments, channel-independence, and forward pass routing to the abstracted layers.UniTS_pkg_v2class representing model metadata (authors,tags) and defined automated testing configurations (get_test_train_params).tests/test_models/test_units_v2.py) validating parameter exceptions, tensor shapes, gradient flow bounds, and default hyperparameter states.Thank you!