Skip to content

ENH: added nunique function to Index #6734

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

Merged
merged 1 commit into from
Apr 6, 2014
Merged

Conversation

sinhrks
Copy link
Member

@sinhrks sinhrks commented Mar 29, 2014

Added nunique function to Index same as Series.

@jreback
Copy link
Contributor

jreback commented Mar 29, 2014

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?

@sinhrks
Copy link
Member Author

sinhrks commented Mar 29, 2014

Sure. I'll try it.

Actually tests are derived from test_series.py. I'll prepare more tests for time-related indexes.

# NAs in object arrays #714
i = np.array(['foo'] * 100)
i[::2] = np.nan
idx = Index(i, dtype='O')
Copy link
Contributor

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).

@sinhrks
Copy link
Member Author

sinhrks commented Mar 31, 2014

I've moved value_counts, unique and nunique to the base class, and added more tests. Could you review this?

@jreback
Copy link
Contributor

jreback commented Mar 31, 2014

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

@jreback
Copy link
Contributor

jreback commented Mar 31, 2014

also for some reason travis is not picking this up...can you rebase and force push?

@sinhrks
Copy link
Member Author

sinhrks commented Mar 31, 2014

Rebased, and I'll take care.

Test has passed.

@jreback
Copy link
Contributor

jreback commented Mar 31, 2014

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).

@nehalecky
Copy link
Contributor

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.

@jreback
Copy link
Contributor

jreback commented Apr 1, 2014

@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.

@sinhrks
Copy link
Member Author

sinhrks commented Apr 1, 2014

@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?

@jreback
Copy link
Contributor

jreback commented Apr 1, 2014

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).

@sinhrks
Copy link
Member Author

sinhrks commented Apr 1, 2014

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.

@jreback
Copy link
Contributor

jreback commented Apr 1, 2014

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')

@sinhrks
Copy link
Member Author

sinhrks commented Apr 1, 2014

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.

@jreback
Copy link
Contributor

jreback commented Apr 1, 2014

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

@sinhrks
Copy link
Member Author

sinhrks commented Apr 2, 2014

Thanks! I've added error handling, and rebased onto latest master.

expected = np.array(pd.to_datetime(['2010-01-01 00:00:00',
'2009-01-01 00:00:00',
'2008-09-09 00:00:00']))
self.assertEqual(result.index.dtype, 'datetime64[ns]')
Copy link
Contributor

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.

@sinhrks
Copy link
Member Author

sinhrks commented Apr 6, 2014

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?

@jreback
Copy link
Contributor

jreback commented Apr 6, 2014

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

self.assertEqual(unique.dtype, 'datetime64[ns]')
# numpy_array_equal cannot compare pd.NaT
self.assert_numpy_array_equal(unique[:3], expected)
self.assertTrue(unique[3] is pd.NaT or unique[3].astype('int64') == pd.tslib.iNaT)
Copy link
Contributor

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
@jreback jreback merged commit 657d255 into pandas-dev:master Apr 6, 2014
@jreback
Copy link
Contributor

jreback commented Apr 6, 2014

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!

@sinhrks sinhrks deleted the ind_nunique branch April 7, 2014 11:59
@sinhrks
Copy link
Member Author

sinhrks commented Apr 7, 2014

Thank you for your review & info. I remember.

I owe much to pandas and hope to assist a little!

@jreback
Copy link
Contributor

jreback commented Apr 7, 2014

no thank you for the contributions!

keep em coming

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants