unit8co / darts

A python library for user-friendly forecasting and anomaly detection on time series.
https://unit8co.github.io/darts/
Apache License 2.0
7.92k stars 860 forks source link

[BUG] Bug when using multiple Scalers constructed with no arguments. #314

Closed mkmkmk124 closed 3 years ago

mkmkmk124 commented 3 years ago

Describe the bug

After learning and transforming multiple time series data by using the fit_transform function of multiple Scaler objects constructed with no arguments, if we try to restore each time series by using the inverse_transform function, the original time series will not be restored except for the last learned time series.

To Reproduce

from darts import TimeSeries
import pandas as pd
import matplotlib.pyplot as plt
from darts.dataprocessing.transformers import Scaler

plt.figure();
timeseries1 = TimeSeries(pd.DataFrame([1,2,3,4,5],index=pd.date_range(start='1/1/2017', periods=5, freq='MS')))
timeseries2 = TimeSeries(pd.DataFrame([50,40,30,20,10],index=pd.date_range(start='1/1/2017', periods=5, freq='MS')))
timeseries1.plot(label="time_series_1")
timeseries2.plot(label="time_series_2")
plt.legend();
plt.title("Fig. 1");

plt.figure();
scaler_1, scaler_2 = Scaler(), Scaler()
timeseries1_scaled = scaler_1.fit_transform(timeseries1)
timeseries2_scaled = scaler_2.fit_transform(timeseries2)
timeseries1_scaled.plot(label="time_series1_scaled")
timeseries2_scaled.plot(label="time_series2_scaled")
plt.legend();
plt.title("Fig. 2");

plt.figure();
timeseries1_inversed = scaler_1.inverse_transform(timeseries1_scaled)
timeseries2_inversed = scaler_2.inverse_transform(timeseries2_scaled)
timeseries1_inversed.plot(label="time_series1_inversed")
timeseries2_inversed.plot(label="time_series2_inversed")

plt.legend();
plt.title("Fig. 3");

fig1 fig2 fig3

Expected behavior

Fig. 1 and Fig. 3 are expected to have the same plot.

Details to be corrected

Change the default value of the scaler argument in the constructor of the Scaler class to None, and implement the processing for the case of None inside the constructor.

Before:

 def __init__(self, scaler=MinMaxScaler(feature_range=(0, 1)), name="Scaler"):
        """
        Generic wrapper class for using scalers that implement `fit()`, `transform()` and
        `inverse_transform()` methods (typically from scikit-learn) on `TimeSeries`.
        Parameters
        ----------
        scaler
            The scaler to transform the data.
            It must provide the `fit()`, `transform()` and `inverse_transform()` methods.
            Default: `sklearn.preprocessing.MinMaxScaler(feature_range=(0, 1))`; this
            will scale all the values of a time series between 0 and 1.
        name
            A specific name for the scaler
        """
        super().__init__(name)

        if (not callable(getattr(scaler, "fit", None)) or not callable(getattr(scaler, "transform", None))
                or not callable(getattr(scaler, "inverse_transform", None))): # noqa W503
            raise_log(ValueError(
                      'The provided transformer object must have fit(), transform() and inverse_transform() methods'),
                      logger)

        self.transformer = scaler
        self.train_series = None

After:

 def __init__(self, scaler=None, name="Scaler"):
        """
        Generic wrapper class for using scalers that implement `fit()`, `transform()` and
        `inverse_transform()` methods (typically from scikit-learn) on `TimeSeries`.
        Parameters
        ----------
        scaler
            The scaler to transform the data.
            It must provide the `fit()`, `transform()` and `inverse_transform()` methods.
            Default: `sklearn.preprocessing.MinMaxScaler(feature_range=(0, 1))`; this
            will scale all the values of a time series between 0 and 1.
        name
            A specific name for the scaler
        """
        super().__init__(name)

        if (not callable(getattr(scaler, "fit", None)) or not callable(getattr(scaler, "transform", None))
                or not callable(getattr(scaler, "inverse_transform", None))): # noqa W503
            raise_log(ValueError(
                      'The provided transformer object must have fit(), transform() and inverse_transform() methods'),
                      logger)

        if scaler is None: 
            self.transformer = MinMaxScaler(feature_range=(0, 1))
        else:
            self.transformer = scaler
        self.train_series = None
tneuer commented 3 years ago

Hello @mkmkmk124. Thanks for bringing this to our attention! Of course the problem you describe is not the intended behaviour. I implemented your suggested changes and make a PR for it as soon as possible. The bug should be fixed in the next release and we keep our eyes open for similar constructors and change them as well!

mkmkmk124 commented 3 years ago

Thank you for your quick response!

hrzn commented 3 years ago

This one is fixed now.