-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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
DEPR: deprecate .ix in favor of .loc/.iloc #15113
Conversation
This is the big kahuna! wasn't that difficult, mostly did a search-and-replace of |
very nice! |
Current coverage is 85.53% (diff: 97.36%)@@ master #15113 diff @@
==========================================
Files 145 145
Lines 51352 51361 +9
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
Hits 43932 43932
- Misses 7420 7429 +9
Partials 0 0
|
Or maybe |
Awesome! That's a long message. Is it worth having a shorter message including a link to the docs? |
i changed the message to have a link |
This is great! Deprecation warnings tend to turn up downstream in application code, where they are often read by users for whom they are not relevant. This is a necessary evil, but to reduce their burden it's best to keep them short. I would stick to a few lines with a link, e.g.,
|
I would be in favor for that reason to go with a more 'softer' deprecation, and in a first place use DeprecationWarnings (these should still be visible by default in interactive usage in ipython), and only in a second phase (one or few releases later) a more visible FutureWarning. |
handle iterator handle NamedTuple .loc retuns scalar selection dtypes correctly, closes pandas-dev#11617 xref pandas-dev#15113
handle iterator handle NamedTuple .loc retuns scalar selection dtypes correctly, closes pandas-dev#11617 xref pandas-dev#15113
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! (I didn't check all the ix -> iloc/loc substitutions in detail :-))
We will need to do a clean-up of the docs as well (usage of ix in the docs besides the actual indexing docs)
My main concern is that the alternative presented here in case you want to use iloc
is not fully complete:
In [18]: df = pd.DataFrame(np.random.randn(5,5), columns=list('ABCDE'))
In [19]: df
Out[19]:
A B C D E
0 0.870482 -0.388952 -0.972597 -0.843245 0.903255
1 -0.483238 1.130196 -0.105157 -0.700333 -0.291880
2 -0.541109 -1.916656 1.039409 -0.678030 -0.995090
3 0.375849 0.313649 -0.621017 -1.517242 -0.888986
4 0.144481 0.155721 0.719531 0.959571 2.066996
It works fine as long as you want a single label:
In [21]: df.iloc[0, df.columns.get_loc('A')]
Out[21]: 0.87048249788122523
but not anymore for multiple labels:
In [22]: df.ix[0, ['A', 'B']]
Out[22]:
A 0.870482
B -0.388952
Name: 0, dtype: float64
In [23]: df.iloc[0, df.columns.get_loc(['A', 'B'])]
...
TypeError: '['A', 'B']' is an invalid key
Then you can use get_indexer
:
In [24]: df.iloc[0, df.columns.get_indexer(['A', 'B'])]
Out[24]:
A 0.870482
B -0.388952
Name: 0, dtype: float64
but this does not work on a single value:
In [25]: df.iloc[0, df.columns.get_indexer('A')]
...
TypeError: Index(...) must be called with a collection of some kind, 'A' was passed
No matter that this is the intended behaviour of get_loc
/get_indexer
, it makes it again more complex for the user.
|
||
This deliberate decision was made to prevent ambiguities and subtle bugs (many | ||
users reported finding bugs when the API change was made to stop "falling back" | ||
on position-based indexing). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above section is still valid for []
on a Series, so should not removed I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
.. warning:: | ||
|
||
Startin in 0.20.0, the ``.ix`` indexer is deprecated, in favor of the more strict ``.iloc`` and ``.loc`` indexers. ``.ix`` offers a lot of magic on the inference of what the user wants to do. To wit, ``.ix`` can decide to index *positionally* OR via *labels*. This has caused quite a bit of user confusion over the years. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
startin -> starting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Deprecate .ix | ||
^^^^^^^^^^^^^ | ||
|
||
The ``.ix`` indexer is deprecated, in favor of the more strict ``.iloc`` and ``.loc`` indexers. ``.ix`` offers a lot of magic on the inference of what the user wants to do. To wit, ``.ix`` can decide to index *positionally* OR via *labels*. This has caused quite a bit of user confusion over the years. The full indexing documentation are :ref:`here <indexing>`. (:issue:`14218`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"user confusion" -> not only user confusion :-) I still can't predict what it will do in all cases!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure what you want to change here
self.assertEqual(result, 1) | ||
|
||
# TODO: this doesn't work, should it? | ||
# result = df.loc[IndexType("foo", "bar")]["A"] | ||
# self.assertEqual(result, 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably yes, but seems quite exotic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that you did already handle this in the other PR!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea I have to rebase this.
self.frame.ix[('bar', 'two'), 'B'] = 5 | ||
self.assertEqual(self.frame.ix[('bar', 'two'), 'B'], 5) | ||
self.frame.loc[('bar', 'two'), 'B'] = 5 | ||
self.assertEqual(self.frame.loc[('bar', 'two'), 'B'], 5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we keep some of the ix
tests here for MultiIndexing? (I see you kept non in this file, but maybe MultiIndex is also tested somewhere else?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i added a few back
That's just an alternative, I think it is much more clear to use direct indexing and use thus
|
handle iterator handle NamedTuple .loc retuns scalar selection dtypes correctly, closes #11617 xref #15113 Author: Jeff Reback <jeff@reback.net> Closes #15120 from jreback/indexing and squashes the following commits: 801c8d9 [Jeff Reback] BUG: indexing changes to .loc for compat to .ix for several situations
ok I updated all of the docs that I could as well. |
eaf57e2
to
0bf5cb5
Compare
@jorisvandenbossche do you feel strongly about this being a |
I am personally in favor of using a DeprecationWarning, yes. But would love to hear some other opinions. The reason that I am in favor is because, if we use FutureWarning, many users will see them (from external libraries they use), don't know where they come from or what they can do about it. Of course you specifically silence them, but that is not very friendly to novice users. Using DeprecationWarning now gives package authors some time to clean-up first (of course, assuming they turn on deprecation warnings in their tests. But eg seaborn and statsmodels both have uses of We do this currently like this for the deprecations in |
ok, I will change this to |
Also +1 for I started to work on removing |
handle iterator handle NamedTuple .loc retuns scalar selection dtypes correctly, closes pandas-dev#11617 xref pandas-dev#15113 Author: Jeff Reback <jeff@reback.net> Closes pandas-dev#15120 from jreback/indexing and squashes the following commits: 801c8d9 [Jeff Reback] BUG: indexing changes to .loc for compat to .ix for several situations
closes pandas-dev#14218 closes pandas-dev#15116 Author: Jeff Reback <jeff@reback.net> Closes pandas-dev#15113 from jreback/ix and squashes the following commits: 1544f50 [Jeff Reback] DEPR: deprecate .ix in favor of .loc/.iloc
closes #14218
closes #15116
This shows a pretty big deprecation message, though its instructive.