Skip to content
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

removed compat check for Series.items method #13920

Closed
wants to merge 1 commit into from

Conversation

lewisacidic
Copy link
Contributor

@lewisacidic lewisacidic commented Aug 5, 2016

Should I add something in the whatsnew?
I couldn't find a relevant test, so this is to check if this breaks any tests in CI.

@lewisacidic lewisacidic changed the title removed compat check for items method removed compat check for Series.items method Aug 5, 2016
@jreback
Copy link
Contributor

jreback commented Aug 5, 2016

can you add a test for this (see where Series.iteritems is test)

@jreback
Copy link
Contributor

jreback commented Aug 5, 2016

yes would need a release note

@lewisacidic
Copy link
Contributor Author

I wasn't sure how was best to test this (still very new to pandas development) - is this OK?

@jreback
Copy link
Contributor

jreback commented Aug 5, 2016

ok this looks reasonable, ping when green.

@@ -202,3 +202,4 @@ Bug Fixes
- Bug in ``DataFrame.to_sparse()`` loses column names for MultiIndexes (:issue:`11600`)
- Bug in ``DataFrame.round()`` with non-unique column index producing a Fatal Python error (:issue:`11611`)
- Bug in ``DataFrame.round()`` with ``decimals`` being a non-unique indexed Series producing extra columns (:issue:`11618`)
- Fixed PY2/PY3 incompatability in ``Series.items`` (:issue:`13918`)
Copy link
Contributor

Choose a reason for hiding this comment

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

put this in the API changes section as that will gain more attention. Say Series has gained the .items() method to mirror .iteritems() and for compat with PY3 (and add for DataFrame) as well.

@shoyer
Copy link
Member

shoyer commented Aug 5, 2016

It would be nice to do the same for DataFrame.items() in this PR if you have time

@lewisacidic
Copy link
Contributor Author

Oh yeah, sure I can add DataFrame.items() in this PR - might take a little while as I am nearing 48 hours without sleep now (you can probably tell...), and bed is calling me.

@codecov-io
Copy link

codecov-io commented Aug 6, 2016

Current coverage is 85.26% (diff: 100%)

No coverage report found for master at 1919e26.

Powered by Codecov. Last update 1919e26...d6d93ba

@jorisvandenbossche
Copy link
Member

@richlewis42 Can you update this?

@lewisacidic
Copy link
Contributor Author

Ahh sorry completely fell off my todo list. First thing tomorrow, I will finish this.

@lewisacidic
Copy link
Contributor Author

@jreback This looks ready now. Is everything good?

@@ -241,6 +241,11 @@ def test_iteritems(self):
for k, v in compat.iteritems(df):
self.assertEqual(type(v), Series)

def test_items(self):
df = DataFrame([[1, 2, 3], [4, 5, 6]], columns=['a', 'a', 'b'])
Copy link
Contributor

@jreback jreback Aug 18, 2016

Choose a reason for hiding this comment

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

add the issue number here as a comment

@jreback jreback added this to the 0.19.0 milestone Aug 18, 2016
@jreback
Copy link
Contributor

jreback commented Aug 18, 2016

minorr change. lgtm.

@shoyer
Copy link
Member

shoyer commented Aug 18, 2016

I am concerned that .items() does not return the expected result on Python 2. It would seem more consistent to write a list of items on Python 2 rather than an iterator.

@lewisacidic
Copy link
Contributor Author

lewisacidic commented Aug 18, 2016

@jreback I added the comment

