Skip to content

CLN: Move some PI/DTI methods to EA subclasses, implement tests #22961

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Oct 8, 2018
Merged

CLN: Move some PI/DTI methods to EA subclasses, implement tests #22961

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
0a5f343
Move some PeriodIndex/DatetimeIndex methods to EA subclasses, impleme…
jbrockmendel Oct 3, 2018
f1f5146
fixup missing import
jbrockmendel Oct 3, 2018
effef65
Merge branch 'master' of https://github.com/pandas-dev/pandas into dl…
jbrockmendel Oct 3, 2018
4eb5060
Suggested renames
jbrockmendel Oct 3, 2018
09f1b02
Revert change that broke things
jbrockmendel Oct 3, 2018
68116c4
Revert change that broke tests
jbrockmendel Oct 3, 2018
12eb1b9
Fix failing test by ammending constructor
jbrockmendel Oct 3, 2018
ed30d73
flake8 whitespace fixup
jbrockmendel Oct 3, 2018
5b4c828
requested verbosification
jbrockmendel Oct 4, 2018
61808d3
fixup property wrapping
jbrockmendel Oct 4, 2018
927e4bd
Merge branch 'master' of https://github.com/pandas-dev/pandas into dl…
jbrockmendel Oct 5, 2018
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Suggested renames
  • Loading branch information
@jbrockmendel
jbrockmendel committed Oct 3, 2018
commit 4eb5060c00ac1a9a8fa33d71b28373c823150659
36 changes: 18 additions & 18 deletions pandas/tests/arrays/test_datetimelike.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@

# TODO: more freq variants
Copy link
Contributor

@jreback jreback Oct 6, 2018

Choose a reason for hiding this comment

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

I think we have a fixture for this?
Copy link
Member Author

@jbrockmendel jbrockmendel Oct 6, 2018

Choose a reason for hiding this comment

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

I don't see it anywhere. I'll make one at the same time as moving these to conftest.
Copy link
Contributor

@jreback jreback Oct 6, 2018

Choose a reason for hiding this comment

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

