extends functionality of DatetimeFeatures to extract features from the dataframe index by dodoarg · Pull Request #405 · feature-engine/feature_engine

dodoarg · 2022-03-31T22:29:29Z

Here's a draft to implement the functionality. I might have forgotten about some test cases and there might be better ways to refactor the logic overall

I tried to keep the logic as consistent as possible with the rest of the cases (e.g. find_or_check_dt_variables dumps "index" into ["index"] as it would do with any other string etc) although the fact that we need to access .index rather than a (set of) column(s) introduces some "boilerplate" code

I did think of having _check_contains_na check for nans in the index but I found out some check estimator tests would fail then, so I just added that logic under an if statement in the datetime transformer (more boilerplate, ugh)

Also, this is probably a trivial note, but as things stand there's no coverage for the case where an actual variable (not the index) is named "index". I suppose we might just leave that as it is as it sounds like a very unrealistic name for a column anyway

Let me know how we can improve it

…ests to fail This reverts commit 28b750d.

solegalli

Hi @dodoarg

Thank you so much for the quick turnaround! Very helpful and very kind of you :)

I was thinking in making fewer changes for this addition. I would only modify the transformer, and not the variables manipulation function. I was wondering if that would be possible?

Would you mind having a look at my comments and see if they make sense?

Thank you!

solegalli · 2022-04-01T07:08:21Z

        # check if datetime variables contains na
        if self.missing_values == "raise":
-            _check_contains_na(X, self.variables_)
+            if self.variables_ == ["index"]:


my idea was that the argument variables in the init, can take 3 values:

None: defaults to finding all dt variables (already implemented)

Variable list: checks if variables entered by user are or can be datetime (already implemented)

"index", taken as a string, which will extract datetime features from the index (new in this PR)

Then, here I would tailor the message to say, there were nan in the index, for example.

solegalli · 2022-04-01T07:09:57Z

-        # convert datetime variables
-        datetime_df = pd.concat(
-            [
+        if self.variables_ == ["index"]:


to extract features from the index, do we absolutely need to make it a column?

Can we not do df.index.month? or something like this? I saw it the other day on a stackoverflow thread.

But maybe it does not fit with how we structured the class?

solegalli · 2022-04-01T07:11:48Z


        # create new features
-        for var in self.variables_:
+        if self.variables_ == ["index"]:


If we extract features from the index, I am wondering whether we should just name the features as the datetime they represent? we are not creating features from multiple variables, so in principle, there would be no need to add the variable name at the front to distinguish them from anything else?

solegalli · 2022-04-01T07:12:29Z

            raise ValueError("No datetime variables found in this dataframe.")

    elif isinstance(variables, (str, int)):
+        if variables == "index":


I would not modify this function. I would handle the logic from the datetime class, because this is probably one of the only classes that will work with the index in this way.

solegalli · 2022-04-01T07:16:54Z

    with pytest.raises(ValueError):
-        DatetimeFeatures().transform(dates_nan)
+        transformer.transform(dates_nan)
+    transformer = DatetimeFeatures(variables="index")


I see, here is when the problem you raised would come up. If the user has a variable called "index", but we are now using this string to indicate the df index, then they would not be able to extract features from this variable.

I think this is very unlikely, but the user still could pass ["index"] and then, they could extract features from the column called index. And if they pass "index" as a string, then, the class would use the index of the df. And we could make this clear in the docstrings.

I think, as a first release, this could be enough, and then we hear user feedback. Unless you think otherwise?

Yea no I agree. If anything if someone comes up with a dataset where a column not only is named "index" but contains datetime info (??) it's probably their fault in terms of bad naming practices and even lack of common sense haha

…columns

dodoarg · 2022-04-01T19:11:18Z

I think I've tackled pretty much all issues you raised @solegalli ?

I removed the "index_" prefix for feature columns extracted from the index
The part where the DatetimeIndex is dumped into a pd.Series was necessary because the way you access to datetime functions on an index object looks like df.index.year i.e. it "bypasses" the call to the dt module. Since our logic calls a set of lambda functions (FEATURES_FUNCTIONS) that all look like x.dt.year I thought it would add less boilerplate code to put the index data into a Series so as to access those functions the same way rather than rewrite all them without the dt part just for this specific case

solegalli

Hi @dodoarg

Thank you so much for the quick fixes on the code. This is really helpful.

I've got a couple of minor cosmetic requests, and then a question about pd.to_datetime() that might be worth exploring.

Would you be able to have a look?

Thank you!

FYI: I answered my own question below.

solegalli · 2022-04-02T08:19:28Z

+                    not is_numeric(X.index) and _is_categorical_and_is_datetime(X.index)
+                )
+            ):
+                raise TypeError("Index is not datetime.")


Could we re-phrase the error as

"The dataframe index is not datetime."

solegalli · 2022-04-02T08:21:09Z

+                if X.index.isnull().any():
+                    raise ValueError(
+                        "Index contain NaN. Check and "
+                        "remove those before using this transformer."


Could we re-phrase this error message?

"The dataframe index contains missing data. Check and remove those before using this transformer or set missing_values to False."

solegalli · 2022-04-02T08:25:59Z

+                        "remove those before using this transformer."
+                    )
+
+            self.variables_: List[Union[str, int]] = ["index"]


If we are using the index to extract features, I would leave variables_ to None.

