Add the smoothing utility #176

dshemetov · 2020-07-31T23:57:47Z

This PR adds the smoothing utility and its tests as part of #171. The utility has not replaced the previous smoother implemetations in the rest of the code base, as I received the suggestion to do those in separate PRs.

dshemetov · 2020-08-01T00:09:01Z

Just to confirm: @krivard I recall you suggested I start by integrating into jhu? I can start that work next.

dshemetov · 2020-08-01T00:11:31Z

I think this utility can contain its default values in its implementation in smooth.py, so I'm removing the dependence on config.py. If someone disagrees with that decision, please let me know, I'm very open to suggestions to all of my code 😃

krivard

Pointed out some dangling TODOs but no substantive problems. We should probably get @statsmaths to do a technical review as a cross-check.

_delphi_utils_python/tests/test_smooth.py

_delphi_utils_python/delphi_utils/smooth.py

.gitignore

* implements the existing smoothers found in the indicators (window average, gaussian local linear regression) * adds the more general Savitzky-Golay filter as 'savgol' with gaussian weighting * adds imputation options * adds tests to match

nmdefries · 2020-10-20T23:34:05Z

_delphi_utils_python/delphi_utils/smooth.py

+    methods and provides reasonable defaults. Basic usage can be found in the examples below.
+
+    The smoother function takes numpy arrays as input, expecting the values to come from a
+    regularly-spaced time grid. NANs are ok, as long as the array does not begin with a NAN. The


Are you expecting to see time series beginning with NaN? If so, it might be worth explicitly trimming them off rather than worrying about how they might cause problems.

I think I have an error if the series begins with a NaN. I wanted the smooth methods to preserve the length of the array, expecting the user to insert the output back into a dataframe with a fixed index.

Line 415. Though now I think it makes sense to pull that check out to __init__.

* move smoother first value nan check to impute, so it's less buried * clean documentation in a few functions * remove @classmethod from savgol_coeffs * fix breaking tests

nmdefries · 2020-10-22T13:35:43Z

_delphi_utils_python/delphi_utils/smooth.py

+    @classmethod
+    def savgol_coeffs(cls, nl, nr, poly_fit_degree, gaussian_bandwidth=100):


I can't see that this function is ever used as a class method (although I'm not familiar with this functionality). If you end up removing the @classmethod, this method can use class attributes for poly_fit_degree and gaussian_bandwidth.

The gaussian_bandwidth default here is different than the default in __init__().

Thanks for catching the different default. I think the @classmethod is left over from when I was testing this class and wanted to call this function without instantiating an object. At this point, it's probably unlikely anyone else will call it independently, so we can probably remove that.

nmdefries · 2020-10-22T14:28:50Z

_delphi_utils_python/delphi_utils/smooth.py

+        basis_vector = np.zeros(window_length)
+        coeffs = np.zeros(window_length)
+        for i in range(window_length):
+            basis_vector = np.zeros(window_length)


Defined basis_vector twice; first is unused.

nmdefries

Looks good to me. Nice documentation -- appreciate the example uses!

A couple tests are failing:

FAILED test_smooth.py::TestSmoothers::test_left_gauss_linear_smoother - ValueError: The window_length must be smaller than the length of the signal.
FAILED test_smooth.py::TestSmoothers::test_causal_savgol_smoother - ValueError: The window_length must be smaller than the length of the signal.

dshemetov · 2020-10-22T23:13:45Z

@nmdefries incorporated your suggestions, thanks!

krivard · 2020-10-28T15:00:03Z

I'm still showing the following failing test; do you have changes you haven't pushed up to github yet? It looks like there was conversation after your last commit.

self = <test_smooth.TestSmoothers object at 0x7fe665caeb50>

    def test_impute(self):
        # test the nan imputer
        signal = np.array([i if i % 3 else np.nan for i in range(1, 40)])
        assert np.allclose(Smoother(impute_method=None).impute(signal), signal, equal_nan=True)

        # test the zeros imputer
        signal = np.array([i if i % 3 else np.nan for i in range(1, 40)])
        assert np.allclose(
            Smoother(impute_method="zeros").impute(signal),
            np.array([i if i % 3 else 0.0 for i in range(1, 40)])
        )

        # make a signal with periodic nans to test the imputer
        signal = np.array([i if i % 3 else np.nan for i in range(1, 40)])
        # test that the non-nan values are unchanged
        not_nans_ixs = np.bitwise_xor(np.isnan(signal, where=True), np.full(len(signal), True))
        smoothed_signal = Smoother().savgol_impute(signal)
        assert np.allclose(signal[not_nans_ixs], smoothed_signal[not_nans_ixs])
        # test that the imputer is close to the true line
>       assert np.allclose(range(1, 40), smoothed_signal, atol=0.5)
E       assert False
E        +  where False = <function allclose at 0x7fe67033dee0>(range(1, 40), array([ 1.        ,  2.        ,  2.40533001,  4.        ,  5.        ,\n        6.47948675,  7.        ,  8.        , ...      , 32.        , 33.01427941, 34.        , 35.        ,\n       36.00357408, 37.        , 38.        , 38.99659138]), atol=0.5)
E        +    where <function allclose at 0x7fe67033dee0> = np.allclose
E        +    and   range(1, 40) = range(1, 40)

