Skip to content

time series forecasting: window features - #380

Closed
Morgan-Sell wants to merge 47 commits into
feature-engine:mainfrom
Morgan-Sell:time-series-window
Closed

time series forecasting: window features#380
Morgan-Sell wants to merge 47 commits into
feature-engine:mainfrom
Morgan-Sell:time-series-window

Conversation

@Morgan-Sell

Copy link
Copy Markdown
Collaborator

Closes #343.

Notes from #343:

The transformer should create computations over windows of past values of the features, and populate them at time t, t being the time of the forecast.

It uses pandas rolling, outputs several comptutations, mean, max, std, etc, and pandas shift to move the computations to the right row.

tmp = (data[variables]
       .rolling(window='3H').mean()  # Average the last 3 hr values.
       .shift(freq='1H')  # Move the average 1 step forward
       )

# Rename the columns
tmp.columns = [v + '_window' for v in variables]

data = data.merge(tmp, left_index=True, right_index=True, how='left')

@Morgan-Sell

Copy link
Copy Markdown
Collaborator Author

@solegalli,

For the above code to work, variables must be a list. Otherwise, we cannot add the _window suffix to the transformed variable because df[variables] is a pandas series. How would you like to handle it?

  • The user could be required to submit a list.
  • In the fit(), we could check if self.variables is a list type, if not, we make change the type to a list.

Is there a case where variables should be allowed to be an integer?

@solegalli

Copy link
Copy Markdown
Collaborator

Hi @Morgan-Sell

Pleas check our variable_manipulation functions. They've got what you need.

@Morgan-Sell

Morgan-Sell commented Feb 26, 2022

Copy link
Copy Markdown
Collaborator Author

Hi @solegalli,

I'm plugging away on WindowFeatures. I'm using LagFeatures as a guide.

I noticed a couple of things regarding LagFeatures:

  • LagFeatures does not check whether the init param freq is a string. It doesn't raise an error b/c there is a default value for periods. Should we consider the edge case when a user enters None for periods?
  • test_error_when_non_permitted_param_periods does not test True or False. These values do not raise an error.

Was this intentional?

@solegalli

Copy link
Copy Markdown
Collaborator

Yes, we leave all the checks to pandas.

@solegalli solegalli left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hi @Morgan-Sell

Thank you so much for such an incredible amount of work!

This is looking really good. Thank you so much.

I made a few comments that are mainly organizational.

Much of the code in this class is identical to LagFeatures, so I think we should move the bits that are similar to a base forecasting class, because it will be easier to maintain.

The second is regarding reorganizing the tests. Those tests that are similar for window and lag features, should also be moved to a common file we already have in the forecasting test folder.

Would you mind having a look at this?

Thanks again!

