-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
DEPR: remove .ix from tests/indexing/multiindex/test_setitem.py #27574
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: remove .ix from tests/indexing/multiindex/test_setitem.py #27574
Conversation
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.
Minor stuff but generally looks good. Are any of the items we switched from .ix
too .loc
duplicated in this module? If so removal would be an option as well
compare_fn=tm.assert_frame_equal, | ||
expected=copy, | ||
) | ||
for index_fn in ("loc",): |
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.
Can we just get rid of the loop altogether now?
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.
yes, there will probably be lots of clean-up opportunities which can be done as a follow-up.
the aim is to just remove for now, deprecations are the prioirty for 1.0 (#27492 (comment)), and keeps the diff easier to review..
here, specificically, more pytest idiom would be nice, so a refactor to just to remove the loop could be just going down another rabbit hole. this pattern of a single loop is not unique to this test.
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.
@simonjayhawkins this pattern is fine
exp = Series(arr, index=[8, 10], name="c", dtype="float64") | ||
tm.assert_series_equal(df.ix[4, "c"], exp) | ||
df.loc[4, "c"] = arr | ||
exp = Series(arr, index=[8, 10], name="c", dtype="float64") |
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.
Would generally prefer long form of expected
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.
same. since the .ix tests have probably been around for a while, the style used is probably inconsistent with how we would write today.
@WillAyd Thanks for the review. in these PR's, i'm basically making two choices, remove code or replace with another indexer for now. i'm hoping that this makes the review process easier.. you can just focus on the stuff removed. This could result in some duplication. but would consider that better, in the short term, than removing useful tests. the priority at the moment is getting .ix out of the test suite to allow the deprecation to begin. |
thanks @simonjayhawkins |
No description provided.