REF: Refactor window/test_moments.py #30542
Conversation
|
Hello @charlesdong1991! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2019-12-30 14:06:20 UTC |
simonjayhawkins
added
Testing
|
|
||
| df2_result = f(df2) | ||
| tm.assert_frame_equal(df2_result, df2_expected) | ||
| except (ImportError): |
There was a problem hiding this comment.
Instead could you use a pytest.param(..., mark=pytest.mark.skipif(...) for the parameter that needs scipy.
Also, shouldn't all these parameters operate without scipy?
There was a problem hiding this comment.
yeah, they could operate without scipy. I removed.
| expected = Series([1.0, 2.0, 3.0]) | ||
| tm.assert_series_equal(result, expected) | ||
|
|
||
| def _check_expanding( |
There was a problem hiding this comment.
Could we break this into several different tests?
There was a problem hiding this comment.
i broke them into two
| ser = Series([], dtype=np.float64) | ||
| tm.assert_series_equal(ser, ser.expanding().apply(lambda x: x.mean(), raw=raw)) | ||
|
|
||
| # GH 8080 |
There was a problem hiding this comment.
This test looks like it could stand alone in its own function.
| # here to make this pass | ||
| self._check_expanding(expanding_mean, np.mean, preserve_nan=False) | ||
|
|
||
| ser = Series([], dtype=np.float64) |
There was a problem hiding this comment.
This test looks like it could stand alone in its own function.
| def test_ewmcorr_pairwise(self): | ||
| self._check_pairwise_moment("ewm", "corr", span=10, min_periods=5) | ||
|
|
||
| def _check_binary_ew(self, name): |
There was a problem hiding this comment.
Could we break this into several different tests?
| result = getattr(ewm, method)() | ||
| tm.assert_almost_equal(result, vals) | ||
|
|
||
| def _check_ew(self, name=None, preserve_nan=False): |
There was a problem hiding this comment.
Could we break this into several different tests?
| super()._create_data() | ||
| self.data = _consistency_data | ||
|
|
||
| def _test_moments_consistency( |
There was a problem hiding this comment.
Could we break this into several different tests?
|
all of @mroeschke suggestions are good but i’d like to first just move things then actually break apart - otherwise the diffs are very hard to see |
|
@jreback thanks for your comments! Could you please explain a bit more about your point, i think currently this PR is basically simply moving things (except for a couple parametrizations). Or you want them to be split to different files/directories? Would be nice to hear more so that I could make changes based on @mroeschke nice reviews later on tonight. Thanks! |
|
i want things move with minimal changes in a PR so pls make this PR solely about moving things to subdirectories (sure you will need some very minor changes to get things to pass) but don’t do anything more than that in this PR |
|
Hi, @jreback I revert the change based on your tips, and in this PR i only split tests into three sub-files To be clear of what i did in this PR: @mroeschke many thanks for your nice reviews, following @jreback suggestion, I will address all of them in the follow-up PR and make this PR solely for tests split into sub-files. |
|
thanks @charlesdong1991 yeah happy to have followups to split / clean this. |
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diff