Fix NLinear normalization

[turbotimon]

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.

Fixes #2699.

Summary

• Fixes the normalization also for the shared_weighs=True path (not covered by fix nlinear norm #2724)
• Uses normalize=True as the default now with an updates docstring.
• Past covariates are now also normalized. But no future covariates to allow further use for temporal characteristics (e.g. via add_encoders)
• Improved tests

Full discussion and details, see #2699.

Other Information

[turbotimon]

Some test still fail, and I'm not sure why. They can be fixed by changing the model to normalize=False, but I'm not sure why.

For example test_lagged_training_data_unequal_freq fails because the values have a differend order?:

expected_X.squeeze()
> array([23.33333333, 24.33333333, 25.33333333, 26.33333333, 25.55555556, 26.55555556, 27.55555556, 28.55555556])
X.squeeze()
> array([23.33333333, 24.33333333, 26.33333333, 25.33333333, 26.55555556, 25.55555556, 27.55555556, 28.55555556])

[codecov]

Codecov Report

Attention: Patch coverage is 25.00000% with 24 lines in your changes missing coverage. Please review.

Project coverage is 43.92%. Comparing base (a92fa41) to head (add1af5).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
...s/tests/models/forecasting/test_dlinear_nlinear.py	0.00%	24 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2757      +/-   ##
==========================================
- Coverage   43.95%   43.92%   -0.04%     
==========================================
  Files         223      223              
  Lines       33252    33274      +22     
==========================================
- Hits        14617    14614       -3     
- Misses      18635    18660      +25     

☔ 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.

[dennisbader]

Thanks for PR and the fixes @turbotimon 🚀 I agree with setting normalize=True as default.

I left some comments regarding the past covariates normalization, where I believe we should try to find on a more long-term solution.

Also, it looks like there is still something fishing going on with normalize=True and auto-regression (forecast horizon n > output_chunk_length). This is already in the current master head, so not introduced by this PR. I left a comment in the tests. Would you mind having a look at that?

[dennisbader]

Thanks for giving this a go. However, I believe we should find a more general solution that we discussed in the issue.

Every change that we introduce here will be a "breaking" change for users that want to load the weights of a model stored with an earlier Darts version (in the sense that the weights were trained on an input with different value space).

So if we also want to add this for future covariates (the non-datetime attrs) then we would have to "break" it again.

In my opinion, either we find a robust solution that covers everything, or we leave the normalization only on the target for now.

By robust I'm thinking about the RINorm changes that we discussed in #2758. That would also allow to use the old behavior (target-only) and optionally also have fine-grained control over covariates normalization.

How does that sound?

[turbotimon]

I agree do some extent, but think we shouldn't overthink this model. It is, as the author says an "embarrassingly simple model" and not sure how many extra features it can handle. Enable the normalization for covariates is just closest to the paper. Except maybe for future (at least for the real future part, not historic future) covariates which are handled by a separate layer anyway in this implementation, so it's ok to handle it different. So I'm not sure if it's a good idea making the implementation more and more complicated if the model most likely can not handle it..

[dennisbader]

I was curious why this would have larger error than the non-normalized one, so I tried it out (I ran a simplification of the test that was failing with 50, e.g. test_single_ts, ...):

import matplotlib.pyplot as plt
import numpy as np
from darts.models import NLinearModel
from darts.datasets import AirPassengersDataset

series = AirPassengersDataset().load().astype(np.float32)
model = NLinearModel(
    input_chunk_length=24,
    output_chunk_length=12,
    random_state=0,
    n_epochs=100,
    normalize=True,
)
model.fit(series[:-36])
preds = model.predict(n=36, series=series[:-36])
series.plot()
preds.plot()
plt.show()

Seems like auto-regression is not performing as expected (this also happens in the current master):

[turbotimon]

If I see it right, the problem happens somewhere here, where the input_past (in your example len=24) get filled with 12 zeros on the left followed with the first output (len=12).

In my opinion, this is wrong and the zeros should be the past series instead. So the step size should be only output_chunk_length (12) and not input_chunk_length (24). I tried use roll_size=12 to overcome this, but got an exception...

You see this drift effect clearly with constant series, then, the input_past will be e.g. [0.0]*12 + [value]*12 and therefore the forecast drops by value/2 in every autoregressive step.

import matplotlib.pyplot as plt
import numpy as np
from darts.models import NLinearModel
from darts.datasets import AirPassengersDataset
import darts.utils.timeseries_generation as tg

series = tg.constant_timeseries(length=240, value=10, freq="M") # value=10.0

model = NLinearModel(
    input_chunk_length=24, # drift does not happen when same size as output_chunk_length
    output_chunk_length=12,
    random_state=1,
    n_epochs=100,
    normalize=True,
)
model.fit(series[:-36])

preds = model.predict(n=36, series=series[:-36]
    # roll_size=12 # exception (but maybe i don't fully understand the parameter)
)

series.plot()
preds.plot()

It is however not present, when input_chunk_length is the same size as output_chunk_length.

Is this a general bug?

[dennisbader]

For me setting the role_size=12 works fine (still the issue though with auto-regression). Also, the role_size is to output_chunk_length if the user doesn't provide it. See here:

darts/darts/models/forecasting/torch_forecasting_model.py

Line 1626 in 836436d

roll_size = self.output_chunk_length

[dennisbader]

I managed to fix it. We also had to clone x since we mutate the values in the norm. See the improved results now below.

I also removed the detach() from seq_last because I believe (and from checking the results) it is not required. A simple clone() works.

[turbotimon]

Great! Yes, now I see why it is necessary to clone x as well. Two things:

1. Maybe a comment for x.clone() is reasonable because we have seen this is not obvious?
2. I think detach() (even if it doesn't change the outcome) is a helpful optimization because it gets excluded from the differentiation graph. See https://pytorch.org/docs/stable/generated/torch.clone.html

[dennisbader]

Hi @turbotimon, I added the related to cloning x.
Everything is ready now. Thanks again for the great PR and apologies that this took a while :) 🚀