Skip to content

Fix WindowStatisticsTransform #1128

Merged
merged 6 commits into from
Feb 28, 2023
Merged

Fix WindowStatisticsTransform #1128

merged 6 commits into from
Feb 28, 2023

Conversation

alex-hse-repository
Copy link
Collaborator

Before submitting (must do checklist)

  • Did you read the contribution guide?
  • Did you update the docs? We use Numpy format for all the methods and classes.
  • Did you write any new necessary tests?
  • Did you update the CHANGELOG?

Proposed Changes

Closing issues

closes #1008

@alex-hse-repository alex-hse-repository self-assigned this Feb 22, 2023
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@alex-hse-repository alex-hse-repository changed the base branch from master to etna_v2 February 22, 2023 17:19
@github-actions
Copy link

github-actions bot commented Feb 22, 2023

@github-actions github-actions bot temporarily deployed to pull request February 22, 2023 17:55 Inactive
@github-actions github-actions bot temporarily deployed to pull request February 27, 2023 07:22 Inactive
@codecov-commenter
Copy link

codecov-commenter commented Feb 27, 2023

Codecov Report

Merging #1128 (036c7a8) into etna_v2 (17ca030) will increase coverage by 35.27%.
The diff coverage is 95.45%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##           etna_v2    #1128       +/-   ##
============================================
+ Coverage    51.37%   86.64%   +35.27%     
============================================
  Files          166      166               
  Lines         9351     9360        +9     
============================================
+ Hits          4804     8110     +3306     
+ Misses        4547     1250     -3297     
Impacted Files Coverage Δ
etna/transforms/math/differencing.py 97.63% <75.00%> (+63.51%) ⬆️
etna/transforms/encoders/categorical.py 100.00% <100.00%> (+54.34%) ⬆️
etna/transforms/math/add_constant.py 98.36% <100.00%> (+59.01%) ⬆️
etna/transforms/math/apply_lambda.py 95.16% <100.00%> (+51.61%) ⬆️
etna/transforms/math/log.py 96.87% <100.00%> (+57.81%) ⬆️
etna/transforms/math/sklearn.py 94.48% <100.00%> (+23.39%) ⬆️
etna/transforms/math/statistics.py 99.43% <100.00%> (+34.14%) ⬆️
etna/transforms/missing_values/resample.py 95.71% <100.00%> (+70.00%) ⬆️
etna/models/tbats.py 85.00% <0.00%> (-6.67%) ⬇️
etna/models/nn/utils.py 81.15% <0.00%> (-2.06%) ⬇️
... and 107 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Mr-Geekman
Copy link
Contributor

Why is branch named issue-1080? Is it somehow connected to this issue?

transform.fit(ts_with_exog_galeshapley.to_pandas())
df = transform.transform(ts_with_exog_galeshapley.to_pandas())
transform.fit(ts_with_exog_galeshapley)
ts = transform.transform(ts_with_exog_galeshapley)
Copy link
Contributor

Choose a reason for hiding this comment

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

How did this test pass before?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is the artefact of rebase, is you see the PR with etna_v2 it fails there

Copy link
Contributor

@Mr-Geekman Mr-Geekman left a comment

Choose a reason for hiding this comment

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

Look at comments above.

@alex-hse-repository
Copy link
Collaborator Author

Why is branch named issue-1080? Is it somehow connected to this issue?

No, it is typo

@github-actions github-actions bot temporarily deployed to pull request February 27, 2023 09:33 Inactive
@github-actions github-actions bot temporarily deployed to pull request February 27, 2023 12:45 Inactive
@@ -81,10 +80,10 @@ def _get_column_name(self) -> str:

def get_regressors_info(self) -> List[str]:
"""Return the list with regressors created by the transform."""
if self.in_column_regressor is None:
raise ValueError("Fit the transform to get the correct regressors info!")
if self.inplace:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need to check inplace after self.in_column_regressor is None?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes we do because in case of inplace=True we return the empty list

@@ -359,10 +358,10 @@ def _get_column_name(self) -> str:

def get_regressors_info(self) -> List[str]:
"""Return the list with regressors created by the transform."""
if self.in_column_regressor is None:
raise ValueError("Fit the transform to get the correct regressors info!")
if self.inplace:
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

@@ -164,10 +164,10 @@ def _get_out_column(self, out_column: Optional[str]) -> str:

def get_regressors_info(self) -> List[str]:
"""Return the list with regressors created by the transform."""
if self.in_column_regressor is None:
raise ValueError("Fit the transform to get the correct regressors info!")
Copy link
Contributor

Choose a reason for hiding this comment

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

Same.

@alex-hse-repository alex-hse-repository merged commit 9e79e79 into etna_v2 Feb 28, 2023
alex-hse-repository added a commit that referenced this pull request Mar 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] WindowStatisticsTransform incorrectly process regressors
3 participants