Improve sample_acf and sample_pacf plots #1004

DBcreator · 2022-11-16T16:46:59Z

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 #682

…e-682

review-notebook-app · 2022-11-16T16:47:03Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

github-actions · 2022-11-16T16:58:05Z

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

codecov-commenter · 2022-11-16T17:12:46Z

Codecov Report

Merging #1004 (b1ad28b) into master (554d4ea) will increase coverage by 0.33%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1004      +/-   ##
==========================================
+ Coverage   85.74%   86.08%   +0.33%     
==========================================
  Files         162      162              
  Lines        8616     8608       -8     
==========================================
+ Hits         7388     7410      +22     
+ Misses       1228     1198      -30

Impacted Files	Coverage Δ
etna/analysis/__init__.py	`100.00% <100.00%> (ø)`
etna/analysis/eda_utils.py	`61.80% <100.00%> (+8.73%)`	⬆️
etna/models/nn/deepar.py	`98.93% <0.00%> (-0.02%)`	⬇️
etna/models/nn/tft.py	`99.10% <0.00%> (-0.01%)`	⬇️
etna/models/utils.py	`100.00% <0.00%> (ø)`
etna/models/nn/mlp.py	`100.00% <0.00%> (ø)`
etna/models/nn/rnn.py	`100.00% <0.00%> (ø)`

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

Mr-Geekman · 2022-11-17T10:35:57Z

etna/analysis/eda_utils.py

        segments = sorted(ts.segments)

+    plot = plot_pacf if partial else plot_acf
+    title = "Partial Autocorrelation" if partial else "Autocorrelation"
+
    k = min(n_segments, len(segments))


It seems like it is not really correct. As I understand, we should ignore n_segments parameter if segments is set. Look at distribution_plot.

Mr-Geekman · 2022-11-17T10:36:28Z

etna/analysis/eda_utils.py

-        plot_acf(x=df_slice["target"].values, ax=ax[i], lags=lags)
+
+        if df_slice["target"].isna().any():
+            print("yes")


It looks like debug string, remove it.

Mr-Geekman · 2022-11-17T10:37:06Z

etna/analysis/eda_utils.py