Comment thread feature_engine/timeseries/forecasting/window_features.py Outdated
Comment thread feature_engine/timeseries/forecasting/window_features.py Outdated
# if freq is not None, it overrides periods.
if self.freq is not None:
tmp = (X[self.variables_]
.rolling(window=self.window).apply(self.function)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@KishManani

the param windows takes strings and integers. Is this enough? should we consider anything else?

@KishManani KishManani Mar 6, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it covers most use cases. However, in Pandas there's also the option to provide a custom class to define windows (see Pandas rolling docs BaseIndexer subclass). If the effort required to support it is high, it may not be worth it and can be added in the future.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is what I've done for this parameter:

  • removed all init checks. We leave that to pandas. Like this, this param can take whatever pandas accepts.
  • in addition, I also allow passing a a list with window values, so that several window features can be calculated with one transformer.
  • in theory, the user could pass a list of callables, but I have not tested this process. @KishManani do you have an example of a useful feature that could be created with a bespoke function? we can use this in a test and more importantly in the documentation.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't have a good example to hand. My recommendation is to implement it only if an issue is raised with a good use case.


return X

def get_feature_names_out(self, input_features: Optional[List] = None) -> List:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is this method identical to lag features? if yes, then let's move it to the base class.

@Morgan-Sell Morgan-Sell Mar 1, 2022

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It's different. It includes the window in the name, e.g. var_A_window_2_freq_3d and var_B_window_3_periods_2.

The two classes do you share the code below. Should we move this code to the BaseForecast class?

check_is_fitted(self)

        # create names for all window features or just the indicated ones.
        if input_features is None:
            input_features_ = self.variables_
        else:
            if not isinstance(input_features, list):
                raise ValueError(
                    f"input_features must be a list. Got {input_features} instead."
                )
            if any([f for f in input_features if f not in self.variables_]):
                raise ValueError(
                    "Some features in input_features were not transformed. This method only "
                    "provides the names of the transform features with this method."
                )
            # create just indicated window features
            input_features_ = input_features

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I modified this method in Morgan-Sell#9

WindowFeatures(periods=None, freq=_freqs)


def test_get_feature_names_out(df_time):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

is this test identical to the one for lag features? if yes, let's move it to the common tests file, and pass the transformer as an argument with parametrize.

The common test file is where we check the tests from check_estimator.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It's different because "window_X" is included in the transformed variables. See the comment above.

self,
variables: List[str] = None,
window: Union[str, int] = 1,
function: Callable = np.mean,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I have two questions for you guys @Morgan-Sell and @KishManani

First, in other transformers I call this parameter "operations" instead of "functions". I'd like to unify the way we call parameters that do similar things throughout the package. So my question is: which is a better name for this parameter? "operations", "functions", something else, or it does not really matter? If it is not important, I would prefer "operations" so I don't have to deprecate in the other transformer.

And second, I see that @Morgan-Sell is passing numpy functions here. But pandas agg() takes strings. So, should this parameter take strings, numpy functions, or we don't care because in any case, pandas takes care of this check? and we just allow everything?

Quick note: whenever functionality is already implemented in packages that have great developer support, like pandas, sklearn and scipy, I tend to leave "the work" to them (that is, their methods and classes), because we are a tiny community in comparison, so maintaining for us is harder.

Thank you!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@solegalli @KishManani

  1. Does sklearn use func as variable name when accepting a function? Pandas uses func when the user passes a function.

  2. As of now the code doesn't use the agg method. It uses the apply method. I don't think apply accepts a string.

@KishManani KishManani Mar 6, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we need to consider what kind of transformations we want to accept. agg is orders of magnitude faster for operations like sum and mean relative to apply (like ms for agg vs seconds for apply). Also the agg API is a bit more flexible in allowing you to specify multiple different transformations for different variables using a dictionary (see second highest comment here).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In Morgan-Sell#9 I modified the functionality of the class so that it works with agg instead of apply.

Agg can take strings representing each one of the rolling() allowed aggregation functions. That is convenient.

What the current functionality is not allowing, is for the user to pass a bespoke function. Which might be upsetting.

We have one of 2 options:

  1. release like this and wait for user feedback
  2. try to think how we can allow the use of bespoke functions

For 2, it would be helpful if @KishManani has an example of what this could be?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bespoke functions to pass to agg is anything that aggregates the time series into a scalar value (e.g., a quantile, the kurtosis, the entropy).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also, allowing a custom function allows you to do weighted averages too:

weights = np.array([0.1, 0.2, 0.3, 0.4])
df.rolling(window=4).agg(lambda x: np.sum(x * weights))

Comment thread feature_engine/timeseries/forecasting/window_features.py Outdated
f"freq must be a string. Got {freq} instead."
)

if missing_values not in ["raise", "ignore"]:

@solegalli solegalli Feb 28, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this parameter and the next one are also in lag features. I would make them part of a base class,.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Just to confirm, are missing_values and drop_original the only init params to be included in BaseForecast init?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

i noticed that variables should also be part of the common tests. Added that in Morgan-Sell#9

…in_index, and test_sort_index to test_check_estimator_forecasting.py
@KishManani

Copy link
Copy Markdown
Contributor

I would also include an option to allow expanding windows: https://pandas.pydata.org/docs/reference/api/pandas.DataFrame.expanding.html

# We need the dataframes to have unique values in the index and no missing data.
# Otherwise, when we merge the window features we will duplicate rows.

if X.index.isnull().sum() > 0:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there any reason for doing this over this:

if X.index.isnull().any():

if not I believe my suggestion is the preferred method for such checks.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

fixed.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

few questions regarding the co

feature_engine checks are not failing for me in this pr Morgan-Sell#9

sklearn check_estimators tests are failing. And not sure why yet.

@solegalli

Copy link
Copy Markdown
Collaborator

I would also include an option to allow expanding windows: https://pandas.pydata.org/docs/reference/api/pandas.DataFrame.expanding.html

Regarding expanding windows, I tried to make this optional, but pandas.expanding windows takes an argument less than rolling windows, so it gets messy with the docs.

Also, it gets even more messy now that the parameter that is missing in pandas expanding is the one that allows the user to create multiple features for different window sizes.

So I think, the best is, to have expanding windows in a different class.

On that note, I am unsure as to how useful expanding windows as implemented in pandas, are to create features for forecasting. In my mind, I would like expanding windows with limits, and not extending all the way from the first observations. For example, I would like the ast 3,4, 5, and 6 months of data, but most likely not all data from the beginning. And this is not covered in pandas expanding which is just brute force from the first observations.

With WindowFeatures, if we pass the window values in a list, we can, in essence, create expanding windows as the ones I describe.

So question for @KishManani is, is pandas.expanding really useful in forecasting? If yes, we should create a separate issue for a separate class.

And the same for the other pandas functions like weighted windows since we are here :p

@solegalli

Copy link
Copy Markdown
Collaborator

Hi @Morgan-Sell @KishManani

Thank you so much for your valuable contributions to this class.

I made a PR to Morgan's repo here: Morgan-Sell#9 where I address most of the questions and issues that came up in this PR thread.