@@ -1096,8 +1096,7 @@ def iteritems(self):
"""
return zip(iter(self.index), iter(self))

if compat.PY3: # pragma: no cover
items = iteritems
Copy link
Contributor

Choose a reason for hiding this comment

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

I think @shoyer is right here,; I think this will work if you do

items=compat.iteritems which already has the correct PY2 (and 3) behavior

Copy link
Member

Choose a reason for hiding this comment

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

I think compa.iteritems just calls the object's iteritems method, so that would not make a difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think @jorisvandenbossche is right - see my new comment.

@lewisacidic
Copy link
Contributor Author

@shoyer I think you are right, there is a PY2/PY3 inconsistency introduced here, and this requires more thought (sorry for leaving this around for so long, been very busy at work).

How I see this:

For dictionaries:

method PY2 PY3
items not lazy lazy
iteritems lazy not implemented

Currently in pandas for Series and DataFrames:

method PY2 PY3
items not implemented lazy
iteritems lazy lazy

In my opinion, PY3's compat.iteritems should be deprecated (or just left as is), and we should implement a compat.items which is not lazy for PY2 but is for PY3. Is that a better solution? @jreback @jorisvandenbossche what do you think?


Python 2

>>> d = dict(a=4, b=6)

>>> d.items()
[('a', 4), ('b', 6)]

>>> d.iteritems()
<dictionary-itemiterator at 0x10bedf310>

>>> s = pd.Series(d)

>>> s.items()
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-5-615b57d0112e> in <module>()
----> 1 s.items()

/Users/rich/anaconda/envs/python2/lib/python2.7/site-packages/pandas/core/generic.pyc in __getattr__(self, name)
   2239                 or name in self._metadata
   2240                 or name in self._accessors):
-> 2241             return object.__getattribute__(self, name)
   2242         else:
   2243             if name in self._info_axis:

AttributeError: 'Series' object has no attribute 'items'

>>> s.iteritems()
<itertools.izip at 0x10bfec488>

>>> df = s.reset_index()

>>> df.items()
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-8-952a7e29c4b1> in <module>()
----> 1 df.items()

/Users/rich/anaconda/envs/python2/lib/python2.7/site-packages/pandas/core/generic.pyc in __getattr__(self, name)
   2239                 or name in self._metadata
   2240                 or name in self._accessors):
-> 2241             return object.__getattribute__(self, name)
   2242         else:
   2243             if name in self._info_axis:

AttributeError: 'DataFrame' object has no attribute 'items'

>>> df.iteritems()
<generator object iteritems at 0x10bfe96e0>

Python 3

>>> d = dict(a=4, b=6)

>>> d.items()
dict_items([('b', 6), ('a', 4)])

>>> d.iteritems()
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-3-6c1b1c343924> in <module>()
----> 1 d.iteritems()

AttributeError: 'dict' object has no attribute 'iteritems'

>>> s = pd.Series(d)

>>> s.items()
<zip at 0x10686e388>

>>> s.iteritems()
<zip at 0x11e05c788>

>>> df = s.reset_index()

>>> df.items()
<generator object DataFrame.iteritems at 0x11deeb0f8>

>>> df.iteritems()
<generator object DataFrame.iteritems at 0x11dea2888>

@jreback
Copy link
Contributor

jreback commented Aug 26, 2016

@richlewis42 compat.items sounds fine to me. It is possible this already exists in six and was never included in our impl (which is included inline in compat/__init__.py).

@jreback jreback removed this from the 0.19.0 milestone Aug 26, 2016
@jreback
Copy link
Contributor

jreback commented Sep 9, 2016

can you rebase / update

@jreback
Copy link
Contributor

jreback commented Nov 16, 2016

can you update

@jreback
Copy link
Contributor

jreback commented Dec 21, 2016

@richlewis42 can you rebase / update.

I think we do need to make a non-lazy PY2 .items() for compat. (maybe something exists in pandas.compat for this? or in six which we can bring over.

@lewisacidic
Copy link
Contributor Author

Hi @jreback sorry my bad somehow wasn't getting these updates. I'll have a look as soon as I can, I'm but I'm afraid I'm writing up my PhD at the moment so really busy.

@jreback
Copy link
Contributor

jreback commented Feb 27, 2017

closing as stale. please ping if you'd like to continue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pandas.items in Python2 vs Python3
5 participants