DEPR: remove ix #27620

jbrockmendel · 2019-07-27T01:45:51Z

Needs release note
Needs geopandas compat
Need to remove ix asvs

WillAyd · 2019-07-27T03:36:52Z

pandas/tests/indexing/common.py

-            filterwarnings("ignore", "\\n.ix", FutureWarning)
-            return f.ix[i]
+
+        # TODO: this used to be f.ix[i]; is loc-then-iloc correct here?


I think should just be .iloc looking at docstring?

that was my first guess too, but that failed

longer term, we should then probably clean up the tests using this get_value

Hmm do you know how many tests this failed? Wonder if its just a problem with the test

WillAyd · 2019-07-27T03:38:44Z

pandas/tests/indexing/test_floats.py

@@ -134,8 +134,7 @@ def test_scalar_non_numeric(self):

                    # these should prob work
                    # and are inconsisten between series/dataframe ATM
-                    # for idxr in [lambda x: x.ix,
-                    #             lambda x: x]:
+                    # for idxr in [lambda x: x]:


I think can just delete this altogether? Looks like it was specific to .ix?

WillAyd · 2019-07-27T03:39:29Z

pandas/tests/indexing/test_iloc.py

@@ -462,71 +462,6 @@ def test_iloc_setitem_dups(self):
        df.iloc[[1, 0], [0, 1]] = df.iloc[[1, 0], [0, 1]].reset_index(drop=True)
        tm.assert_frame_equal(df, expected)

-    def test_iloc_getitem_frame(self):


Do we still have coverage for this? Looks like main intention was .iloc testing so ix was secondary

ill have to do a readthrough to check. its not entirely easy to tell what kind of coverage we have for indexing

WillAyd · 2019-07-27T03:40:23Z

pandas/tests/indexing/test_partial.py

-        # don't allow not string inserts
-        with pytest.raises(TypeError):
-            with catch_warnings(record=True):
-                df.loc[100.0, :] = df.ix[0]


Maybe should also replace the rhs side of these with scalar values

jreback · 2019-07-27T14:06:56Z

pandas/tests/test_downstream.py

@@ -123,6 +123,7 @@ def test_geopandas():
    assert geopandas.read_file(fp) is not None


+@pytest.mark.xfail(reason="ix-only methods have been removed from _NDFrameIndexer")


cc @jorisvandenbossche

@jbrockmendel can you open an issue on geopandas tracker?

Vague plan is to wait for Joris to get back and advise on how to proceed

There is already an open issue for that: geopandas/geopandas#1042

My advice is to keep compatibility with released geopandas for now (see the comment I made above on a specific method of NDFrameIndexer). I know geopandas shouldn't be using this (I opened a PR to clean it up: geopandas/geopandas#1072), but it is, and given we know that, it would be nice to not break directly.

jreback · 2019-07-27T14:07:39Z

@jbrockmendel lgtm. can you PR to geopandas to fix?

jreback · 2019-07-27T14:07:56Z

can you add the issue number at the top

jorisvandenbossche · 2019-07-29T11:17:08Z

pandas/core/indexing.py

-    def __iter__(self):
-        raise NotImplementedError("ix is not iterable")
-
-    def __getitem__(self, key):


Could we keep this method? (or part of it, for compat with released geopandas)

That makes sense. Possibly with a warning telling users they need to upgrade geopandas?

Yep, that sounds good. But maybe start having it as a DeprecationWarning? Then we can easily switch to FutureWarning once there is an actual geopandas release.

jorisvandenbossche · 2019-07-29T11:19:35Z

pandas/tests/indexing/common.py

-            filterwarnings("ignore", "\\n.ix", FutureWarning)
-            return f.ix[i]
+
+        # TODO: this used to be f.ix[i]; is loc-then-iloc correct here?


longer term, we should then probably clean up the tests using this get_value

jorisvandenbossche · 2019-07-29T11:20:36Z

pandas/tests/indexing/test_chaining_and_caching.py

-        df["A"]  # cache series
-        df.ix["Hello Friend"] = df.ix[0]
-        assert "Hello Friend" in df["A"].index
-        assert "Hello Friend" in df["B"].index


Should this be kept but with something else as ix?

Also, need to remove the filterwarnings mark I think

jorisvandenbossche · 2019-07-29T11:23:26Z

pandas/tests/indexing/test_partial.py

-        expected = DataFrame(dict({"A": [0, 2, 4], "B": [0, 2, 4]}))
-        df = df_orig.copy()
-        with catch_warnings(record=True):
-            df.ix[:, "B"] = df.ix[:, "A"]


is this tested for loc ? (otherwise keep test but with loc? same for the ones below)

