API: map() on Index returns an Index, not array #12798

jrings · 2016-04-05T01:36:20Z

Continued in #14506

closes API: Index.map should return Index rather than array #12766
tests added / passed
passes git diff upstream/master | flake8 --diff
whatsnew entry

jrings · 2016-04-05T01:36:39Z

Not sure about where the whatsnew entry will go?

sinhrks · 2016-04-05T09:35:23Z

pandas/indexes/base.py

@@ -2198,7 +2198,7 @@ def groupby(self, to_groupby):
        return self._groupby(self.values, _values_from_object(to_groupby))

    def map(self, mapper):
-        return self._arrmap(self.values, mapper)
+        return Index(self._arrmap(self.values, mapper))


This impl losts metadata. Pls use _shallow_copy_with_infer.

sinhrks · 2016-04-05T09:37:21Z

Thanks! whatsnew locates on doc/source/whatsnew.

sinhrks · 2016-04-05T09:39:00Z

Also, I think it is an API change, rather than a bug (as original issue is tagged).

jrings · 2016-04-05T22:45:04Z

Thanks @sinhrks for the friendly review! This is my first PR, so it's a good opportunity to get a start!
I made those changes, let me know if this is what you had in mind!

Also, the Travis failures seem to be unrelated?

jreback · 2016-04-05T23:26:32Z

@jrings you need to rebase on master. Things changed a bit today.

jrings · 2016-04-05T23:39:36Z

Ugh rebase doesn't like me. Never has never will. Have rebased, will try to squash if changes are ok? (or cry for help)

jreback · 2016-04-06T01:30:54Z

http://pandas.pydata.org/pandas-docs/stable/contributing.html#contributing-your-changes-to-pandas

git checkout yourbranch
git rebase -i origin/master

then
git push yourremote yourbranch -f

jrings · 2016-04-06T02:08:24Z

Yeah I merged on the master to my master, then to my branch. Will work on fixing it, I finally need to figure this out :D

sinhrks · 2016-04-06T21:21:35Z

ci/requirements-3.5_OSX.run

@@ -1,5 +1,5 @@
 pytz
-numpy
+numpy=1.10.4


Can you revert unnecessary changes?

git blame tells me this comes from @jreback jrings@fded942

Pls only include changes you've made(see other PRs). You need rebase:

git checkout master git pull upstream git checkout <your branch> git rebase -i master

sinhrks · 2016-04-06T21:27:26Z

After properly rebased, pls fix datetimelike map consistently.

https://github.com/pydata/pandas/blob/master/pandas/tseries/base.py#L226

Also, adding more dtype tests (you can find some examples here).

https://github.com/pydata/pandas/blob/master/pandas/tests/indexes/test_base.py#L37

jrings · 2016-04-06T22:05:45Z

Ok, cleaned up the rebase mess, will react to the rest later or tomorrow. Thanks!

jrings · 2016-04-06T22:09:12Z

@sinhrks Oh I see, by different dtypes you meant testing map on the different index flavors, not a type conversion on the Index?

sinhrks · 2016-04-06T22:52:21Z

@jrings thx for the fix. yes i meant different classes.

jrings · 2016-04-07T23:56:57Z

Ok, need some advice. Added almost all the index types successfully (pushed what I have so far), but failed on the categorical:

======================================================================
ERROR: test_map (pandas.tests.indexes.test_base.TestIndex)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/joerg/src/pandas/pandas/tests/indexes/test_base.py", line 1593, in test_map
    testIdx = self.catIndex.map(lambda x: int(x))
  File "/home/joerg/src/pandas/pandas/indexes/base.py", line 2201, in map
    return self._shallow_copy_with_infer(self._arrmap(self.values, mapper))
TypeError: Argument 'index' has incorrect type (expected numpy.ndarray, got Categorical)

Can you give me a pointer to how to fix this? i guess since the .c mapper doesn't cover this, I'd check for a CategoricalIndex and handle it in Python instead? Or, rather, implement a separate map in CategoricalIndex?

sinhrks · 2016-04-08T22:00:40Z

doc/source/whatsnew/v0.18.1.txt

@@ -105,6 +105,10 @@ API changes
 - ``pd.show_versions()`` now includes ``pandas_datareader`` version (:issue:`12740`)
 - Provide a proper ``__name__`` and ``__qualname__`` attributes for generic functions (:issue:`12021`)

+- ``map`` on an ``Index`` now returns an ``Index``, not an array (:issue:`12766`)
+
+- ``map`` on an ``Index`` now returns an ``Index``, not an array (:issue:`12766`)


sinhrks · 2016-04-08T22:02:40Z

The CategoricalIndex error should be handled in #12532. Let me update it.