I think Feature-engine users would expect to find the variables that are considered for the transformation in this attribute. And in this case, no variable is. Because we are using the index. Also, if we set this to ["index"], it would clash with the unlikely situation in which the user has a variable called "index".

solegalli · 2022-04-02T08:27:32Z

        _check_input_matches_training_df(X, self.n_features_in_)

+        # special case index
+        if self.variables_ == ["index"]:


We could use either self.variables_ is None or self.variables =="index" without the underscore.

solegalli · 2022-04-02T08:29:34Z

+                if X.index.isnull().any():
+                    raise ValueError(
+                        "Index contain NaN. Check and "
+                        "remove those before using this transformer."


could we rephrase as in fit()?

Also wondering, we are repeating code in fit and transform, should we capture it in a private method in this class?

if self.missing_values == "raise": self._check_contains_na()

…eature_names out works as intended in that case

dodoarg · 2022-04-02T11:25:48Z

Should be better now. Notice that when setting self.variables_ as None in the index case it occurred to me that I had to adapt get_feature_names_out to the index case

solegalli · 2022-04-02T15:09:38Z

Thank you so much @dodoarg !!!! This will do!

Merging on Monday.

Have a nice weekend.

solegalli · 2022-04-03T17:44:40Z

+
+            for feat in self.features_to_extract_:
+                X[FEATURES_SUFFIXES[feat][1:]] = FEATURES_FUNCTIONS[feat](
+                    pd.Series(


I just noticed that with this logic, we would be converting the index to datetime for every feature.

Could we not convert it one, capture the series outside the loop, and then enter the loop using the already prepared series?

Sure. My bad.

solegalli · 2022-04-03T17:45:11Z

+                    )
+                )
+
+            return X


could we have just 1 return at the end of the method and not break the logic in 2?

If you'd rather have what follows under an else statement yea we can do that

solegalli · 2022-04-03T17:46:06Z

            _check_contains_na(X, self.variables_)

        # reorder variables to match train set
        X = X[self.feature_names_in_]


This line should also apply for the index features. Is the final of the transform checks, before starting any transform logic. It ensures the df passed to transform has the columns in the same order than the one passed in fit.

Right. It probably didn't matter as far as the test go since we're not ever changing the existing columns but the index only when variables="index"

solegalli · 2022-04-03T17:49:30Z

            Alternatively, only the names for the datetime features derived from
            input_features will be returned.
+            If the transformer was fitted on the dataframe index, you may only pass
+            `index` as input_features, or leave it as `None`.


Could we reprhase these 2 sentences as "If the features were derived from the dataframe index, pass 'index' to obtain the datetime features or None for all output features."

solegalli

Hi @dodoarg

I took a second look at the PR just now, and I am wondering if we could make the loop more efficient by taking the pandas series out of the loop?

What do you think?

dodoarg · 2022-04-04T08:52:22Z

Should have made quick work of that in my break

restructured get_feature_names_out for readability updated user guide

dodoarg · 2022-04-06T07:28:26Z

is this ready to merge @solegalli ?

dodoarg added 7 commits March 30, 2022 20:22

adds support for datetime index when checking for dt vars

7709ccf

adds nan check for index

28b750d

adds dt index functionality to DatetimeFeatures

5c499f3

small update on transformer docstring

d8b10a7

Revert "adds nan check for index" as it would cause check_estimator t…

5301f54

…ests to fail This reverts commit 28b750d.

deals with nans in datetime index

c01cb48

fixes mypy exception

2b39174

solegalli reviewed Apr 1, 2022

View reviewed changes

dodoarg added 3 commits April 1, 2022 19:33

reverts variable_manipulation to original state

3c4c971

moves all new logic into the datetime transformer; renames extracted …

e902ac5

…columns

fixes mypy complaint

07087b6

solegalli reviewed Apr 2, 2022

View reviewed changes

dodoarg added 3 commits April 2, 2022 12:39

rephrases error msgs

158d8d6

self.variables_ is set as None when extracting vars from index; get_f…

139c70d

…eature_names out works as intended in that case

refactors na check for index in private method

93197c3

solegalli reviewed Apr 3, 2022

View reviewed changes

various small fixes

4ac5287

solegalli added 2 commits April 4, 2022 12:35

minor changes to docstrings, restructures get_feature_names_out

cb531c9

quick start with documentation

2b601aa

solegalli changed the title ~~Issue397~~ extends functionality of DatetimeFeatures to extract features from the dataframe index Apr 4, 2022

solegalli linked an issue Apr 4, 2022 that may be closed by this pull request

extend DatetimeFeatures to extract features from index as well #397

Closed

added demos to user guide

d9c4213

Merge pull request #8 from feature-engine/dtfromindex

7184923

restructured get_feature_names_out for readability updated user guide

solegalli added 2 commits April 6, 2022 10:14

fix typos in rst file

4d82000

fixes typo in docstring

3e9264c

solegalli merged commit fe50aab into feature-engine:main Apr 6, 2022

Uh oh!

Conversation

dodoarg commented Mar 31, 2022

Uh oh!

solegalli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dodoarg commented Apr 1, 2022

Uh oh!

solegalli left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dodoarg commented Apr 2, 2022

Uh oh!

solegalli commented Apr 2, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

solegalli left a comment

Choose a reason for hiding this comment

Uh oh!

dodoarg commented Apr 4, 2022

Uh oh!

dodoarg commented Apr 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

solegalli left a comment •

edited

Loading