Separate out non-scalar tests from scalar tests; move to ?? in follow-up #18142

jbrockmendel · 2017-11-06T16:32:58Z

The goal I have in mind is getting to the point where we can test (and measure coverage) tslibs in isolation. That means isolating DataFrame and Series tests from everything else.

This PR doesn't change any tests and doesn't move anything across modules, just puts DataFrame and Series tests in their own classes. Exactly what modules those belong in I haven't thought out.

If I did this right, it shouldn't have any overlap with other outstanding PRs.

codecov · 2017-11-06T18:04:44Z

Codecov Report

Merging #18142 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #18142      +/-   ##
==========================================
- Coverage   91.38%   91.36%   -0.02%     
==========================================
  Files         164      164              
  Lines       49790    49790              
==========================================
- Hits        45501    45492       -9     
- Misses       4289     4298       +9

Flag	Coverage Δ
#multiple	`89.17% <ø> (ø)`	⬆️
#single	`39.49% <ø> (-0.07%)`	⬇️

Impacted Files	Coverage Δ
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.8% <0%> (-0.1%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 774030c...16ccef4. Read the comment docs.

jreback · 2017-11-06T18:21:58Z

pandas/tests/scalar/test_timedelta.py

@@ -640,27 +640,6 @@ def conv(v):
        # invalid
        pytest.raises(ValueError, ct, '- 1days, 00')

-    def test_overflow(self):


so rather than move these within the same file I would like to create another directory level (like we have for indexes) and put them in separate files, e.g.

pandas/tests/scalar/timedeltas/test_basic.py .....test_slicing.py

etc. much more organized to have separate units of tests in separate named files (rather than classes). be sure to update setup.py with the additional directories.

I'm amenable to that in general, but the relevantissue here is that the new classes (which I agree don't belong here) are not scalar tests at all. Some are for DatetimeIndex/TimedeltaIndex which I can figure out destinations for. I haven't looked at Series/DataFrame tests to see where those might go. Or there could be something like tests/tslibs/vectorized/

the would rather u do a PR which actually moves them to the right location rather than a temporary one

Fair enough

jbrockmendel · 2017-11-06T22:32:08Z

Just pushed a commit moving these tests to the appropriate modules.

jreback

you need to be very careful with this, move less at a time maybe

jreback · 2017-11-07T13:20:34Z

pandas/tests/frame/test_timestamps.py

+                    Series, DataFrame, DatetimeIndex)
+
+
+class TestDataFrameTimestamps(object):


so we already have test_timeseries.py, is there a reason you are creating a new module here?

Not really, will move.

jreback · 2017-11-07T13:20:59Z

pandas/tests/frame/test_timestamps.py

+        ts = Timestamp(t)
+        data[ts] = np.nan  # works
+
+    def test_to_html_timestamp(self):


prob go with other html output tests (which I think are in formats)

jreback · 2017-11-07T13:21:13Z

pandas/tests/frame/test_timestamps.py

+        result = df.to_html()
+        assert '2000-01-01' in result
+
+    def test_compare_invalid(self):


this has nothing to do with frame

I agree it looks like an unnecessary use of the constructor, am assuming that someone had a reason.

jreback · 2017-11-07T13:21:27Z

pandas/tests/frame/test_timestamps.py

+        s = Series(date_range('1/1/2000', periods=10))
+
+        def f(x):
+            return (x.hour, x.day, x.month)


this should be in series

It relies on DataFrame (although the whole thing is a smoke test). Maybe rename test_series_map_box_timestamps--> test_map_box_timestamps?

jreback · 2017-11-07T13:21:34Z

pandas/tests/frame/test_timestamps.py

+    def test_frame_setitem_timestamp(self):
+        # GH#2155
+        columns = DatetimeIndex(start='1/1/2012', end='2/1/2012',
+                                freq=offsets.BDay())


? the columns above are explicitly passed to the DataFrame constructor...

jreback · 2017-11-07T13:22:23Z

pandas/tests/indexes/datetimes/test_datetime.py