jorisvandenbossche · 2019-07-29T11:27:02Z

pandas/tests/test_downstream.py

@@ -123,6 +123,7 @@ def test_geopandas():
    assert geopandas.read_file(fp) is not None


+@pytest.mark.xfail(reason="ix-only methods have been removed from _NDFrameIndexer")


There is already an open issue for that: geopandas/geopandas#1042

My advice is to keep compatibility with released geopandas for now (see the comment I made above on a specific method of NDFrameIndexer). I know geopandas shouldn't be using this (I opened a PR to clean it up: geopandas/geopandas#1072), but it is, and given we know that, it would be nice to not break directly.

jorisvandenbossche · 2019-07-29T11:29:25Z

pandas/tests/frame/test_indexing.py

@@ -402,12 +401,6 @@ def test_getitem_ix_mixed_integer(self):
        expected = df.loc[df.index[:-1]]
        assert_frame_equal(result, expected)

-        with catch_warnings(record=True):
-            simplefilter("ignore", FutureWarning)
-            result = df.ix[[1, 10]]


replace with loc? (the above loc seems to be do something slightly different)

jbrockmendel · 2019-08-20T22:54:39Z

closing, will revisit once ix deprecation is enforced

jorisvandenbossche · 2019-08-21T06:58:02Z

What do you mean exactly? I think we want to remove it now?

jbrockmendel · 2019-08-21T14:08:35Z

You're right, it was the NDFrameIndexer.__getitem__ that we're keeping around a little while longer until geopandas stops relying on it. Either way, this is low priority for me at the moment.

jorisvandenbossche · 2019-08-21T14:11:44Z

Either way, this is low priority for me at the moment.

OK, but let's keep the PR open for now. Then it is clearer that there is already a PR to start from in case somebody wants to do the work (and I think we still want this done for 1.0)

WillAyd

Can you merge master?

WillAyd · 2019-08-26T18:10:14Z

pandas/tests/frame/test_indexing.py

@@ -852,55 +802,6 @@ def test_delitem_corner(self, float_frame):
        del f["B"]
        assert len(f.columns) == 2

-    def test_getitem_fancy_2d(self, float_frame):


Are these tests covered elsewhere or just specific to .ix?

Most of these mix loc/iloc behavior, so are specific to ix

jbrockmendel · 2019-11-27T19:46:24Z

rebased

jbrockmendel · 2019-12-01T22:46:33Z

@jorisvandenbossche is the geopandas compat problem resolved? if so, does this just need a whatsnew note?

jbrockmendel · 2019-12-08T19:14:04Z

@jorisvandenbossche gentle ping, we're running out of deprecations to enforce

jreback · 2019-12-08T20:35:16Z

asv_bench/benchmarks/frame_methods.py

-            self.df.ix[50:1000, 20:50] = np.nan
-            self.df.ix[2000:3000] = np.nan
-            self.df.ix[:, 60:70] = np.nan
+            self.df.iloc[50:1000, 20:50] = np.nan


i would remove the warnings catching if its no longer necessary

jreback · 2019-12-08T20:37:36Z

pandas/tests/indexing/common.py

+        # TODO: this used to be f.ix[i]; is loc-then-iloc correct here?
+        try:
+            return f.loc[i]
+        except KeyError:


err, this is not good here. can you separate the cases into ones for .loc and ones for .iloc

jreback · 2019-12-08T20:39:05Z

lgtm generally, 2 items which i think should be done in this PR

jbrockmendel · 2019-12-09T18:07:17Z

updated+green

jreback · 2019-12-10T13:26:08Z

I am ok with merging this, long overdue.

I would say create an issue to make sure that for .loc we have enough tests that absolute expected cases (rather than .ix comparisons).

@pandas-dev/pandas-core any objections.

WillAyd

lgtm. agreed long overdue - thanks for tackling this @jbrockmendel

jorisvandenbossche · 2019-12-12T12:15:21Z

Yes, let's handle further testing clean-up in a follow-up issue.

@jorisvandenbossche is the geopandas compat problem resolved?

It's still in a released version (the same as when we discussed this a few months ago), but it is already fixed in the meantime. I would prefer to keep the compat code here a little bit longer (the geopandas version that fixes it is only recently released)

jorisvandenbossche · 2019-12-12T12:16:08Z

@jbrockmendel thanks a lot for this!

jbrockmendel added 3 commits July 26, 2019 18:08

remove IX

704b422

remove ix-only methods

ec02d1f

lint fixups

db1567f

WillAyd reviewed Jul 27, 2019

View reviewed changes

WillAyd added the Deprecate Functionality to remove in pandas label Jul 27, 2019

jreback reviewed Jul 27, 2019