test_smooth.py:133: AssertionError

dshemetov · 2020-10-28T23:47:40Z

This is really strange... I have that test, it's passing on my end, and I have pushed everything. What else could be wrong?

dshemetov · 2020-10-29T00:05:25Z

The commit hash I'm on locally is the same as the one here. Stumped.

* converts to numpy arrays under the hood and converts back after * updated docstrings and tests to match

dshemetov · 2020-10-29T19:12:13Z

While this is blocked, I added an interface for handling pd.Series. This should let indicators use the transform method and not worry about numpy conversions.

krivard · 2020-11-03T19:51:49Z

What version of python are you using? Prod is 3.8.2; my local is 3.8.5.

dshemetov · 2020-11-03T21:10:15Z

I'm using 3.8.5.

krivard · 2020-11-03T22:18:52Z

Dmitry and I debugged this offline. Reproduced test failure on a fresh clone and fresh python environment on my machine; reproduced passing condition on a fresh clone and fresh python environment (two different kinds!) on his with matching Python version; reproduced passing condition on a fresh clone and fresh python environment on prod with indicator's Python. Prod wins; I give up. 👿 👻 👺

dshemetov requested a review from krivard July 31, 2020 23:59

dshemetov marked this pull request as draft August 1, 2020 00:04

dshemetov changed the title ~~Adding the smoothing utility~~ Add the smoothing utility Aug 1, 2020

dshemetov mentioned this pull request Aug 4, 2020

Add Gaussian weight regression to the Savitzky-Golay smoother #174

Closed

krivard approved these changes Aug 10, 2020

View reviewed changes

dshemetov mentioned this pull request Aug 12, 2020

GHT has misleading declining trend --- in areas with unexpectedly low volume? #67

Closed

dshemetov self-assigned this Aug 13, 2020

dshemetov marked this pull request as ready for review September 2, 2020 23:44

krivard reviewed Sep 3, 2020

View reviewed changes

_delphi_utils_python/tests/test_smooth.py Outdated Show resolved Hide resolved

_delphi_utils_python/delphi_utils/smooth.py Outdated Show resolved Hide resolved

.gitignore Outdated Show resolved Hide resolved

krivard requested a review from taylor-arnold October 9, 2020 20:54

krivard mentioned this pull request Oct 9, 2020

Refactor indicators to use delphi_utils library calls #306

Closed

Add the smoothing utility class

3912f4c

* implements the existing smoothers found in the indicators (window average, gaussian local linear regression) * adds the more general Savitzky-Golay filter as 'savgol' with gaussian weighting * adds imputation options * adds tests to match

dshemetov force-pushed the dev-smoothing branch from aab94b9 to 3912f4c Compare October 15, 2020 01:04

dshemetov added 2 commits October 15, 2020 19:46

Add deprecation warning to left_gauss_linear and add more documentation

f6af0da

Small test docstring change

9925348

nmdefries self-requested a review October 16, 2020 20:36

krivard removed the request for review from taylor-arnold October 16, 2020 21:15

nmdefries reviewed Oct 20, 2020

View reviewed changes

Small smoother updates:

4bf3410

* move smoother first value nan check to impute, so it's less buried * clean documentation in a few functions * remove @classmethod from savgol_coeffs * fix breaking tests

nmdefries reviewed Oct 22, 2020

View reviewed changes

Add smoother interface for handling pandas Series:

7cfed72

* converts to numpy arrays under the hood and converts back after * updated docstrings and tests to match

Merge branch 'main' into dev-smoothing

bab0b51

krivard merged commit 92a750c into main Nov 3, 2020

krivard deleted the dev-smoothing branch February 9, 2021 18:54

		@classmethod
		def savgol_coeffs(cls, nl, nr, poly_fit_degree, gaussian_bandwidth=100):

Add the smoothing utility #176

Add the smoothing utility #176

Uh oh!

Conversation

dshemetov commented Jul 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dshemetov commented Aug 1, 2020

Uh oh!

dshemetov commented Aug 1, 2020

Uh oh!

krivard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nmdefries Oct 20, 2020

Choose a reason for hiding this comment

Uh oh!

dshemetov Oct 22, 2020

Choose a reason for hiding this comment

Uh oh!

dshemetov Oct 22, 2020

Choose a reason for hiding this comment

Uh oh!

nmdefries Oct 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dshemetov Oct 22, 2020

Choose a reason for hiding this comment

Uh oh!

nmdefries Oct 22, 2020

Choose a reason for hiding this comment

Uh oh!

nmdefries left a comment

Choose a reason for hiding this comment

Uh oh!

dshemetov commented Oct 22, 2020

Uh oh!

krivard commented Oct 28, 2020

Uh oh!

dshemetov commented Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dshemetov commented Oct 29, 2020

Uh oh!

dshemetov commented Oct 29, 2020

Uh oh!

krivard commented Nov 3, 2020

Uh oh!

dshemetov commented Nov 3, 2020

Uh oh!

krivard commented Nov 3, 2020

Uh oh!

Uh oh!

dshemetov commented Jul 31, 2020 •

edited

Loading

nmdefries Oct 22, 2020 •

edited

Loading

dshemetov commented Oct 28, 2020 •

edited

Loading