ENH: added nunique function to Index by sinhrks · Pull Request #6734 · pandas-dev/pandas (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation24 Commits1 Checks0 Files changed

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 Series and Index to self.obj.
• Modify 'TestIndexOps.test_ops' to exclude tz-related Series and Index from 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

Apr 6, 2014

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

Algos

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