jrings · 2016-04-08T22:12:57Z

Ok, yeah sorry that print was left over from figuring out the catIndex, and I'll dedupe the whatsnew entry as well. Should I wait for #12532 to be merged then?

jrings · 2016-04-08T22:39:11Z

Also, should I do anything for empty and tuples Index?

sinhrks · 2016-04-18T20:29:45Z

#12532 has been merged, can you update?

jorisvandenbossche · 2016-04-18T21:23:53Z

pandas/indexes/base.py

+        """
+        Apply mapper function to an index
+
+         Parameters


The indentation is one space off here (as well as all lines below)

jrings · 2016-04-23T17:24:16Z

Ok categorical works now! Rebased on master and squashed commits.
@sinhrks

jreback · 2016-04-23T17:50:53Z

pandas/tests/indexes/test_base.py

@@ -1557,6 +1557,41 @@ def test_string_index_repr(self):

                self.assertEqual(coerce(idx), expected)

+    def test_map(self):


put all of this in pandas/tests/index/common.py/Base it will then get tested on each index type. see the structure of other methods. If you need to override it then put it specifically in a pandas/test/index/test_* file.

jrings · 2016-04-29T20:35:03Z

Will work on it this weekend!

jrings · 2016-05-04T22:32:53Z

Sorry for being slow, am traveling a lot at the moment. But one question @jreback, would a very simple function for map be enough, just to test that a map still produces an Index, because then putting it all in the loop of the various indexes would be fine, but if I would try to transfer my more varied tests that would lead to a lot of isinstance for each type that would sort of defeat the purpose - so then I'd distribute them over all the different test classes for each type of index?

jreback · 2016-05-04T22:39:24Z

doc/source/whatsnew/v0.18.1.txt

@@ -149,6 +149,8 @@ API changes
 - Provide a proper ``__name__`` and ``__qualname__`` attributes for generic functions (:issue:`12021`)
 - ``pd.concat(ignore_index=True)`` now uses ``RangeIndex`` as default (:issue:`12695`)

+- ``map`` on an ``Index`` now returns an ``Index``, not an array (:issue:`12766`)


move to 0.18.2

jreback · 2016-05-14T16:40:13Z

can you rebase/update

jreback · 2016-05-31T15:22:03Z

can you rebase/update

jreback · 2016-09-09T22:41:53Z

can you rebase / update?

jreback · 2016-10-06T10:53:58Z

pls rebase / update

[ENH] Using _shallow_copy_with_infer, additional test for type change and attribute conservation TST: Tests for various index types REF: Move to tm.assert_index_equal DOC: docstring for the map function

nateyoder · 2016-10-25T13:09:27Z

If this has stalled I'd be more than happy to take it over as I'd really like to see and #12756 make it into a release soon. Is the easiest way to do this by creating a new PR or can I use this one somehow?

jreback · 2016-10-25T13:59:54Z

create a new PR. you can incorporate this commit (if you'd like). you can add a closes (this PR number) as well as the issue number.

jorisvandenbossche · 2016-11-07T20:54:33Z

Continued in #14506

closes #12766 closes #12798 This is a follow on to #12798. Author: Nate Yoder <nate@whistle.com> Closes #14506 from nateyoder/index_map_index and squashes the following commits: 95e4440 [Nate Yoder] fix typo and add ref tag in whatsnew b36e83c [Nate Yoder] update whatsnew, fix documentation 4635e6a [Nate Yoder] compare as index a17ddab [Nate Yoder] Fix unused import and docstrings per pep8radius docformatter; change other uses of assert_index_equal to testing instead os self ab168e7 [Nate Yoder] Update whatsnew and add git PR to tests to denote changes 504c2a2 [Nate Yoder] Fix tests that weren't run by PyCharm 23c133d [Nate Yoder] Update tests to match dtype int64 07b772a [Nate Yoder] use the numpy results if we can to avoid repeating the computation just to create the object a110be9 [Nate Yoder] make map on time tseries indices return index if dtype of output is not a tseries; sphinx changes; fix docstring a596744 [Nate Yoder] introspect results from map so that if the output array has tuples we create a multiindex instead of an index 5fc66c3 [Nate Yoder] make map return an index if it operates on an index, multi index, or categorical index; map on a categorical will either return a categorical or an index (rather than a numpy array)

closes pandas-dev#12766 closes pandas-dev#12798 This is a follow on to pandas-dev#12798. Author: Nate Yoder <nate@whistle.com> Closes pandas-dev#14506 from nateyoder/index_map_index and squashes the following commits: 95e4440 [Nate Yoder] fix typo and add ref tag in whatsnew b36e83c [Nate Yoder] update whatsnew, fix documentation 4635e6a [Nate Yoder] compare as index a17ddab [Nate Yoder] Fix unused import and docstrings per pep8radius docformatter; change other uses of assert_index_equal to testing instead os self ab168e7 [Nate Yoder] Update whatsnew and add git PR to tests to denote changes 504c2a2 [Nate Yoder] Fix tests that weren't run by PyCharm 23c133d [Nate Yoder] Update tests to match dtype int64 07b772a [Nate Yoder] use the numpy results if we can to avoid repeating the computation just to create the object a110be9 [Nate Yoder] make map on time tseries indices return index if dtype of output is not a tseries; sphinx changes; fix docstring a596744 [Nate Yoder] introspect results from map so that if the output array has tuples we create a multiindex instead of an index 5fc66c3 [Nate Yoder] make map return an index if it operates on an index, multi index, or categorical index; map on a categorical will either return a categorical or an index (rather than a numpy array)

jrings changed the title ~~map() on Index returns an Index, not array~~ BUG: map() on Index returns an Index, not array Apr 5, 2016

sinhrks reviewed Apr 5, 2016
View reviewed changes

sinhrks added API Design Compat pandas objects compatability with Numpy or Python functions labels Apr 5, 2016

jrings changed the title ~~BUG: map() on Index returns an Index, not array~~ API: map() on Index returns an Index, not array Apr 5, 2016

sinhrks reviewed Apr 6, 2016
View reviewed changes

jrings force-pushed the index_map_return_index branch 3 times, most recently from 1ce3348 to 678a6b7 Compare April 6, 2016 22:04

sinhrks reviewed Apr 8, 2016
View reviewed changes

sinhrks mentioned this pull request Apr 8, 2016

BUG: Series.map may raise TypeError in Categorical or DatetimeTz #12532

Closed

4 tasks

jorisvandenbossche reviewed Apr 18, 2016
View reviewed changes

jrings force-pushed the index_map_return_index branch 2 times, most recently from fab8205 to 2351ec8 Compare April 23, 2016 17:23

jreback reviewed Apr 23, 2016
View reviewed changes

jreback reviewed May 4, 2016
View reviewed changes

jrings force-pushed the index_map_return_index branch from 2351ec8 to 5c8c57e Compare May 22, 2016 21:12

jrings added 2 commits October 19, 2016 21:40

API: map() on Index returns an Index, not array

5849db5

[ENH] Using _shallow_copy_with_infer, additional test for type change and attribute conservation TST: Tests for various index types REF: Move to tm.assert_index_equal DOC: docstring for the map function

Moving tests

c30c862

jrings force-pushed the index_map_return_index branch from 5c8c57e to c30c862 Compare October 20, 2016 17:15

nateyoder mentioned this pull request Oct 26, 2016

API: map() on Index returns an Index, not array #14506

Closed

5 tasks

jorisvandenbossche added this to the No action milestone Nov 7, 2016

jorisvandenbossche closed this Nov 7, 2016

jreback modified the milestones: 0.20.0, No action Dec 16, 2016

		@@ -1557,6 +1557,41 @@ def test_string_index_repr(self):

		self.assertEqual(coerce(idx), expected)

		def test_map(self):

API: map() on Index returns an Index, not array #12798

API: map() on Index returns an Index, not array #12798

Conversation

jrings commented Apr 5, 2016 • edited by jorisvandenbossche Loading

jrings commented Apr 5, 2016

Choose a reason for hiding this comment

sinhrks commented Apr 5, 2016

sinhrks commented Apr 5, 2016

jrings commented Apr 5, 2016

jreback commented Apr 5, 2016

jrings commented Apr 5, 2016

jreback commented Apr 6, 2016

jrings commented Apr 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sinhrks commented Apr 6, 2016

jrings commented Apr 6, 2016

jrings commented Apr 6, 2016

sinhrks commented Apr 6, 2016

jrings commented Apr 7, 2016

Choose a reason for hiding this comment

sinhrks commented Apr 8, 2016

jrings commented Apr 8, 2016

jrings commented Apr 8, 2016

sinhrks commented Apr 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrings commented Apr 23, 2016

Choose a reason for hiding this comment

jrings commented Apr 29, 2016

jrings commented May 4, 2016

Choose a reason for hiding this comment

jreback commented May 14, 2016

jreback commented May 31, 2016

jreback commented Sep 9, 2016

jreback commented Oct 6, 2016

nateyoder commented Oct 25, 2016

jreback commented Oct 25, 2016

jorisvandenbossche commented Nov 7, 2016

jrings commented Apr 5, 2016 •

edited by jorisvandenbossche

Loading