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

[ENH] fix get_window utility when window_length was None #2866

Merged
merged 5 commits into from Jun 27, 2022
Merged

Conversation

fkiraly
Copy link
Collaborator

@fkiraly fkiraly commented Jun 25, 2022

The get_window utility was broken when window_length was None, it would return the object obj unchanged.

This has now been fixed.

In addition, the lag default has been set to zero, which is interpreted as a zero integer or timedelta, depending on obj.

Test cases for None input behaviour have been added.

@fkiraly fkiraly added module:datatypes datatypes module: data containers, checkers & converters bugfix Fixes a known bug or removes unintended behavior labels Jun 25, 2022
@fkiraly fkiraly requested a review from aiwalter as a code owner June 25, 2022 22:57
Copy link
Contributor

@khrapovs khrapovs left a comment

Choose a reason for hiding this comment

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

Would you be so kind to update sktime.datatypes.tests.test_utils.test_get_window_expected_result to reflect the changes you made in this PR? Thanks!

@fkiraly
Copy link
Collaborator Author

fkiraly commented Jun 26, 2022

Good catch! No bugfix without tests that nail the correct behaviour down, optimally. Done.

@fkiraly fkiraly merged commit adad11d into main Jun 27, 2022
@fkiraly fkiraly deleted the get_window-fix branch June 27, 2022 08:57
fkiraly added a commit that referenced this pull request Jul 5, 2022
…ecasters (#2865)

This series of PR introduces auto-vectorization over columns for univariate estimators, i.e., the estimator is applied by column. Applies to forecasters and transformers.
The `forecasters_` and `transformers_` attribute, in this case, is a `pd.DataFrame` which has columns corresponding to the columns vectorized over.

This replaces previous behaviour where, instead, an error would be raised.

This PR is part 1 of a series of two PRs, one for forecasters and transformers, for easier review.
This PR contains vectorization for forecasters.
Part 2, transformers, is in #2867.

The PR relies on:
* PR #2864 which introduces column (variable) vectorization to `VectorizedDF`.
* PR #2866 as `get_window` had a bug and the new tests were trying to use it.
* PR #2870 since the cutoff was not properly updated in the vectorized case
fkiraly added a commit that referenced this pull request Jul 5, 2022
…nsformers (#2867)

This series of PR introduces auto-vectorization over columns for univariate estimators, i.e., the estimator is applied by column. Applies to forecasters and transformers.
The `forecasters_` and `transformers_` attribute, in this case, is a `pd.DataFrame` which has columns corresponding to the columns vectorized over.

This replaces previous behaviour where, instead, an error would be raised.

This PR is part 2 of a series of two PRs, one for forecasters and transformers, for easier review.
This PR contains vectorization for transformers.
Part 1, forecasters, is in #2865.

The PR relies on:
* Part 1, #2865
* PR #2864 which introduces column (variable) vectorization to `VectorizedDF`.
* PR #2866 as `get_window` had a bug and the new tests were trying to use it.
* PR #2870 since the cutoff was not properly updated in the vectorized case
fkiraly added a commit that referenced this pull request Jul 6, 2022
…nsformers (#2937)

This series of PR introduces auto-vectorization over columns for univariate estimators, i.e., the estimator is applied by column. Applies to forecasters and transformers.
The `forecasters_` and `transformers_` attribute, in this case, is a `pd.DataFrame` which has columns corresponding to the columns vectorized over.

This replaces previous behaviour where, instead, an error would be raised.

This PR is part 2 of a series of two PRs, one for forecasters and transformers, for easier review.
This PR contains vectorization for transformers.
Part 1, transformers, is in #2865.

The PR relies on:
* PR #2865
* PR #2864 which introduces column (variable) vectorization to `VectorizedDF`.
* PR #2866 as `get_window` had a bug and the new tests were trying to use it.
* PR #2870 since the cutoff was not properly updated in the vectorized case
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixes a known bug or removes unintended behavior module:datatypes datatypes module: data containers, checkers & converters
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants