Skip to content

Conversation

@gdevos010
Copy link
Contributor

@gdevos010 gdevos010 commented Sep 23, 2024

Checklist before merging this PR:

  • Mentioned all issues that this PR fixes or addresses.
  • Summarized the updates of this PR under Summary.
  • Added an entry under Unreleased in the Changelog.

TODO

  • update doc strings
  • add tests
  • add proper attribution
  • add futures covariate support (this will probably need to be seperate MR)

Fixes #2537.

Summary

Adds the TimesNet mode based on this code
https://github.com/thuml/Time-Series-Library/blob/main/models/TimesNet.py

Other Information

Forecast_0_0 264_TimesNetModel_batch_size=16,hidden_size=128,num_layers=2,num_kernels=2,top_k=5,use_reversible_instance_norm=True

Forecast_0_1 786_TimesNetModel_batch_size=32,hidden_size=16,num_layers=3,num_kernels=4,top_k=5,use_reversible_instance_norm=True

@gdevos010 gdevos010 marked this pull request as draft September 23, 2024 03:45
@gdevos010 gdevos010 marked this pull request as ready for review September 25, 2024 18:35
@codecov
Copy link

codecov bot commented Sep 25, 2024

Codecov Report

Attention: Patch coverage is 95.19651% with 11 lines in your changes missing coverage. Please review.

Project coverage is 94.11%. Comparing base (d909589) to head (c985d54).
Report is 65 commits behind head on master.

Files with missing lines Patch % Lines
darts/models/forecasting/times_net_model.py 93.20% 7 Missing ⚠️
darts/models/components/embed.py 97.56% 3 Missing ⚠️
darts/models/__init__.py 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2538      +/-   ##
==========================================
- Coverage   94.14%   94.11%   -0.04%     
==========================================
  Files         139      141       +2     
  Lines       14884    15113     +229     
==========================================
+ Hits        14013    14223     +210     
- Misses        871      890      +19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@gdevos010
Copy link
Contributor Author

gdevos010 commented Sep 29, 2024

@dennisbader This model is ready for review.

Copy link
Contributor

@madtoinou madtoinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for this great PR @gdevos010 and sorry for the delay, we have been quite busy with others responsibilities.

Some comments to reduce the code redundancy in the implementation of this quite complex model.

Also, could you please also include an example notebook to compare it to models such as TiDEModel and/or TSMixerModel on a toy example, just to make sure that the default configuration yield acceptable results?

from darts.utils.torch import MonteCarloDropout


class PositionalEmbedding(nn.Module):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the PositionalEncoding class already implements this logic in darts/models/forecasting/transformer_model.py, let's maybe move it here and keep the old name

Comment on lines +88 to +97
w = torch.zeros(c_in, d_model).float()
w.require_grad = False

position = torch.arange(0, c_in).float().unsqueeze(1)
div_term = (
torch.arange(0, d_model, 2).float() * -(math.log(10000.0) / d_model)
).exp()

w[:, 0::2] = torch.sin(position * div_term)
w[:, 1::2] = torch.cos(position * div_term)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this code snippet can also be found in PositionalEncoding, let's abstract it to reduce redundancy

return self.dropout(x)


class DataEmbedding_inverted(nn.Module):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
class DataEmbedding_inverted(nn.Module):
class DataEmbeddingInverted(nn.Module):

return self.dropout(x)


class DataEmbedding_wo_pos(nn.Module):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic is so similar to DataEmbedding and DataEmbedding_inverted that they should probably be combined into a single class and the logic difference should be implemented in the forward() method instead (by adding a parameter/attribute type that could take the values "normal", "inverted" or "wopos".

from darts.models.forecasting.torch_forecasting_model import PastCovariatesTorchModel


class Inception_Block_V1(nn.Module):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just of the sake of homogeneity.

Suggested change
class Inception_Block_V1(nn.Module):
class InceptionBlock(nn.Module):

..
random_state
Control the randomness of the weights initialization. Check this
Control the randomness of the weight's initialization. Check this
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's revert this change, since there are several weights, the apostrophe does not seem appropriate.

Or we could eventually rephrase it into "Control the randomness in the initialization of the weights" if you have the impression that the original sentence was not clear.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rename the file "timesnet_model.py" so that it's homogeneous with the others

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rename the file "test_timesnet_model.py" to keep the number of underscores to the minimum

- Added support for retraining model(s) every `n` iteration and on custom conditions in `historical_forecasts` method of `ForecastingModel`s. [#1139](https://github.com/unit8co/darts/pull/1139) by [Francesco Bruzzesi](https://github.com/fbruzzesi).
- Added support for beta-NLL in `GaussianLikelihood`s, as proposed in [this paper](https://arxiv.org/abs/2203.09168). [#1162](https://github.com/unit8co/darts/pull/1162) by [Julien Herzen](https://github.com/hrzn).
- New LayerNorm alternatives, RMSNorm and LayerNormNoBias [#1113](https://github.com/unit8co/darts/pull/1113) by [Greg DeVos](https://github.com/gdevos010).
- New LayerNorm alternatives, RMSNorm and LayerNormNoBias [#1113](https://github.com/unit8co/darts/issues/1113) by [Greg DeVosNouri](https://github.com/gdevos010).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorrect link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add TimesNet model

2 participants