[BUG] fix `temporal_train_test_split` for hierarchical and panel data in case where `fh` is not passed #5330

fkiraly · 2023-09-29T17:13:42Z

This PR fixes #5107 by replacing the inner workings of temporal_train_test_split with a splitter, making the design both DRY and reliant on splitter boilerplate.

This is done as follows:

addition of a new splitter TemporalTrainTestSplitter, which encapsulates the temporal_train_test_split logic
internally, TemporalTrainTestSplitter is a re-implementation of the temporal_train_test_split logic as a splitter class, giving access to the BaseSplitter boilerplate.
removed osuleaf example, as that was time series classification and got split across instances in the sample rather than time points. This would be inconsistent with the general splitter interface and the docstring specification.

The issue with the panel data is solved by recurring to the existing broadcasting boilerplate of BaseSplitter.

Depends on #5332 for testing.

…_split-hierarchical

fkiraly · 2023-09-29T17:48:15Z

not sure why this fails - perhaps undocumented part of contract?

sktime/split/expandinggreedy.py

hazrulakmal · 2023-09-30T15:06:29Z

not sure why this fails - perhaps undocumented part of contract?

are you referring to the fh argument? I think the original author may not follow the convention of writing the docstring but he/she did specify the fh's expected input type.

fkiraly · 2023-09-30T16:08:41Z

are you referring to the fh argument?

no, to the failure that seems to arise from sth numpy

This reverts commit 0010ca7.

fkiraly · 2023-10-01T13:06:08Z

works now - re-requested review @hazrulakmal

yarnabrina

Asked two clarification questions/doubts.

Also, may be it's a silly question, I couldn't spot a change that looked to be specifically for hierarchical/panel data. Can you please point me to the line that handles that part? Does this branch resolves both #4968 and #5107?

sktime/split/temporal_train_test_split.py

yarnabrina · 2023-10-02T05:02:42Z

sktime/split/temporal_train_test_split.py

-                test_size=test_size,
-                train_size=train_size,
-            )
+        return y_train, y_test


This probably does not need an else block, since if block contains a return already.

I thought the else block made the condition easier to spot for a reader of the code, because it matches the indentation and can be tracked up to the if condition.

If we remove the else, the if condition will be harder to find.

This is my rationale (in general) to do this even if you have if-return, but I'm not too strongly opined about this.

For instance, I did the opposite with if fh is not None:, because the else was very long.

I made a change to the code flow, and added comments - better? I went with your suggestion, but added comments to the fh and the X branching to make it clear to the reader

It was a non-blocking comment really. Usually tools like pylint will complain against these (ref. R1705).

The change looks good to me. pylint or etc. may still complain that function return signature vary between different return statements (ref. R1710), but I don't think there's a way to solve that here at all.

fkiraly · 2023-10-02T06:03:04Z

Also, may be it's a silly question, I couldn't spot a change that looked to be specifically for hierarchical/panel data. Can you please point me to the line that handles that part?

Hm, we should also perhaps add tests for this?

The part that solves this imo is as I claimed in the PR preamble: "The issue with the panel data is solved by recurring to the existing broadcasting boilerplate of BaseSplitter."

In more detail, temporal_train_test_split now calls TemporalTrainTestSplitter's split_series, which is the boilerplate in BaseSplitter (not new, hence DRY, and takes care of the hierarchical case) that ends up calling _split. This includes a broadcasting loop for hierarchical/panel data, all in the boilerplate layer of BaseSplitter, hence solving the issue in #5107.

It does not resolve #4968, as this deals only with the case where fh is not passed, the code for the case where fh is passed remains unchanged.

Can you please point me to the line that handles that part?

To point you to the precise line, there are multiple of relevance, all in split.base._base_splitter.py:

split_series calls split, line 224; the surrounding conversions (lines 222, 227, 228) take care of every sktime series type being supported
in split, if y is panel or hierarchical, we enter _split_vectorized, in via line 125 and 127
the broadcasting happens in _split_vectorized, the key line is 170 which calls _split again for each slice, in our case that's where we return to TemporalTrainTestSplitter._split