+    """
+    acf_plot(ts=ts, n_segments=n_segments, lags=lags, segments=segments, figsize=figsize, partial=False)
+    warnings.warn(
+        "DeprecationWarning: This function is deprecated and will be removed in etna=1.14.0; Please use acf_plot instead",


I amn't really sure about this version of deprecation.

etna/analysis/eda_utils.py

Mr-Geekman · 2022-11-17T10:46:07Z

etna/analysis/eda_utils.py

+        if df_slice["target"].isna().any():
+            print("yes")
+            df_slice["target"].dropna(inplace=True)
+            warnings.warn("Values with NaN dropped!")


I don't think we should raise this warning. Let's clarify this moment in docstring of the function. Smth like:

This function removes any NaNs before plotting.

Mr-Geekman · 2022-11-17T10:49:04Z

I don't see any images about the results of new functions. You should add script for plotting + result images. You should check both partial=True/False and data with/without nans. You can look at scripts in #691, #706, https://github.com/tinkoff-ai/etna/milestone/9?closed=1 as a reference.

Mr-Geekman

Look at comments above.

DBcreator · 2022-11-17T15:00:04Z

from etna.analysis import acf_plot
import pandas as pd
import matplotlib.pyplot as plt
from etna.datasets import TSDataset
import warnings 
warnings.filterwarnings("ignore")


data = pd.read_csv("examples/data/nordic_merch_sales.csv")
df = TSDataset.to_dataset(data)
ts = TSDataset(df, freq="D")

acf_plot(ts, n_segments=9, columns_num=3, partial=False)
plt.savefig("Autocorrelation")
acf_plot(ts, n_segments=9, columns_num=3, partial=True)
plt.savefig("Partial Autocorrelation")

Autocorrelation:

Partial Autocorrelation:

Mr-Geekman · 2022-11-18T09:31:59Z

etna/analysis/eda_utils.py

    if segments is None:
-        segments = sorted(ts.segments)
+        exist_segments = df_pd.segment.unique()


Why can't we write this if like:

if segments is None: segments = sorted(ts.segments) selected_segments = np.random.choice(...) segments = list(selected_segments)

In this case we don't need df_pd variable.

Mr-Geekman · 2022-11-18T09:38:16Z

etna/analysis/eda_utils.py

+    """
+    acf_plot(ts=ts, n_segments=n_segments, lags=lags, segments=segments, figsize=figsize, partial=False)
+    warnings.warn(
+        "DeprecationWarning: This function is deprecated and will be removed soon; Please use acf_plot instead",


Negotiating about version to be removed.

DBcreator · 2022-11-21T13:57:15Z

Example with NaN:

from etna.analysis import acf_plot
import pandas as pd
import numpy as np
import matplotlib.pyplot as plt
from etna.datasets import TSDataset, generate_const_df, generate_ar_df
import warnings 

df_1 = generate_const_df(periods=100, start_time="2020-01-01", scale=1)
df_1["target"] = [np.nan]*len(df_1["target"])
df_2 = generate_ar_df(periods=100, start_time="2020-04-10")
df = pd.concat([df_1, df_2])

df = TSDataset.to_dataset(df)
ts = TSDataset(df, freq="D")

acf_plot(ts)
plt.savefig("Autocorrelation with NaN.png")

acf_plot(ts, partial=True)
plt.savefig("Partial Autocorrelation with NaN.png")

Autocorrelation:

Partial Autocorrelation:

Mr-Geekman · 2022-11-21T14:10:02Z

CHANGELOG.md

@@ -10,7 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Added
 -
 - 
- 
+- Improve `sample_acf_plot` and `sample_pacf_plot` ([#1004](https://github.com/tinkoff-ai/etna/pull/1004))


I think that it should be moved to Changed block and you should write smth like: "Add acf_plot, deprecated sample_acf_plot, sample_pacf_plot".

Mr-Geekman · 2022-11-21T14:15:39Z

etna/analysis/eda_utils.py

        df_slice = ts[:, name, :][name]
-        plot_acf(x=df_slice["target"].values, ax=ax[i], lags=lags)
+
+        if df_slice["target"].isna().any():


Do we need this if?

Mr-Geekman · 2022-11-22T14:02:55Z

etna/analysis/eda_utils.py

+        df_slice = ts.to_pandas()[name]["target"]
+        if partial:
+            # for partial autocorrelation remove NaN from the beginning and end of the series
+            indices_nan = np.argwhere(np.isnan(df_slice.values)).squeeze(1)


Try to use first_valid_index

Mr-Geekman · 2022-11-22T14:03:09Z

etna/analysis/eda_utils.py

+
+            indices2remove_end = []
+
+            current_end = len(df_slice.values) - 1


Try to use last_valid_index

Mr-Geekman · 2022-11-22T14:04:25Z

etna/analysis/eda_utils.py

+
+            candidates2delete = indices2remove_begin + indices2remove_end
+            if set(indices_nan) != set(candidates2delete):
+                raise ValueError("There is a NaN in the middle of the time series!")


Errors should be moved to Raises block in a docstring. Look at docstring of cross_corr_plot for example.

Mr-Geekman · 2022-11-24T09:22:33Z

etna/analysis/eda_utils.py

+    fig.suptitle(title, fontsize=16)
+
+    for i, name in enumerate(segments):
+        df_slice = ts.to_pandas()[name].reset_index()["target"]


Let's move this to_pandas() higher to make it only once.

Mr-Geekman · 2022-11-24T09:24:25Z

etna/analysis/eda_utils.py


    Notes
    -----
    `Definition of autocorrelation <https://en.wikipedia.org/wiki/Autocorrelation>`_.

+    `Definition of partial autocorrelation <https://en.wikipedia.org/wiki/Partial_autocorrelation_function>`_.
+
+    This function removes any NaNs from the beginning and end of the series if partial=True.


Write:

* If ``partial=False`` function works with NaNs at any place of the time-series. * if ``partial=True`` function works only with NaNs at the edges of the time-series and fails if there are NaNs inside of it.

…ssue-682

ext.ytarasyuk added 3 commits November 16, 2022 19:42

improve acf and pacf plots

3cb857b

Merge branch 'master' of https://github.com/tinkoff-ai/etna into issu…

27191b3

…e-682

update EDA notebook

8227b8f

github-actions bot temporarily deployed to pull request November 16, 2022 16:58 Inactive

martins0n requested a review from Mr-Geekman November 17, 2022 07:40

Mr-Geekman reviewed Nov 17, 2022

View reviewed changes

etna/analysis/eda_utils.py Show resolved Hide resolved

Mr-Geekman reviewed Nov 17, 2022

View reviewed changes

Mr-Geekman suggested changes Nov 17, 2022

View reviewed changes

fix comments

1a65abb

github-actions bot temporarily deployed to pull request November 17, 2022 15:09 Inactive

fix test

839f9eb

github-actions bot temporarily deployed to pull request November 17, 2022 15:56 Inactive

DBcreator requested a review from Mr-Geekman November 18, 2022 09:00

Mr-Geekman reviewed Nov 18, 2022

View reviewed changes

some fix

ab3bb49

github-actions bot temporarily deployed to pull request November 21, 2022 10:11 Inactive

DBcreator requested a review from Mr-Geekman November 21, 2022 13:57

Mr-Geekman reviewed Nov 21, 2022

View reviewed changes

fix moment with NaN

dde68b4

fix changelog

54ea8e8

DBcreator requested a review from Mr-Geekman November 22, 2022 13:35

github-actions bot temporarily deployed to pull request November 22, 2022 13:38 Inactive

Mr-Geekman reviewed Nov 22, 2022

View reviewed changes

fix NaN moment and changelog

a974d5d

github-actions bot temporarily deployed to pull request November 23, 2022 08:40 Inactive

Modify warning ignore instruction, add autoclose fixture for plots

111444b

github-actions bot temporarily deployed to pull request November 23, 2022 10:55 Inactive

Fix pacf warning

0a0dc09

github-actions bot temporarily deployed to pull request November 23, 2022 11:36 Inactive

DBcreator requested a review from Mr-Geekman November 24, 2022 09:19

Mr-Geekman reviewed Nov 24, 2022

View reviewed changes

ext.ytarasyuk added 2 commits November 24, 2022 12:42

fix in docs

093cb0b

Merge branch 'issue-682' of https://github.com/tinkoff-ai/etna into i…

9ab3eaa

…ssue-682

github-actions bot temporarily deployed to pull request November 24, 2022 09:48 Inactive

Mr-Geekman approved these changes Nov 24, 2022

View reviewed changes

Merge branch 'master' into issue-682

b1ad28b

github-actions bot temporarily deployed to pull request November 25, 2022 06:54 Inactive

DBcreator merged commit cfbfb01 into master Nov 25, 2022

Mr-Geekman deleted the issue-682 branch December 7, 2022 10:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve sample_acf and sample_pacf plots #1004

Improve sample_acf and sample_pacf plots #1004

DBcreator commented Nov 16, 2022 •

edited by Mr-Geekman

review-notebook-app bot commented Nov 16, 2022

github-actions bot commented Nov 16, 2022 •

edited

codecov-commenter commented Nov 16, 2022 •

edited

Mr-Geekman Nov 17, 2022

Mr-Geekman Nov 17, 2022

Mr-Geekman Nov 17, 2022 •

edited

Mr-Geekman Nov 17, 2022

Mr-Geekman commented Nov 17, 2022

Mr-Geekman left a comment

DBcreator commented Nov 17, 2022

Mr-Geekman Nov 18, 2022

Mr-Geekman Nov 18, 2022

DBcreator commented Nov 21, 2022

Mr-Geekman Nov 21, 2022

Mr-Geekman Nov 21, 2022

Mr-Geekman Nov 22, 2022

Mr-Geekman Nov 22, 2022

Mr-Geekman Nov 22, 2022

Mr-Geekman Nov 24, 2022

Mr-Geekman Nov 24, 2022


		indices2remove_end = []

		current_end = len(df_slice.values) - 1

Improve sample_acf and sample_pacf plots #1004

Improve sample_acf and sample_pacf plots #1004

Conversation

DBcreator commented Nov 16, 2022 • edited by Mr-Geekman

Before submitting (must do checklist)

Proposed Changes

Closing issues

review-notebook-app bot commented Nov 16, 2022

github-actions bot commented Nov 16, 2022 • edited

codecov-commenter commented Nov 16, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mr-Geekman Nov 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mr-Geekman commented Nov 17, 2022

Mr-Geekman left a comment

Choose a reason for hiding this comment

DBcreator commented Nov 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DBcreator commented Nov 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DBcreator commented Nov 16, 2022 •

edited by Mr-Geekman

github-actions bot commented Nov 16, 2022 •

edited

codecov-commenter commented Nov 16, 2022 •

edited

Mr-Geekman Nov 17, 2022 •

edited