[ENH] Add more train_test_split strategies - #2087
Conversation
…ecasting into train_test_split
…/pytorch-forecasting into train_test_split
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2087 +/- ##
=======================================
Coverage ? 84.28%
=======================================
Files ? 177
Lines ? 10587
Branches ? 0
=======================================
Hits ? 8923
Misses ? 1664
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| self._train_size : self._train_size + self._val_size | ||
| ] | ||
| # Ensure categorical encoders are fitted | ||
| if getattr(self, "_categorical_encoders", "auto") == "auto": |
There was a problem hiding this comment.
Can you please specify where are you defining _categorical_encoders?
There was a problem hiding this comment.
I think this should be handled sperately? or maybe you can stack both PRs?
There was a problem hiding this comment.
Hi @phoeenniixx! You're absolutely right _categorical_encoders was never defined in it. But it is in this my PR #2082 . So, could you review that and we'd stack over after merging that I'll continue from here and make this categorical_encoder's train_test_split feature more stable. I just added that as a part of reference for future implementation.
There was a problem hiding this comment.
I think categorical_encoders are out of scope for this PR? If you want to use them, I think you should stack on the the current PR using git merge?
phoeenniixx
left a comment
There was a problem hiding this comment.
Thanks!
There are some unrelated changes to layers (like changes to the documentation) please remove that
train_test_split strategiestrain_test_split strategies
Thanks for guiding @phoeenniixx, Kindly review the PR. I've removed the un-related changes. |
|
Hi @phoeenniixx , @PranavBhatP , @fkiraly Kindly review this PR |
I've analyzed This approach allows us to support anchor ("start"/"end") semantics and 3-way splits (train/val/test) using global timestamp cutoffs. The main benefit is that it simplifies the Does this direction look good to you? If so, I can start on a backward-compatible PR. |
|
What if we have 2 groups but with different lifespan? If we use temporal cutoff we may end with one series in train and the other in validation, but maybe we don't want this. We should also think to have a percentage approach that computes the cutoff PER GROUP and then use the temporal cutoff, what do you think? |
Hi @agobbifbk Could you please explain to me this what do you mean by 2 groups. If there are 2 groups then they should be separated first into group 1 and group 2 and then the temporal split applies. So, what is the situation you're talking about that we end up? |
|
If group 1 goes from 2000 to 2010 and group 2 goes from 2005 to 2015 and you put a global cutoff at 2005, for the train/validation split you will end up with half of group 1 on train and half in test, the group 2 entirely in validation. But what if I want half of both in train and half in validation? I'm saying that global cutoff can be used if I want to end up in situation 1, but we need also to use the percentage and compute local cutoff if I want to end up with situation 2. |
…ecasting into train_test_split
…/pytorch-forecasting into train_test_split
|
Hi @phoeenniixx, @agobbifbk, I've made the following changes, kindly have a look:
If data is provided from 2020 to 2027 andthe user applied the temporal cutoff by themselves Train: [2020, 2022]
|
|
Nice one @Muhammad-Rebaal! Just one comment. Since we can pass both the percentage and the global cutoff, maybe it worth to print a warning that says, when both are not None, that global cutoff is checked first. Did you check if the temporal split works also for datetime, date, and other non-numerical timestamps? From a first review it seems that the percentage split is applied per series in case of temporal split instead of per group isn't it? for s_idx, timestamps in series_timestamps.items():
t_min = float(np.min(timestamps))
t_max = float(np.max(timestamps))
t_range = t_max - t_min
if t_range == 0:
series_cutoffs[s_idx] = None
else:
train_cutoff = t_min + train_val_test_split[0] * t_range
val_cutoff = train_cutoff + train_val_test_split[1] * t_range
series_cutoffs[s_idx] = (train_cutoff, val_cutoff)
all_zero = all(v is None for v in series_cutoffs.values())If you do it per series, it may be that one group has some future data in validation no? Or I'm reading it wrongly? |
…passed both temporal cutoff value and split
Yes I've added the warning for that. and you're right for the rest I've added a fix to tackle that. |
| all_ts = np.concatenate(list(series_timestamps.values())) | ||
| global_timeline = np.unique(all_ts) | ||
|
|
||
| if len(global_timeline) <= 1: | ||
| total_w = len(windows) | ||
| train_end = int(np.round(train_val_test_split[0] * total_w)) | ||
| if train_end == 0 and train_val_test_split[0] > 0 and total_w > 0: | ||
| train_end = 1 | ||
| val_end = train_end + int(np.round(train_val_test_split[1] * total_w)) | ||
| val_end = min(val_end, total_w) | ||
| return windows[:train_end], windows[train_end:val_end], windows[val_end:] | ||
|
|
||
| n = len(global_timeline) | ||
| train_pos = min(int(np.round(train_val_test_split[0] * n)), n - 1) | ||
| val_pos = min( | ||
| int(np.round((train_val_test_split[0] + train_val_test_split[1]) * n)), | ||
| n - 1, | ||
| ) | ||
|
|
||
| train_cutoff = global_timeline[train_pos] | ||
| val_cutoff = global_timeline[val_pos] | ||
|
|
||
| train_windows, val_windows, test_windows = [], [], [] | ||
| for w in windows: | ||
| end_time = _get_window_end_time(w, series_timestamps) | ||
| if end_time <= train_cutoff: | ||
| train_windows.append(w) | ||
| elif end_time <= val_cutoff: | ||
| val_windows.append(w) | ||
| else: | ||
| test_windows.append(w) | ||
| return train_windows, val_windows, test_windows |
There was a problem hiding this comment.
Going with default split (70,15,15)
We've 2 series :
S1: [1,2,3,4,5,6] , S2 : [4,6,7,8]
then combine that via np.concatenate() -> [1,2,3,4,5,6,7,8] -> length of 8
train_pos = global_timeline[6] -> [6]
val_pos = global_timeline[7] -> [7]
train_cutoff = 7
val_cutoff = 8
Which later by looping we get
end_time = 8
train_windows = [1-7)
val_windows = [8]
test_windows = [empty in this case]
There was a problem hiding this comment.
Almost: this concatenation is performed by group or regardless the group? Your logic works if S1 and S2 are from the same group even if this is not possible (it can not be that the same group has the same timestamp in two series).
if S1 is a series of group 1 and S2 of group 2 you should not concatenate the TS you should perform the split per series (as it was at the beginning).
I give another example:
percentage: (80,10,10) (for convenience)
G1: S1: [1,2,3,4,5,6] , S2 : [7,8,9,10] --> 1-8 in train, 9 validation 10 test
G2: S3: [11,12,13,14,15,16] , S4 : [17,18,19,20] --> 11-18 in train, 19 validation 20 test
So the concatenation must be per group, hard for me to read if from the code, if it is already like this good :-)
Moreover, if the user goes for a temporal split we should suggest to use the group as categorical feature (known).
Since you are at this point, can we implement also the group-time split? In this case first you divide the groups into train, validation and test, then the groups in train are divided using the temporal logic into train validation and test! In this case the group CAN not be used as categorical variable otherwise the model can hardly generalize on new groups!
Hope this clarifies!
There was a problem hiding this comment.
Actually its not that way implemented but I completely understand what you mean so I'd adjust that accordingly then we'd move towards the group-time split
There was a problem hiding this comment.
Just one question :
In this case first you divide the groups into train, validation and test
It would be train_val_test_split: tuple = (0.8,0.1,0.1) or train_val_test_split: tuple = (0.7, 0.15, 0.15)
There was a problem hiding this comment.
Maybe we should call train_val_test_split differently if is referred to the temporal split or group split, so we need one more parameter here
There was a problem hiding this comment.
Yeah I agree, I've added a param for that called group-split to handle it.
phoeenniixx
left a comment
There was a problem hiding this comment.
Would it make sense to move the splitting logic to split folder and then you access it from there, in that way you dont have to implement the same thing twice
Yeah, I think that's a good idea to do so but I think first the implementation should be done then we'd refactor. |
But rn (when the thing is being actually designed) - it is easier to refactor no? Once it is released, we would need deprecastions etc to make this same refactor. Also, this will overcomplicate the datamodule class once we want to add more splitting algos (which will happen soon ig) |
|
Also, the code-quality is failing :) |
Yes you're right I didn't mean the release thing ? I mean if there are split strategies or this is the last one for this let's say there 3 functions of splitting and there is another that needs to be done so I wrote the 4th one and then refactor in the same PR. What is your suggestion? |
|
Sorry, I dont understand what you mean? Did you plan to first reach to a consensus and then refactor in the same PR? That is also a good idea... Everything depends on you as you are the main person on this feat rn |
No worries! Yes, I meant reaching a consensus first and then refactoring it all in the same PR. Thanks for understanding. |
|
Hi @agobbifbk, these are the following changes:
Is there any other split strategies that needs to be implemented. Kindly let me know. |
Closes #2064 which references #1974
Hi @PranavBhatP , @phoeenniixx !
I've resolved the issue to expand the train_test_split capabilities within the ptf-v2 DataModules. Could you please review the PR?
Here is a summary of the changes made:
pytorch_forecasting/data/splitters.pywhich implements three new splitting functions:random_series_split(acts as group-based split),stratified_series_split, andtemporal_window_split.split_strategyparameter ("random", "stratified", "temporal", "group") and delegated splitting logic to bothEncoderDecoderTimeSeriesDataModule(pytorch_forecasting/data/data_module.py) andTslibDataModule(pytorch_forecasting/data/_tslib_data_module.py).split_strategy="random".Thank You !