Here are some debug screenshots to prove my claim (seq_last gets changed to 0):

Hi @turbotimon and thanks a lot for raising this issue 👍
This indeed looks like a bug. I've also tested it and the performance is much better with the fix 🚀

Regarding why normalization was disabled by default (from #1292 by @hrzn).

In my understanding, the normalization part of NLinear does not apply to the probabilistic case in general (or at least not without a bit of extra logic), as it does not make sense to add original domains values to arbitrary distribution parameters (e.g. variance) => for the time being, I made the normalization optional, and disabled support for probabilistic likelihoods in case it is enabled.

However, since it raises an error when using a Likelihood (probabilistic model), it probably should have been True by default.

I think we can set True by default, but make it clear in the Changelog that this can be a breaking change, since probabilistic models will the raise an error by default.

So let's do this:

• fix the bug
• make normalization=True by default (add a note about breaking change)
• we could also allow normalization when likelihood is a QuantileRegression, since there the normalization would be valid.

Regarding normalization of covariates. I'm a bit hesitant to just apply it to all covariates. We had a discussion already about it in the last changes to the model #1873. :)

I add it to our backlog. Would you be interested in contributing to this fix?

I think we can set True by default, but make it clear in the Changelog

Agree. In addition, I think a warning:: in the model docstring for normalize would also be appropriate for it is a braking change.

we could also allow normalization when likelihood is a QuantileRegression, since there the normalization would be valid.

Sounds reasonable

Regarding normalization of covariates. I'm a bit hesitant to just apply it to all covariates. We had a discussion already about it in the last changes to the model #1873.

I see your point for the special case of future covariates like datetime attributes. However, I strongly agree with this statements from felixdivo:

For other types of data, where the absolute date is irrelevant, eliminating some drift over time can be very useful. And that is why it was proposed in the original publication.
#1873 (comment)

For example, I currently have the case of covariates that show similar cyclic patterns as the target, but very different trend levels over time. This is way I came up with this Idea of a "per sample normalization", just to find out that something like this already exists (as always :) ). However, I can not use the current (even fixed) implementation for my purpose.

In my opinion, the darts implementation should be as close as possible to the original paper, only extending it where possible or fixing major flaws. At least that's what I would expect as a user of the library.

I see the following solutions:

1. If true apply normalization to all covariates with an note:: in the documentation, that this is affecting all covariates and therefore may prevents the meaningful use of covariates where the level matters (such as datetime attributes).

2. Optional: In addition, allowing fine controlling of the normalization e.g. by allowing to turn off/on the normalization per componenent. E.g. instead of true allowing a tuple (series, past, future) of True/False or again tuples of true/false for every single on). Hower, this woud be a complex logic.

Let me know what you guys think of it.

Would you be interested in contributing to this fix?

I would like to do so (as I need it to fix anyway for me personal to use)

Agree. In addition, I think a warning:: in the model docstring for normalize would also be appropriate for it is a braking change.

True :) But I'd avoid having a warning raised everytime someone creates an NLinearModel, so a "🔴" (Breaking Change) note in the CHANGELOG.md should be enough.

I see your point for the special case of future covariates like datetime attributes. However, I strongly agree with this statements from felixdivo:

I'm totally with you that applying it to covariates as well should be possible. The reason I'm reluctant is that I'd want to find a unified solution for this accross all models / normalizations (also use_reversible_instance_norm (RINorm) and other models that use similar type of normalization).

Allowing component-wise selection would be the ultimate goal, as in many use cases datetime encodings are used together with other covariates. :)

In my opinion, the darts implementation should be as close as possible to the original paper, only extending it where possible or fixing major flaws. At least that's what I would expect as a user of the library.

That's true but we also add additional features to models that didn't exist in the source paper to improve model performance (see RINorm for example, probabilistic support for all models, ...). So if we can find a neat way to generalize this logic, I'm all in.

I would like to do so (as I need it to fix anyway for me personal to use)

Thanks, that's great to hear :) 🚀

@dennisbader I found another discussible thing in the implementation: With const_init=True, the weights in the layer will be const init, but the bias is still random. What do you think about that? I would suggest that the weights could be set to 0.0 in this case. However, I'm not sure about that and the paper is unspecific in that case. The only thing I could find is "we initialize the weights of the linear layers in DLinear as 1/L rather than random initialization.".

P.S. Still looking forward to making a merge request (also because the copy() error currently is only fixed in the shared_weights=False part)

@dennisbader I finally found time to fix the nlinear #2757. Details on the fixes in the PR summary.

Some important remarks for the tests in test_dlinear_nlinear.py::TestDlinearNlinearModels.test_multivariate_and_covariates. I struggled for a while, but then decided to leave them untouched (for normalize=false) and implement others for the normalize=true:

• darts/darts/tests/models/forecasting/test_dlinear_nlinear.py

  Line 207 in 4277679

  [fut_cov1, fut_cov2] if past_cov1 is not None else None

  looks like a error to me (using fut_cov1 for the past_covariates)? But anyway, the models are never tested with past covariates in this tests (it was only one test for normalize=true which i replaced, but e.g. dlinear is never not with past covariates)
• I'm questioning the test in general, as according to the paper, dlinear is most useful with trend and nlinear with distribution shifts in the data, but the test data used is stationary. Therefore, nlinear performs not good in these tests and dlinear is not tested for its intended usage/benefit.