ENH: added nunique function to Index by sinhrks · Pull Request #6734 · pandas-dev/pandas (original) (raw)
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Added nunique function to Index same as Series.
I like all the tests!
I would rather see this added in core/base.py (and then remove the Series) one
you can leave some of the tests but add a couple in test_base
did you cover datetime/period index for tests?
Sure. I'll try it.
Actually tests are derived from test_series.py. I'll prepare more tests for time-related indexes.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't specify dtype when constructing an Index, let it infer it properly (unless the test actually is trying to test that).
I've moved value_counts, unique and nunique to the base class, and added more tests. Could you review this?
FYI, I find it helpful to pull master and rebase when I push a PR
(you don't have to , but always nice to get an auto merge), which are usually caused by release note conflicts
also for some reason travis is not picking this up...can you rebase and force push?
Rebased, and I'll take care.
Test has passed.
can you move the imports to the top of the test script?
better to just have them centralized.
was a little leary of a .value_counts() on an index returning a Series, but it DOES make sense.
can you add a timezone aware series & index in the test_base/Ops/objs.py (all should work, but not sure that case is there).
This is great. I use such functionality all the time and indeed wrap like pd.Series(s.index.tolist()).value_counts().
+1 for .value_counts() added to base Index class.
@nehalecky thanks for the feedback!
see #6382 for methods/properties that I think should be moved/changed to the OpsMin (some more tricky than others). Anything else pls comment.
@jreback When I include tz series and index, I get ValueError: Cannot compare tz-naive and tz-aware timestamps during min and max tests. Maybe Timestamp compares checking tzinfo property which datetime64 doesn't have?
can you post the comparion? (e.g. you need to have the test use a Timestamp with a tz otherwise you SHOULD get that error).
Sorry, I'm reffering to existing TestIndexOps.test_ops (L189). Tests for test_value_counts_unique_nunique can be passed including tz. I pushed my current code.
Is it OK for me to do:
- Inculde tz-related
SeriesandIndextoself.obj. - Modify 'TestIndexOps.test_ops' to exclude tz-related
SeriesandIndexfrom the arguments.
These should all work (which is what the structure tests)
I think you need to use keep_tz when you create the series (so that the tz are kept!)
In [14]: i = tm.makeDateIndex(10).tz_localize(tz='US/Eastern')
In [15]: i
Out[15]:
<class 'pandas.tseries.index.DatetimeIndex'>
[2000-01-03 00:00:00-05:00, ..., 2000-01-14 00:00:00-05:00]
Length: 10, Freq: B, Timezone: US/Eastern
In [16]: i.to_series(keep_tz=True).value_counts()
Out[16]:
2000-01-05 00:00:00-05:00 1
2000-01-11 00:00:00-05:00 1
2000-01-14 00:00:00-05:00 1
2000-01-04 00:00:00-05:00 1
2000-01-07 00:00:00-05:00 1
2000-01-10 00:00:00-05:00 1
2000-01-13 00:00:00-05:00 1
2000-01-03 00:00:00-05:00 1
2000-01-06 00:00:00-05:00 1
2000-01-12 00:00:00-05:00 1
dtype: int64
In [17]: i.to_series(keep_tz=True).max()
Out[17]: Timestamp('2000-01-14 00:00:00-0500', tz='US/Eastern', offset='B')
Thanks. I feel I could understand the problem now, the error isn't caused from min or max itselves, but by comparison done in AssertEqual
>>> i = tm.makeDateIndex(10).tz_localize(tz='US/Eastern')
# What TestIndexOps.test_ops does:
>>> i.max() == i.values.max()
ValueError: Cannot compare tz-naive and tz-aware timestamps
>>> i.values.max() == i.max()
False
>>> i.values.max().tolist() == i.max().value
True
Changing the test logic seems to be a workaround, but Timestamp class should be changed to allow the comparison with datetime64? If I still misunderstood, please let me know.
you can't really compare Timestamp WITH a tz with a np.datetime64 which does not have a tz (I know it shows that it does, but that's just a local representation), their is a proposal to 'fix' this (see http://mail.scipy.org/pipermail/numpy-discussion/2014-March/069282.html thread).
These SHOULD compare equal though (but because of the tz are not).
In [26]: np.datetime64('2001-01-01 00:00:00').astype('M8[ns]').astype('int64')
Out[26]: 978325200000000000
In [29]: x = Timestamp('20010101').tz_localize('EST')
In [34]: x.asm8
Out[34]: numpy.datetime64('2001-01-01T00:00:00.000000000-0500')
In [35]: x.value
Out[35]: 978325200000000000
for now. just text those cases explicity
Thanks! I've added error handling, and rebased onto latest master.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how much of these tests did you change? I don't like ANY tests where you have to use equalContets/assertEqual
I get for a numpy arrange out put that is ok (e.g. unique). but if the output is a Series (e.g. with value_counts), I don't think that should used at all. I know this is waht the original test did, but value_counts here is a series.) Can you go thru tests and change as much as possible to assert_series_equal?
In the cases of a numpy array is returned use self.assert_numpy_array_equal. The problem with checking contents and equal is it can mask issues with the ordering and/or return types.
Understood. I've modified tests to use series_equal for value_counts and numpy_array_equal for unique.
And is the current value_counts behavior ok to include NaT?
You mean like this? yes NaT should be included
In [64]: x = pd.DatetimeIndex(date_range('20130101',periods=10).take(np.random.randint(0,10,size=100)).tolist() + [pd.NaT,pd.NaT])
In [65]: x
Out[65]:
<class 'pandas.tseries.index.DatetimeIndex'>
[2013-01-10, ..., NaT]
Length: 102, Freq: None, Timezone: None
In [66]: x.to_series().value_counts()
Out[66]:
2013-01-06 16
2013-01-05 13
2013-01-08 11
2013-01-07 11
2013-01-10 11
2013-01-01 9
2013-01-03 9
2013-01-09 7
2013-01-02 7
2013-01-04 6
NaT 2
dtype: int64
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is fine, though array_equavalent will match this (as it uses pd.isnull() under the hood which handles NaT properly). even np.array_equal works here. The actual pd.NaT values are translated to pd.tslib.iNaT which is actually an integer; this type of testing is even easier that floats FYI.
jreback added a commit that referenced this pull request
ENH: added nunique function to Index
thanks @sinhrks
lots of nice PR's from you!
keep em coming
FYI my comments about the NaT testing were really just for your information...this PR is good!
Thank you for your review & info. I remember.
I owe much to pandas and hope to assist a little!
no thank you for the contributions!
keep em coming
Labels
Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff
API Design Enhancement Indexing
Related to indexing on series/frames, not to indexes themselves