+        # extra fields from DatetimeIndex like quarter and week
+        idx = tm.makeDateIndex(100)
+
+        fields = ['dayofweek', 'dayofyear', 'week', 'weekofyear', 'quarter',


these are in ops or misc

Moving this class to test_ops. Keeping it together because I like the fact that it is reasonably circumscribed as "vectorized versions of Timestamp operations", i.e. except for the DatetimeIndex constructor, most of what is being tested is in tslibs.

jreback · 2017-11-07T13:22:38Z

pandas/tests/indexes/datetimes/test_datetime.py

+                         periods=5).tz_localize('UTC').tz_convert('US/Eastern')
+        result = dti.round('D')
+        expected = date_range('20130101', periods=5).tz_localize('US/Eastern')
+        tm.assert_index_equal(result, expected)


jbrockmendel · 2017-11-07T15:16:31Z

you need to be very careful with this, move less at a time maybe

Hah that was kinda the idea with the original version.

jreback · 2017-11-07T16:06:14Z

pandas/tests/frame/test_timeseries.py

+            return (x.hour, x.day, x.month)
+
+        # it works!
+        s.map(f)


split this test: 1 for series (move to series dir); leave other here but i think it goes in ops
or wherever apply tests are

jreback · 2017-11-07T16:08:38Z

pandas/tests/frame/test_timeseries.py

+        # GH 8058
+        df = DataFrame(np.random.randn(5, 2))
+        a = df[0]
+        b = Series(np.random.randn(5))


this is a series test

OK. Any complaint if I change the DataFrame call to use Series instead?

jreback · 2017-11-07T16:09:01Z

pandas/tests/indexes/datetimes/test_date_range.py

+        ts = Timestamp('20090415', tz=pytz.timezone('US/Eastern'), freq='D')
+        assert ts == stamp
+
+    def test_date_range_timestamp_equiv_explicit_dateutil(self):


this can be made a decorator FYI

add a note to do this

How does the decorator usage work? grepping didn't turn up any examples.

jreback · 2017-11-07T16:09:24Z

pandas/tests/indexes/datetimes/test_date_range.py

+
+    def test_date_range_timestamp_equiv_explicit_dateutil(self):
+        tm._skip_if_windows_python_3()
+        from pandas._libs.tslibs.timezones import dateutil_gettz as gettz


import should be at the top

jreback · 2017-11-07T16:09:47Z

pandas/tests/indexes/datetimes/test_ops.py

+        assert idx.freq == Timestamp(idx[-1], idx.freq).freq
+        assert idx.freqstr == Timestamp(idx[-1], idx.freq).freqstr
+
+    def test_round(self):


move with similar tests

The other test_round in this module already needs refactoring to use pytest.parametrize. Will follow up.

that’s fine but move it next to it

jreback · 2017-11-07T16:10:22Z

pandas/tests/indexes/timedeltas/test_timedelta.py

+class TestTimedeltaIndexVectorizedTimedelta(object):
+    def test_contains(self):
+        # Checking for any NaT-like objects
+        # GH 13603


this is an indexing test

jreback · 2017-11-07T16:10:52Z

pandas/tests/indexes/timedeltas/test_timedelta.py

+        for v in [pd.NaT, None, float('nan'), np.nan]:
+            assert (v in td)
+
+    def test_nat_converters(self):


we have a section on nat tests

There are many of these spread around. I really think we should focus for now on getting non-scalar tests out of scalar and recognize that this is a more-than-one-PR task.

jreback · 2017-11-07T16:10:58Z

pandas/tests/indexes/timedeltas/test_timedelta.py

+        assert all(hash(td) == hash(td.to_pytimedelta()) for td in tds)
+
+    def test_round(self):
+        t1 = timedelta_range('1 days', periods=3, freq='1 min 2 s 3 us')


move to like tests

jreback · 2017-11-07T16:14:25Z

pandas/tests/scalar/test_timestamp.py

@@ -410,17 +406,6 @@ def test_tz(self):
        assert conv.hour == 19

    def test_tz_localize_ambiguous(self):
-
-        ts = Timestamp('2014-11-02 01:00')
-        ts_dst = ts.tz_localize('US/Eastern', ambiguous=True)


why are you moving this?

Because this portion of the test was moved to a non-scalar class.

this is a scalar test

The goal here is to isolate tests for tslibs. The portion of the test that got moved uses date_range, which puts it in the DatetimeIndex tests.

…alar_idx_ser_df

jreback · 2017-11-08T11:39:09Z

pandas/tests/indexes/timedeltas/test_timedelta.py

@@ -401,3 +401,44 @@ def test_series_box_timedelta(self):
        s = Series(rng)
        assert isinstance(s[1], Timedelta)
        assert isinstance(s.iat[2], Timedelta)