pandas/tests/tseries/offsets/conftest.py ? (those are the classes, but imagine have the abbrevs also.
Copy link
Member Author

@jbrockmendel jbrockmendel Oct 6, 2018

Choose a reason for hiding this comment

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

Those don't exist at the moment. I'll make them in the same pass as the other fixture stuff discussed here.
Copy link
Contributor

@jreback jreback Oct 6, 2018

Choose a reason for hiding this comment

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

k cool
@pytest.fixture(params=['D', 'B', 'W', 'M', 'Q', 'Y'])
def per_index(request):
def period_index(request):
Copy link
Member

@jorisvandenbossche jorisvandenbossche Oct 3, 2018

Choose a reason for hiding this comment

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

shouldn't this rather be called 'period_array' ? (since this is in the array tests directory, and is meant to later become the actual array tests?)
Copy link
Member Author

@jbrockmendel jbrockmendel Oct 3, 2018

Choose a reason for hiding this comment

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

shouldn't this rather be called 'period_array'

No, the fixture is specifically returning a PeriodIndex. I imagine at some point when these are more fleshed out they'll go up in conftest, or parts might go in pd.util.testing
"""
A fixture to provide PeriodIndex objects with different frequencies.

Expand All @@ -29,7 +29,7 @@ def per_index(request):


@pytest.fixture(params=['D', 'B', 'W', 'M', 'Q', 'Y'])
Copy link
Contributor

@jreback jreback Oct 6, 2018

Choose a reason for hiding this comment

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

at some point should move these to a pandas/tests/arrays/conftest.py (or event the very to-level conftest)
Copy link
Member Author

@jbrockmendel jbrockmendel Oct 6, 2018

Choose a reason for hiding this comment

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

Definitely. This is bare-bones at the moment and needs some major buffing-up. Current thought is to move them up once they are closer to "done".
Copy link
Contributor

@jreback jreback Oct 6, 2018

Choose a reason for hiding this comment

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

sure
def dt_index(request):
def datetime_index(request):
"""
A fixture to provide DatetimeIndex objects with different frequencies.

Expand Down Expand Up @@ -69,8 +69,8 @@ def test_astype_object(self, tz_naive_fixture):
assert list(asobj) == list(dti)

@pytest.mark.parametrize('freqstr', ['D', 'B', 'W', 'M', 'Q', 'Y'])
def test_to_period(self, dt_index, freqstr):
dti = dt_index
def test_to_period(self, datetime_index, freqstr):
dti = datetime_index
arr = DatetimeArrayMixin(dti)

expected = dti.to_period(freq=freqstr)
Expand All @@ -82,9 +82,9 @@ def test_to_period(self, dt_index, freqstr):
tm.assert_index_equal(pd.Index(result), pd.Index(expected))
Copy link
Member

@jorisvandenbossche jorisvandenbossche Oct 3, 2018

Choose a reason for hiding this comment

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

I am not sure we should test the equivalence between the index and array method in the array tests, that feels a bit out of place (since it is the index one that is calling the array)
Copy link
Member Author

@jbrockmendel jbrockmendel Oct 3, 2018

Choose a reason for hiding this comment

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

The point is that we have fairly robust tests for the Index methods. Instead of effectively re-implementing all those tests, we can just test that the Index/Array methods behave identically.
Copy link
Member

@jorisvandenbossche jorisvandenbossche Oct 3, 2018

Choose a reason for hiding this comment

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

OK, that makes sense (at least for now)
But you could still have fixtures for the arrays and create the index of it if needed inside the test (instead of currently the other way around) ?

@pytest.mark.parametrize('propname', pd.DatetimeIndex._bool_ops)
def test_bool_properties(self, dt_index, propname):
def test_bool_properties(self, datetime_index, propname):
# in this case _bool_ops is just `is_leap_year`
dti = dt_index
dti = datetime_index
arr = DatetimeArrayMixin(dti)
assert dti.freq == arr.freq

Expand All @@ -94,8 +94,8 @@ def test_bool_properties(self, dt_index, propname):
tm.assert_numpy_array_equal(result, expected)

@pytest.mark.parametrize('propname', pd.DatetimeIndex._field_ops)
def test_int_properties(self, dt_index, propname):
dti = dt_index
def test_int_properties(self, datetime_index, propname):
dti = datetime_index
arr = DatetimeArrayMixin(dti)

result = getattr(arr, propname)
Expand Down Expand Up @@ -126,8 +126,8 @@ def test_astype_object(self):

class TestPeriodArray(object):

def test_from_pi(self, per_index):
pi = per_index
def test_from_pi(self, period_index):
pi = period_index
arr = PeriodArrayMixin(pi)
assert list(arr) == list(pi)

Expand All @@ -136,17 +136,17 @@ def test_from_pi(self, per_index):
assert isinstance(pi2, pd.PeriodIndex)
assert list(pi2) == list(arr)

def test_astype_object(self, per_index):
pi = per_index
def test_astype_object(self, period_index):
pi = period_index
arr = PeriodArrayMixin(pi)
asobj = arr.astype('O')
assert isinstance(asobj, np.ndarray)
assert asobj.dtype == 'O'
assert list(asobj) == list(pi)

@pytest.mark.parametrize('how', ['S', 'E'])
def test_to_timestamp(self, how, per_index):
pi = per_index
def test_to_timestamp(self, how, period_index):
pi = period_index
arr = PeriodArrayMixin(pi)

expected = DatetimeArrayMixin(pi.to_timestamp(how=how))
Expand All @@ -158,9 +158,9 @@ def test_to_timestamp(self, how, per_index):
tm.assert_index_equal(pd.Index(result), pd.Index(expected))

@pytest.mark.parametrize('propname', pd.PeriodIndex._bool_ops)
def test_bool_properties(self, per_index, propname):
def test_bool_properties(self, period_index, propname):
# in this case _bool_ops is just `is_leap_year`
pi = per_index
pi = period_index
arr = PeriodArrayMixin(pi)

result = getattr(arr, propname)
Expand All @@ -169,8 +169,8 @@ def test_bool_properties(self, per_index, propname):
tm.assert_numpy_array_equal(result, expected)

@pytest.mark.parametrize('propname', pd.PeriodIndex._field_ops)
def test_int_properties(self, per_index, propname):
pi = per_index
def test_int_properties(self, period_index, propname):
pi = period_index
arr = PeriodArrayMixin(pi)

result = getattr(arr, propname)
Expand Down