* origin/main: (24 commits) [MNT] skpro as a soft dependency (sktime#5273) [DOC] Correct code block formatting for pre-commit install command (sktime#5377) [ENH] parallelization backend calls in utility module - part 2, backend parameter passing (sktime#5311) [MNT] [Dependabot](deps): Bump stefanzweifel/git-auto-commit-action from 4 to 5 (sktime#5373) [MNT] python 3.12 compatibility - replace `distutils` calls with equivalent functionality (sktime#5376) [DOC] `sktime` intro notebook (sktime#3793) [ENH] Skip unnecessary fit in `ForecastX` if inner `forecaster_y` ignores `X` (sktime#5353) [ENH] refactor parallelization backend calls in utility module (sktime#5268) [ENH] ARCH model (arch package) Issue sktime#2173 (sktime#5326) [ENH] add error message return to `deep_equals` assert in `test_reconstruct_identical` (sktime#4927) [DOC] Added all feature names to docstring for DateTimeFeatures class (sktime#5283) [BUG] fix `STLForecaster` tag `ignores-exogenous-X` to be correctly set for composites (sktime#5365) [BUG] Fix inconsistent date/time index in `plot_windows` sktime#4919 (sktime#5321) &benshamza, fkiraly [BUG] Fix `numba` errors when calling `tslearn` `lcss` (sktime#5368) [MNT] Dataset file restructure (sktime#5239) [BUG] fix `temporal_train_test_split` for hierarchical and panel data in case where `fh` is not passed (sktime#5330) [MNT] [Dependabot](deps): Bump styfle/cancel-workflow-action from 0.11.0 to 0.12.0 (sktime#5355) [ENH] minor fixes to `NaiveAligner` (sktime#5344) [DOC] Improve Readability of Notebook 2 - Classification, Regression & Clustering (sktime#5312) [DOC] Documented ax argument and the figure in plot_series (sktime#5325) ...

…recasting * origin/split-ci: (25 commits) added flags to codecov [MNT] skpro as a soft dependency (sktime#5273) [DOC] Correct code block formatting for pre-commit install command (sktime#5377) [ENH] parallelization backend calls in utility module - part 2, backend parameter passing (sktime#5311) [MNT] [Dependabot](deps): Bump stefanzweifel/git-auto-commit-action from 4 to 5 (sktime#5373) [MNT] python 3.12 compatibility - replace `distutils` calls with equivalent functionality (sktime#5376) [DOC] `sktime` intro notebook (sktime#3793) [ENH] Skip unnecessary fit in `ForecastX` if inner `forecaster_y` ignores `X` (sktime#5353) [ENH] refactor parallelization backend calls in utility module (sktime#5268) [ENH] ARCH model (arch package) Issue sktime#2173 (sktime#5326) [ENH] add error message return to `deep_equals` assert in `test_reconstruct_identical` (sktime#4927) [DOC] Added all feature names to docstring for DateTimeFeatures class (sktime#5283) [BUG] fix `STLForecaster` tag `ignores-exogenous-X` to be correctly set for composites (sktime#5365) [BUG] Fix inconsistent date/time index in `plot_windows` sktime#4919 (sktime#5321) &benshamza, fkiraly [BUG] Fix `numba` errors when calling `tslearn` `lcss` (sktime#5368) [MNT] Dataset file restructure (sktime#5239) [BUG] fix `temporal_train_test_split` for hierarchical and panel data in case where `fh` is not passed (sktime#5330) [MNT] [Dependabot](deps): Bump styfle/cancel-workflow-action from 0.11.0 to 0.12.0 (sktime#5355) [ENH] minor fixes to `NaiveAligner` (sktime#5344) [DOC] Improve Readability of Notebook 2 - Classification, Regression & Clustering (sktime#5312) ...

* origin/split-ci: (25 commits) added flags to codecov [MNT] skpro as a soft dependency (sktime#5273) [DOC] Correct code block formatting for pre-commit install command (sktime#5377) [ENH] parallelization backend calls in utility module - part 2, backend parameter passing (sktime#5311) [MNT] [Dependabot](deps): Bump stefanzweifel/git-auto-commit-action from 4 to 5 (sktime#5373) [MNT] python 3.12 compatibility - replace `distutils` calls with equivalent functionality (sktime#5376) [DOC] `sktime` intro notebook (sktime#3793) [ENH] Skip unnecessary fit in `ForecastX` if inner `forecaster_y` ignores `X` (sktime#5353) [ENH] refactor parallelization backend calls in utility module (sktime#5268) [ENH] ARCH model (arch package) Issue sktime#2173 (sktime#5326) [ENH] add error message return to `deep_equals` assert in `test_reconstruct_identical` (sktime#4927) [DOC] Added all feature names to docstring for DateTimeFeatures class (sktime#5283) [BUG] fix `STLForecaster` tag `ignores-exogenous-X` to be correctly set for composites (sktime#5365) [BUG] Fix inconsistent date/time index in `plot_windows` sktime#4919 (sktime#5321) &benshamza, fkiraly [BUG] Fix `numba` errors when calling `tslearn` `lcss` (sktime#5368) [MNT] Dataset file restructure (sktime#5239) [BUG] fix `temporal_train_test_split` for hierarchical and panel data in case where `fh` is not passed (sktime#5330) [MNT] [Dependabot](deps): Bump styfle/cancel-workflow-action from 0.11.0 to 0.12.0 (sktime#5355) [ENH] minor fixes to `NaiveAligner` (sktime#5344) [DOC] Improve Readability of Notebook 2 - Classification, Regression & Clustering (sktime#5312) ...

fkiraly added 4 commits September 29, 2023 18:45

Update expandinggreedy.py

097f5a6

Update expandinggreedy.py

75f8c77

reversed greedy

b5c7912

temporal fix

69a93f5

fkiraly added enhancement Adding new functionality module:splitters&resamplers data splitters and resamplers labels Sep 29, 2023

fkiraly requested review from achieveordie, benHeid and yarnabrina as code owners September 29, 2023 17:13

fkiraly added 13 commits September 29, 2023 19:13

Update expandinggreedy.py

234a361

own class

5f47f51

linting

504df4e

treat hierarchical splitter

2c56c0f

Update temporal_train_test_split.py

02a642a

Update temporal_train_test_split.py

0425e15

typo

153171a

Merge branch 'expandinggreedysplitter-float' into temporal_train_test…

d7285a3

…_split-hierarchical

Update expandinggreedy.py

c9fe290

Update temporal_train_test_split.py

4ec9a10

Update temporal_train_test_split.py

0357421

Update expandinggreedy.py

e536bff

Merge branch 'expandinggreedysplitter-float' into temporal_train_test…

c1ec099

…_split-hierarchical

hazrulakmal reviewed Sep 30, 2023

View reviewed changes

sktime/split/expandinggreedy.py Outdated Show resolved Hide resolved

fkiraly added 4 commits October 1, 2023 10:10

Merge branch 'main' into temporal_train_test_split-hierarchical

24c2b89

fixes

fa4d02a

docstrig

fb591ee

Update test_temporaltraintest.py

58c4f60

fkiraly added 5 commits October 1, 2023 12:23

docstring

4ad2580

Update temporal_train_test_split.py

f4d32a5

Revert "Revert "export""

5334209

This reverts commit 0010ca7.

Update __init__.py

6de3de0

Update temporal_train_test_split.py

bbee8a8

fkiraly requested a review from hazrulakmal October 1, 2023 12:49

yarnabrina reviewed Oct 2, 2023

View reviewed changes

fkiraly mentioned this pull request Oct 2, 2023

[ENH] add tests for temporal_train_test_split #5332

Merged

fkiraly added 4 commits October 2, 2023 08:08

docstring

18cebc5

Update temporal_train_test_split.py

ce2592c

test for hierarchical

612e408

Update test_temporaltraintest.py

2418aac

fkiraly requested a review from yarnabrina October 2, 2023 06:27

fkiraly added 2 commits October 2, 2023 08:29

Update temporal_train_test_split.py

f2b9571

Update temporal_train_test_split.py

a7a2250

fkiraly changed the title ~~[BUG] fix temporal_train_test_split for hierarchical and panel pandas data~~ [BUG] fix temporal_train_test_split for hierarchical and panel data Oct 2, 2023

fkiraly changed the title ~~[BUG] fix temporal_train_test_split for hierarchical and panel data~~ [BUG] fix temporal_train_test_split for hierarchical and panel data in case where fh is not passed Oct 2, 2023

Update temporal_train_test_split.py

4a8da32

yarnabrina previously approved these changes Oct 2, 2023

View reviewed changes

yarnabrina mentioned this pull request Oct 2, 2023

[ENH] in ExpandingGreedySplitter, allow float step_size #5329

Merged

Merge branch 'main' into temporal_train_test_split-hierarchical

3a97912

fkiraly dismissed yarnabrina’s stale review via 3a97912 October 2, 2023 12:19

remove incorrect osuleaf example

efa1dec

fkiraly merged commit bbfbeed into main Oct 7, 2023
24 checks passed

fkiraly deleted the temporal_train_test_split-hierarchical branch October 7, 2023 09:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUG] fix `temporal_train_test_split` for hierarchical and panel data in case where `fh` is not passed #5330

[BUG] fix `temporal_train_test_split` for hierarchical and panel data in case where `fh` is not passed #5330

fkiraly commented Sep 29, 2023 •

edited

fkiraly commented Sep 29, 2023

hazrulakmal commented Sep 30, 2023

fkiraly commented Sep 30, 2023

fkiraly commented Oct 1, 2023

yarnabrina left a comment •

edited

yarnabrina Oct 2, 2023

fkiraly Oct 2, 2023

fkiraly Oct 2, 2023

fkiraly Oct 2, 2023

yarnabrina Oct 2, 2023

fkiraly commented Oct 2, 2023 •

edited

[BUG] fix temporal_train_test_split for hierarchical and panel data in case where fh is not passed #5330

[BUG] fix temporal_train_test_split for hierarchical and panel data in case where fh is not passed #5330

Conversation

fkiraly commented Sep 29, 2023 • edited

fkiraly commented Sep 29, 2023

hazrulakmal commented Sep 30, 2023

fkiraly commented Sep 30, 2023

fkiraly commented Oct 1, 2023

yarnabrina left a comment • edited

Choose a reason for hiding this comment

yarnabrina Oct 2, 2023

Choose a reason for hiding this comment

fkiraly Oct 2, 2023

Choose a reason for hiding this comment

fkiraly Oct 2, 2023

Choose a reason for hiding this comment

fkiraly Oct 2, 2023

Choose a reason for hiding this comment

yarnabrina Oct 2, 2023

Choose a reason for hiding this comment

fkiraly commented Oct 2, 2023 • edited

[BUG] fix `temporal_train_test_split` for hierarchical and panel data in case where `fh` is not passed #5330

[BUG] fix `temporal_train_test_split` for hierarchical and panel data in case where `fh` is not passed #5330

fkiraly commented Sep 29, 2023 •

edited

yarnabrina left a comment •

edited

fkiraly commented Oct 2, 2023 •

edited