Skip to content

fix get_statistics_relevance_table #672

Merged
merged 8 commits into from May 17, 2022
Merged

fix get_statistics_relevance_table #672

merged 8 commits into from May 17, 2022

Conversation

Ama16
Copy link
Contributor

@Ama16 Ama16 commented May 7, 2022

IMPORTANT: Please do not create a Pull Request without creating an issue first.

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?

Type of Change

  • Examples / docs / tutorials / contributors update
  • Bug fix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves an existing feature)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Proposed Changes

Related Issue

Closing issues

#668

@github-actions
Copy link

github-actions bot commented May 7, 2022

🚀 Deployed on https://deploy-preview-672--etna-docs.netlify.app

@github-actions github-actions bot temporarily deployed to pull request May 7, 2022 09:43 Inactive
@codecov-commenter
Copy link

codecov-commenter commented May 7, 2022

Codecov Report

Merging #672 (5a4bd3a) into master (bed4e1d) will increase coverage by 0.28%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #672      +/-   ##
==========================================
+ Coverage   83.80%   84.09%   +0.28%     
==========================================
  Files         119      119              
  Lines        6399     6412      +13     
==========================================
+ Hits         5363     5392      +29     
+ Misses       1036     1020      -16     
Impacted Files Coverage Δ
etna/analysis/feature_relevance/relevance_table.py 100.00% <100.00%> (ø)
etna/libs/tsfresh/relevance.py 76.19% <0.00%> (+15.23%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@github-actions github-actions bot temporarily deployed to pull request May 7, 2022 09:47 Inactive
etna/analysis/feature_relevance/relevance_table.py Outdated Show resolved Hide resolved
etna/analysis/feature_relevance/relevance_table.py Outdated Show resolved Hide resolved
)
df_exog_now["target"] = df_now
df_exog_now = df_exog_now.dropna()
if len(df_exog_now) != len(df_now) and not none_warning:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add this warning also to the second method and add test case

@martins0n martins0n requested a review from Mr-Geekman May 12, 2022 07:45
)
df_exog_now["target"] = df_now
df_exog_now = df_exog_now.dropna()
if len(df_exog_now) != len(df_now) and not none_warning_raised:
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to drop the nan values before casting categoricals to float, exchange this two sections

"Exogenous data contains columns with category type! It will be converted to float. If this is not desired behavior, use encoders."
)
df_exog_now["target"] = df_now
df_exog_now = df_exog_now.dropna()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's drop nans the same way as here to make this step similar in both methods

df = pd.DataFrame({"segment": seg, "timestamp": timestamps, "target": target})
ts = TSDataset.to_dataset(df)

cast = ["1.1"] * 10 + ["2"] * 10 + ["56.1"] * 10
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add nans to this column + test for this case(nans in castable categorical column)


def test_target_none_model_table(exog_and_target_dfs):
df, df_exog = exog_and_target_dfs
tmp = np.arange(len(df["a", "target"]), dtype=float)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let'c create the separate fixture for this case, you can make it out of the exog_and_target_dfs

@github-actions github-actions bot temporarily deployed to pull request May 16, 2022 05:39 Inactive
Copy link
Collaborator

@alex-hse-repository alex-hse-repository left a comment

Choose a reason for hiding this comment

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

👍

@alex-hse-repository alex-hse-repository enabled auto-merge (squash) May 17, 2022 04:36
@github-actions github-actions bot temporarily deployed to pull request May 17, 2022 04:39 Inactive
@alex-hse-repository alex-hse-repository merged commit 4570f02 into master May 17, 2022
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.

None yet

3 participants