Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: allow for mode="backward" #44

Merged
merged 2 commits into from
May 14, 2024
Merged

feat: allow for mode="backward" #44

merged 2 commits into from
May 14, 2024

Conversation

FBruzzesi
Copy link
Owner

Description

Implements the possibility to specify mode="backward" so that splits are generate from the end to the beginning.

Notice that this (implementation) implies that:

  • test size will always correspond to forecast_horizon for backward mode, while it is not necessarily the case in forward mode
  • train size will always be of size at least train_size in both mode's and both window's type

Fix #41

TODO

Add specific tests for checking forward vs backward swaps

@FBruzzesi
Copy link
Owner Author

FBruzzesi commented May 12, 2024

cc: @mdancho84

Please take a look when you have the time. This should satisfy what we discussed in the issue

The logic is not too ugly, hence I would be comfortable to maintain.

@@ -115,3 +120,21 @@ def total_length(self: Self) -> timedelta:
A `timedelta` object representing the time between `train_start` and `forecast_end`.
"""
return self.forecast_end - self.train_start

def __add__(self: Self, other: Union[timedelta, pd.Timedelta]) -> SplitState:
Copy link
Owner Author

Choose a reason for hiding this comment

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

Ended up not using this as for the window="rolling" case it would break and be handled anyway, yet I don't mind to keep it

Comment on lines +224 to +227
train_delta = -self.train_delta
forecast_delta = -self.forecast_delta
gap_delta = -self.gap_delta
stride_delta = -self.stride_delta
Copy link
Owner Author

Choose a reason for hiding this comment

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

Flip the sign to maintain the same update logic below

@FBruzzesi
Copy link
Owner Author

FBruzzesi commented May 12, 2024

@baggiponte would you also be so kind to take a look and drop your opinion? 😁

@baggiponte
Copy link

At a wedding, will review next week. With pleasure, and thanks for asking 😊

@baggiponte
Copy link

Looks clean to me! 🔥

@mdancho84
Copy link

Agree. Let me know once it's merged and I'll begin integrating into pytimetk.

@FBruzzesi
Copy link
Owner Author

FBruzzesi commented May 14, 2024

Thanks everyone! I am glad for the positive feedback ✨

@mdancho84 I will merge this one (so you can develop using upstream/main branch), however I would like to:

  • Move scikit-learn compatible Splitter to its own module in order to let scikit-learn optional (Make scikit-learn optional #45)
  • Document all the latest changes (There are some breaking, such as, almost all arguments are now keyword only).

@FBruzzesi FBruzzesi merged commit 618b91a into main May 14, 2024
8 checks passed
@FBruzzesi FBruzzesi deleted the feat/backward-mode branch May 14, 2024 14:10
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.

CV Splits - Is it Possible to Start With Most Recent Data?
3 participants