+
+


make a note to parametrize this

Not clear what "this" is in context.

this is the calling of testit, simple enough should be to paramerize the units which its iterating

jreback · 2017-11-08T11:39:35Z

pandas/tests/indexes/datetimes/test_date_range.py

+        ts = Timestamp('20090415', tz=pytz.timezone('US/Eastern'), freq='D')
+        assert ts == stamp
+
+    def test_date_range_timestamp_equiv_explicit_dateutil(self):


add a note to do this

jreback · 2017-11-08T11:40:38Z

pandas/tests/scalar/test_timedelta.py

@@ -502,66 +469,6 @@ def test_round(self):
        for freq in ['Y', 'M', 'foobar']:


some more round that should move?

This is the scalar test, pretty sure its in the right place.

jreback · 2017-11-08T11:41:36Z

pandas/tests/scalar/test_timedelta.py

@@ -676,9 +562,6 @@ def test_timedelta_hash_equality(self):
        d = {td: 2}
        assert d[v] == 2

-        tds = timedelta_range('1 second', periods=20)


why did u move this? and not all

why did u move this?

timedelta_range --> not scalar

and not all

You stopped in the middle of a

jreback · 2017-11-08T11:45:55Z

pandas/tests/tseries/test_timezones.py

+        repr(series.index[0])
+
+    def test_getitem_pydatetime_tz(self):
+        index = date_range(start='2012-12-24 16:00', end='2012-12-24 18:00',


indexing test

This test uses methods of this class; let's be extra careful before breaking it up.

jreback · 2017-11-08T11:47:06Z

pandas/tests/tseries/test_timezones.py

+
+        s = Series(np.random.randn(len(dr)), index=dr)
+
+        # it works!


this belongs elsewhere

Have something in mind?

pandas/tests/series/test_timeseries.py where asfreq is tested.

jreback · 2017-11-08T11:48:43Z

pandas/tests/tseries/test_timezones.py

+
+
+class TestDataFrameTimeZones(object):
+    timezones = ['UTC', 'Asia/Tokyo', 'US/Eastern', 'dateutil/US/Pacific']


what is this for?

There are a bunch of GH issues related to timezones/timeseries/... and many of them involve behavior that is specific to DataFrame/Series/FooIndex. The goal for this PR (and presumably its a multi-PR goal) is to separate these cases to test in isolation.

i.e. this just collects all of the test_timezones tests that use pd.DataFrame.

…alar_idx_ser_df

scalar_idx_ser_df

…alar_idx_ser_df

jreback · 2017-11-17T01:18:48Z

can you rebase

…alar_idx_ser_df

jreback

I would do this in several stages. You are moving too much. Let's do the non-controversial things first.

jreback · 2017-11-19T16:41:28Z

pandas/tests/indexes/timedeltas/test_indexing.py

@@ -11,6 +11,17 @@
 class TestTimedeltaIndex(object):
    _multiprocess_can_split_ = True


remove this

jreback · 2017-11-19T16:42:05Z

pandas/tests/indexes/timedeltas/test_indexing.py

+        # Checking for any NaT-like objects
+        # GH 13603
+        td = pd.to_timedelta(range(5), unit='d') + pd.offsets.Hour(1)
+        for v in [pd.NaT, None, float('nan'), np.nan]:


maybe should consolidate these types of tests in scalar/test_nat.py (even though you are testing the index itself here).

jreback · 2017-11-19T16:42:58Z

pandas/tests/indexes/timedeltas/test_timedelta.py

@@ -401,3 +401,44 @@ def test_series_box_timedelta(self):
        s = Series(rng)
        assert isinstance(s[1], Timedelta)
        assert isinstance(s.iat[2], Timedelta)
+
+


this is the calling of testit, simple enough should be to paramerize the units which its iterating

jreback · 2017-11-19T16:43:41Z

pandas/tests/indexes/timedeltas/test_ops.py

+        t1a = timedelta_range('1 days', periods=3, freq='1 min 2 s')
+        t1c = pd.TimedeltaIndex([1, 1, 1], unit='D')
+
+        # note that negative times round DOWN! so don't give whole numbers


also should parametrize this (TODO)

jreback · 2017-11-19T16:45:40Z

pandas/tests/tseries/test_timezones.py

+        assert_series_equal(result, expected1)
+
+    def test_localized_at_time_between_time(self):
+        from datetime import time


imports to the top

jreback · 2017-11-19T16:46:26Z

pandas/tests/tseries/test_timezones.py

+
+        s = Series(np.random.randn(len(dr)), index=dr)
+
+        # it works!


pandas/tests/series/test_timeseries.py where asfreq is tested.

jreback · 2017-11-19T16:47:52Z

pandas/tests/tseries/test_timezones.py

@@ -65,6 +65,145 @@ def cmptz(self, tz1, tz2):
        # tests.
        return tz1.zone == tz2.zone

+


I think you need to audit what you are moving here. A lot of these are fairly generic routines for series/dataframe that are actually testing the operation in question, and NOT the timezone per-se. Moving them here is really confusing.

jbrockmendel · 2017-11-19T16:59:58Z

I would do this in several stages. You are moving too much. Let's do the non-controversial things first.

Sounds good.

jreback · 2017-11-20T11:18:21Z

closing as scope out of hand; replaced by multiple smaller prs

Separate out non-scalar tests from scalar tests; move to ?? in follow-up

e7d5373

gfyoung added Clean Testing pandas testing functions or related to the test suite labels Nov 6, 2017

jreback requested changes Nov 6, 2017

View reviewed changes

per reviewer request, move non-scalar test classes

f99aa04

jreback requested changes Nov 7, 2017

View reviewed changes

jbrockmendel added 3 commits November 7, 2017 07:18

move test_timestamps-->test_timeseries per reviewer request

73e2f02

move html test per request

157c7fa

move tests from test_datetime per reviewer request

14810f6

jreback requested changes Nov 7, 2017

View reviewed changes

jbrockmendel added 6 commits November 7, 2017 08:32

Movement per reviewer request

cb932a2

Merge branch 'master' of https://github.com/pandas-dev/pandas into sc…

c0a5f77

…alar_idx_ser_df

movements per reviewer request

b575b3a

Merge branch 'master' of https://github.com/pandas-dev/pandas into sc…

9f06079

…alar_idx_ser_df

flake8 whitespace fixup

f628aef

Merge branch 'master' of https://github.com/pandas-dev/pandas into sc…

ea7d33b

…alar_idx_ser_df

jreback requested changes Nov 8, 2017

View reviewed changes

jbrockmendel added 4 commits November 8, 2017 07:40

Merge branch 'master' of https://github.com/pandas-dev/pandas into sc…

817281b

…alar_idx_ser_df

Merge branch 'master' of https://github.com/pandas-dev/pandas into

e467040

scalar_idx_ser_df

dummy commit to force CI

66c65c1

Merge branch 'master' of https://github.com/pandas-dev/pandas into sc…

f932060

…alar_idx_ser_df

Merge branch 'master' of https://github.com/pandas-dev/pandas into sc…

16ccef4

…alar_idx_ser_df

jreback requested changes Nov 19, 2017

View reviewed changes

jbrockmendel mentioned this pull request Nov 20, 2017

move a small set of non-scalar tests out of scalar.test_timestamp #18377

Merged

jreback closed this Nov 20, 2017

jbrockmendel deleted the scalar_idx_ser_df branch December 8, 2017 19:38

		Series, DataFrame, DatetimeIndex)


		class TestDataFrameTimestamps(object):

		@@ -502,66 +469,6 @@ def test_round(self):
		for freq in ['Y', 'M', 'foobar']:



		class TestDataFrameTimeZones(object):
		timezones = ['UTC', 'Asia/Tokyo', 'US/Eastern', 'dateutil/US/Pacific']

		@@ -11,6 +11,17 @@
		class TestTimedeltaIndex(object):
		_multiprocess_can_split_ = True

		@@ -65,6 +65,145 @@ def cmptz(self, tz1, tz2):
		# tests.
		return tz1.zone == tz2.zone

Separate out non-scalar tests from scalar tests; move to ?? in follow-up #18142

Separate out non-scalar tests from scalar tests; move to ?? in follow-up #18142

Conversation

jbrockmendel commented Nov 6, 2017

codecov bot commented Nov 6, 2017 • edited Loading

Codecov Report

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

jbrockmendel commented Nov 6, 2017

jreback left a 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

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

jbrockmendel commented Nov 7, 2017

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

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

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

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

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

jreback commented Nov 17, 2017

jreback left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented Nov 19, 2017

jreback commented Nov 20, 2017

codecov bot commented Nov 6, 2017 •

edited

Loading