-
-
Notifications
You must be signed in to change notification settings - Fork 19.4k
BUG: Empty PeriodIndex issues #13079
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.
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
Conversation
8ba87d0 to
db8b3ad
Compare
pandas/tseries/tests/test_period.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can u also test _simple_new adding name attribute?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test above does that, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
above doesn't check empty case. Though _shallow_copy internally uses _simple_new, I meant to guarantee _simple_new API.
db8b3ad to
724c587
Compare
pandas/tseries/period.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jreback Is this the best way to do this? Previously it was creating float64 arrays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that would probably work. IIUC, this is a non ndarray, that could be ints, or objects (meaning Periods, or coercable to).
have a look at com.is_period_arraylike as well. Though that actually traverses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
com.is_period_arraylike didn't work, so we're back to the try except
a486745 to
2cac538
Compare
doc/source/whatsnew/v0.18.2.txt
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
explain what changing is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also say: .resample(..) with a PeriodIndex
c23aafb to
cca6cd3
Compare
|
@jreback @sinhrks: ======================================================================
ERROR: test_resample_empty (pandas.tseries.tests.test_resample.TestPeriodIndex)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/travis/build/pydata/pandas/pandas/tseries/tests/test_resample.py", line 2054, in test_resample_empty
result = getattr(s.resample(freq), method)()
File "/home/travis/build/pydata/pandas/pandas/tseries/resample.py", line 511, in f
return self._downsample(_method)
File "/home/travis/build/pydata/pandas/pandas/tseries/resample.py", line 805, in _downsample
return self._groupby_and_aggregate(how, grouper=grouper)
File "/home/travis/build/pydata/pandas/pandas/tseries/resample.py", line 379, in _groupby_and_aggregate
result = grouped.apply(how, *args, **kwargs)
File "/home/travis/build/pydata/pandas/pandas/core/groupby.py", line 645, in apply
@wraps(func)
File "/home/travis/miniconda/envs/pandas/lib/python2.7/functools.py", line 33, in update_wrapper
setattr(wrapper, attr, getattr(wrapped, attr))
AttributeError: 'str' object has no attribute '__module__'
----------------------------------------------------------------------
Ran 10105 tests in 537.776s
FAILED (SKIP=177, errors=1) |
|
I remember seeing those. I think an attribute is being passed which isn't defined for that object |
doc/source/whatsnew/v0.18.2.txt
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't make sense; the bug is returning an incorrect freq when empty, right?
pandas/tseries/period.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks to change the current behavior below.
pd.PeriodIndex._simple_new(np.array([pd.Period('2011-01', freq='M')]))
# PeriodIndex(['2011-01'], dtype='int64', freq='M')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is correct though, any reason we don't want that?
@MaximilianR as an aside pls run asv on --bench period to verify no changes (under py3) in perf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is ugly and I would love a better way of doing it.
I do think the outcomes are correct though - @sinhrks do you disagree?
Open to nice implementation though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just wondering whether the following behavior (on current master) are the same. Also is no inconsistencies between list and ndarray.
pd.PeriodIndex._simple_new([pd.Period('2011-01', freq='M')])
# PeriodIndex(['2011-01'], dtype='int64', freq='M')
pd.PeriodIndex._simple_new(np.array([pd.Period('2011-01', freq='M')]))
# PeriodIndex(['2011-01'], dtype='int64', freq='M')
pd.PeriodIndex._simple_new([1.1], freq='M')
# ValueError: Ordinal must be an integer
pd.PeriodIndex._simple_new(np.array([1.1]), freq='M')
# ValueError: Ordinal must be an integer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sinhrks not sure I understand, those seem right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @sinhrks is asking whether the new implementation is consistent with those examples, from master. I'll check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wasn't working well. Changed & tested.
|
want to rebase/update |
Yup! |
19d5178 to
5982e21
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can u explicitly compare .freq also, as assert_index_equal doesn't (i'll submit another PR once this is fixed).
0672f28 to
c65e83e
Compare
|
Should I remove this test now @jreback? I think it's covered by the Base tests now https://github.com/pydata/pandas/blob/master/pandas/tseries/tests/test_resample.py#L1411 |
c65e83e to
5bd2f40
Compare
|
yes (just inspect it to make sure that we are covering the bases) |
81cb7a3 to
1e539df
Compare
|
OK, I moved a couple around to ensure we weren't losing anything |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you put this in Base? (e.g. its a generic test)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done. I had thought generic was to run tests across a number of types, while this doesn't depend on the index type.
1e539df to
4a5fa12
Compare
4a5fa12 to
8c7b9db
Compare
|
thanks @MaximilianR @sinhrks this is great8! small issue. I am not convinced that we are testing |
|
thanks @MaximilianR :) |
git diff upstream/master | flake8 --diffcloses #13067
closes #13212