View reviewed changes

jreback added this to the 1.0 milestone Jul 27, 2019

Merge branch 'master' of https://github.com/pandas-dev/pandas into rmix

c8ff4a0

jorisvandenbossche reviewed Jul 29, 2019

View reviewed changes

jbrockmendel added 2 commits August 17, 2019 10:01

Merge branch 'master' of https://github.com/pandas-dev/pandas into rmix

2c8ef9a

Merge branch 'master' of https://github.com/pandas-dev/pandas into rmix

1ed8a96

jbrockmendel closed this Aug 20, 2019

jorisvandenbossche reopened this Aug 21, 2019

jbrockmendel added 9 commits August 21, 2019 20:31

Merge branch 'master' of https://github.com/pandas-dev/pandas into rmix

8abdbaf

restore getitem needed by geopandas

23d2eca

remove xfail

ecc8b36

Merge branch 'master' of https://github.com/pandas-dev/pandas into rmix

9f78ec7

Merge branch 'master' of https://github.com/pandas-dev/pandas into rmix

3ca8c86

Merge branch 'master' of https://github.com/pandas-dev/pandas into rmix

812b5fb

Merge branch 'master' of https://github.com/pandas-dev/pandas into rmix

2e09bd6

restore and update tests per comments

966f749

remove test_ix

18033b4

WillAyd reviewed Aug 26, 2019

View reviewed changes

Merge branch 'master' of https://github.com/pandas-dev/pandas into rmix

b9f61cb

jbrockmendel added 2 commits November 26, 2019 17:10

Merge branch 'master' of https://github.com/pandas-dev/pandas into rmix

1756a69

Merge branch 'master' of https://github.com/pandas-dev/pandas into rmix

07b393b

jbrockmendel added 5 commits November 27, 2019 13:42

Merge branch 'master' of https://github.com/pandas-dev/pandas into rmix

f9a394a

Merge branch 'master' of https://github.com/pandas-dev/pandas into rmix

3d484cd

Merge branch 'master' of https://github.com/pandas-dev/pandas into rmix

247eabf

remove unused imports

7dc7de6

Merge branch 'master' of https://github.com/pandas-dev/pandas into rmix

2b3dba7

jbrockmendel added 3 commits December 4, 2019 15:12

Merge branch 'master' of https://github.com/pandas-dev/pandas into rmix

721d88b

whatsnew, remove _ix

0ca6762

Merge branch 'master' of https://github.com/pandas-dev/pandas into rmix

446c21f

jreback requested changes Dec 8, 2019

View reviewed changes

jbrockmendel added 2 commits December 8, 2019 18:50

update per comments

a2aef80

Merge branch 'master' of https://github.com/pandas-dev/pandas into rmix

74127d6

jreback approved these changes Dec 10, 2019

View reviewed changes

WillAyd approved these changes Dec 10, 2019

View reviewed changes

mroeschke approved these changes Dec 10, 2019

View reviewed changes

jorisvandenbossche approved these changes Dec 12, 2019

View reviewed changes

jorisvandenbossche merged commit a5c992b into pandas-dev:master Dec 12, 2019

jsexauer mentioned this pull request Dec 12, 2019

DEPR: Clean up list of deprecations from prior versions #6581

Closed

1 task

jreback mentioned this pull request Dec 12, 2019

DEPR: deprecations log for removed issues #13777

Closed

proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019

DEPR: remove ix (pandas-dev#27620)

1f954cd

proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019

DEPR: remove ix (pandas-dev#27620)

cc13a73

		@@ -123,6 +123,7 @@ def test_geopandas():
		assert geopandas.read_file(fp) is not None


		@pytest.mark.xfail(reason="ix-only methods have been removed from _NDFrameIndexer")

DEPR: remove ix #27620

DEPR: remove ix #27620

Conversation

jbrockmendel commented Jul 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Jul 27, 2019

jreback commented Jul 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented Aug 20, 2019

jorisvandenbossche commented Aug 21, 2019

jbrockmendel commented Aug 21, 2019

jorisvandenbossche commented Aug 21, 2019

WillAyd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented Nov 27, 2019

jbrockmendel commented Dec 1, 2019

jbrockmendel commented Dec 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Dec 8, 2019

jbrockmendel commented Dec 9, 2019

jreback commented Dec 10, 2019

WillAyd left a comment

Choose a reason for hiding this comment

jorisvandenbossche commented Dec 12, 2019

jorisvandenbossche commented Dec 12, 2019 • edited Loading

jbrockmendel commented Jul 27, 2019 •

edited

Loading

jorisvandenbossche commented Dec 12, 2019 •

edited

Loading