The main things outstanding are:

  1. expand the tests for when window is a list, flagged with TODO (@Morgan-Sell )
  2. fix test for sort_index, flagged with TODO (@Morgan-Sell )
  3. fix sklearn check_estimator (@solegalli )
  4. expand the docs user guide, flagged with TODO (@solegalli )
  5. create a new issue for expanding windows (@KishManani )
  6. test passing a base indexer to window (if @KishManani has an example, we can take it from there)
  7. see if we can allow bespoke functions instead of just add with strings (team thinking)

I put some tentative names next to each issue. Would this be alright for all of you?

@Morgan-Sell if you go ahead with merging and the first changes, I 'll pick up afterwards with the sklearn tests.

Thank you all!

@solegalli

Copy link
Copy Markdown
Collaborator

@KishManani

When recreating the notebooks for section 3, I realise that it would be convenient to have the features created for timestamps not present in the train data.

For example, if last timestamp of train data is 2020-01-01, and say we have daily data. If we create lag 1 day features, we would have the value for timestamp 2020-01-02, however, the current implementation of lag features (and also window features) don't add these values, because they would add values up to the timestamp in the train data.

Is this desired? or would we want the future value as well?

I am thinking for multistep forecasting, I want to pass the value of the feature at time t, and want the transformer to return t+1, this would not work if the dataframe contains 1 row only at the moment.

thoughts?

@Morgan-Sell

Morgan-Sell commented Mar 13, 2022

Copy link
Copy Markdown
Collaborator Author

Hi @solegalli and @KishManani,

I'm back! I was visiting family on the east coast. Thanks for all the great work while I was gone!

@solegalli,

  1. I'm game to run point on:
- expand the tests for when window is a list, flagged with TODO (@Morgan-Sell )--
- fix test for sort_index, flagged with TODO (@Morgan-Sell )
  1. Your abovementioned idea of having a window function for missing data is interesting! In energy, it is not uncommon for a SCADA to miss recording time sequences. Consequently, there are missing values for energy generation, irradiation, etc. I see the window function that you are proposing to serve as a fillna in such scenarios

  2. [Question] We haven't created fit_transform() methods in these classes. Does the TransformerMixin class autogenerate fit_transform if the created class - e.g. WindowFeatures - has both fit() and transform() methods?

@KishManani

Copy link
Copy Markdown
Contributor

#380 (comment)

So question for @KishManani is, is pandas.expanding really useful in forecasting? If yes, we should create a separate issue for a separate class.

And the same for the other pandas functions like weighted windows since we are here :p

Indeed, creating windows of various sizes is different from expanding windows. Expanding windows is the expansion of a window from the start and contains all data points. The main feature I've seen created using an expanding window is taking the mean of the entire history. In my experience these features tend not to be as important as looking at averages over recent windows. Worth adding a separate class then.

Weighting can be helpful. For, example in giving more weight to recent time periods compared to ones in the past. I've normally achieved this by passing a custom function into agg rather than specifying win_type.

@KishManani

KishManani commented Mar 13, 2022

Copy link
Copy Markdown
Contributor

#380 (comment)

We will need to project features into the future at predict time for forecasting. However, I don't think that logic should belong to these transformers. I think it is simpler and more intuitive that the transformers should return the values of the lag and window features for the time periods provided in the inputted dataframe in the .transform() method.

)


def test_sort_index(df_time):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This test could be simplified to the following:

from pandas.testing import assert_frame_equal
def test_sort_index(df_time):
    # Shuffle dataframe
    Xs = df_time.sample(frac=1)

    transformer = WindowFeatures(sort_index=False)
    df_tr = transformer.fit_transform(Xs)
    assert_frame_equal(df_tr[transformer.variables_], Xs[transformer.variables_])

    transformer = WindowFeatures(sort_index=True)
    df_tr = transformer.fit_transform(Xs)
    assert_frame_equal(
        df_tr[transformer.variables_], Xs[transformer.variables_].sort_index()
    )

pandas `shift()`.

sort_index: bool, default=True
Whether to order the index of the dataframe before creating the lag features.

@KishManani KishManani Mar 20, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Typo: I think it should say window features. rather than lag features.

X[self.variables_]
.rolling(window=win)
.agg(self.functions)
.shift(periods=self.periods, freq=self.freq)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What is the use case to shift the rolling window feature by greater than one period? If there isn't one, then removing it could help simplify this class.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think it is a fair point.

I just worry that some users will be using pandas periods mostly for shift, and then obliging them to use freq instead must be disorienting/frustrating.

Also, supposedly, pandas also shifts and rolls even if we do not have a datetime in the index. So in those cases, freq won't be useful.


if isinstance(self.window, list):
feature_names = [
str(feature) + f"_window_{win}" + f"_{agg}"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This could be simplified to:

f"{feature}_window_{win}_{agg}"

fit=_fit_not_learn_docstring,
n_features_in_=_n_features_in_docstring,
)
class BaseForecast(BaseEstimator, TransformerMixin):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Minor point: I think naming this class BaseForecast could be misleading. It's not a base forecast and doesn't produce forecasts. The docstring is clear though. A more explicit but verbose name could be: BaseForecastTransformer, BaseTimeseriesTransformer.

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.

time series forecasting: window features

3 participants