[BUG] TimeGapSplit mutates the data

[rubenvdg]

TimeGapSplit mutates the pandas.DataFrame it operates on if the date column is not a datetime type. For example:

from pandas.api.types import (
    is_object_dtype,
    is_datetime64_any_dtype
)

date_range = pd.date_range(start='1/1/2018', end='1/30/2018')
dates = [date.strftime('%m-%d-%Y') for date in date_range]

df = (
    pd.DataFrame(
        data=np.random.randint(0, 30, size=(30, 4)),
        columns=list('ABCy')
    )
    .assign(
        date=dates
    )
)

assert is_object_dtype(df['date'])

cv = TimeGapSplit(
    df=df,
    date_col='date',
    train_duration=timedelta(days=3),
    valid_duration=timedelta(days=1),
)

assert is_datetime64_any_dtype(df['date'])

Is this desirable behavior?

Possible remedies are:

• Only accept the datetime type
• Accept str type, but make a copy and leave the pandas.DataFrame as is.

Happy to hear your thoughts @kayhoogland @stephanecollot @koaning

[stephanecollot]

Hi, Nice catch. Yep that is because I did the conversion : https://github.com/koaning/scikit-lego/blob/master/sklego/model_selection.py#L39 Actually we don't need the entire dataframe, we just need a serie of the date with the same index as the dataframe. Therefore we could replace parameters `df, date_col` with `date_serie` = `df['date]` and remove the conversion. About enforcing the type to be `datetime`, why not but if I'm not mistaken there are plenty of different datetime types, would be nice to give the flexibility, if it works with pd.DateOffset or timedelta. Cheers

…

On Thu, 19 Sep 2019 at 09:35, Ruben van de Geer ***@***.***> wrote: TimeGapSplit mutates the pandas.DataFrame it operates on if the date column is not a datetime type. For example: from pandas.api.types import ( is_object_dtype, is_datetime64_any_dtype ) df = ( pd.DataFrame( data=np.random.randint(0, 30, size=(30, 4)), columns=list('ABCy') ) .assign( date=dates ) ) assert is_object_dtype(df['date']) cv = TimeGapSplit( df=df, date_col='date', train_duration=timedelta(days=3), valid_duration=timedelta(days=1), ) assert is_datetime64_any_dtype(df['date']) Is this desirable behavior? Possible remedies are: - Only accept the datetime type - Accept str type, but make a copy and leave the pandas.DataFrame as is. Happy to hear your thoughts @kayhoogland <https://github.com/kayhoogland> @stephanecollot <https://github.com/stephanecollot> @koaning <https://github.com/koaning> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#191?email_source=notifications&email_token=AAJBFDDWKB3YTP6OFAE5YODQKMTTLA5CNFSM4IYHQR72YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HMKUV3A>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJBFDGX3MNBW5R3NRLUGT3QKMTTLANCNFSM4IYHQR7Q> .

[koaning]

copy feels like a safer option for now. it allows for flexibility in the future.

[rubenvdg]

I tried to fix this issue by adding

self.df = df.copy().reset_index(drop=True)

in the __init__. However, this results in tests not working.

This has to do with the fact that in the initalization a df is passed and then in the .split() method a different dataframe is passed, c.f., https://github.com/koaning/scikit-lego/blob/master/tests/test_model_selection/test_timegapsplit.py#L23.

Is there a particular reason why you would like to __init__ with a different pd.DataFrame than to which you like to apply the .split() to?

[stephanecollot]

Why do you do .reset_index(drop=True)?
The index is very important, that how it can work with 2 different df -> the X_train usually doesn't contain the date column, and df contains the date (and y for example)

If you just remove reset_index it should work

[stephanecollot]

And in the test train = df.head(25) this is actually testing the fact that you don't mess with the index ^^.

[rubenvdg]

To (try to) resolve #193.

[rubenvdg]

But maybe I don't understand the use case that TimeGapSplit tries to solve. The convention in sklearn is to __init__ with some settings (but without data) and then apply the splitter to data in .split(), c.f., https://scikit-learn.org/stable/modules/generated/sklearn.model_selection.TimeSeriesSplit.html.

[stephanecollot]

Yep it seem so.
You can read:
https://scikit-lego.readthedocs.io/en/latest/timegapsplit.html
https://scikit-lego.readthedocs.io/en/latest/api/sklego.html?highlight=TimeGapSplit#sklego.model_selection.TimeGapSplit

This TimeGapSplit needs data in the init, this is why it is scikit-lego.

A remark that can useful for your understand I think: The date column cannot be the index because you could have multiple row per date.

Solve 1 ticket per MR I would suggest.

[stephanecollot]

Ok I'm fixing this, with a big performance optimisation.

[stephanecollot]

This was fixed and merged in #227

[stephanecollot]

@koaning can you close?