Skip to content

CLN: trim unreachable indexing code #31768

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

Merged
merged 7 commits into from
Feb 9, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 2 additions & 10 deletions pandas/core/frame.py
Original file line number Diff line number Diff line change
Expand Up @@ -3011,17 +3011,12 @@ def _set_value(self, index, col, value, takeable: bool = False):
col : column label
value : scalar
takeable : interpret the index/col as indexers, default False

Returns
-------
DataFrame
If label pair is contained, will be reference to calling DataFrame,
otherwise a new object.
"""
try:
if takeable is True:
series = self._iget_item_cache(col)
return series._set_value(index, value, takeable=True)
series._set_value(index, value, takeable=True)
return

series = self._get_item_cache(col)
engine = self.index._engine
Expand All @@ -3031,7 +3026,6 @@ def _set_value(self, index, col, value, takeable: bool = False):
series._values[loc] = value
# Note: trying to use series._set_value breaks tests in
# tests.frame.indexing.test_indexing and tests.indexing.test_partial
return self
except (KeyError, TypeError):
# set using a non-recursive method & reset the cache
if takeable:
Expand All @@ -3040,8 +3034,6 @@ def _set_value(self, index, col, value, takeable: bool = False):
self.loc[index, col] = value
self._item_cache.pop(col, None)

return self

def _ensure_valid_index(self, value):
"""
Ensure that if we don't have an index, that we can create one from the
Expand Down
17 changes: 2 additions & 15 deletions pandas/core/series.py
Original file line number Diff line number Diff line change
Expand Up @@ -902,7 +902,7 @@ def _get_with(self, key):
return self._get_values(key)
raise

if not isinstance(key, (list, np.ndarray, Series, Index)):
if not isinstance(key, (list, np.ndarray, ExtensionArray, Series, Index)):
Copy link
Contributor

Choose a reason for hiding this comment

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

we likely don't have many tests which do this on EAs FYI

key = list(key)

if isinstance(key, Index):
Expand Down Expand Up @@ -987,8 +987,6 @@ def __setitem__(self, key, value):

try:
self._set_with_engine(key, value)
except com.SettingWithCopyError:
raise
except (KeyError, ValueError):
values = self._values
if is_integer(key) and not self.index.inferred_type == "integer":
Expand All @@ -997,9 +995,6 @@ def __setitem__(self, key, value):
self[:] = value
else:
self.loc[key] = value
except InvalidIndexError:
# e.g. slice
self._set_with(key, value)

except TypeError as e:
if isinstance(key, tuple) and not isinstance(self.index, MultiIndex):
Expand Down Expand Up @@ -1070,7 +1065,7 @@ def _set_with(self, key, value):

def _set_labels(self, key, value):
key = com.asarray_tuplesafe(key)
indexer = self.index.get_indexer(key)
indexer: np.ndarray = self.index.get_indexer(key)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't need to annotate like this; if anything should set the return of get_indexer to np.ndarray

Copy link
Member Author

Choose a reason for hiding this comment

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

yah, im trying to pin down what gets passed to _set_values below. The check on L1075 is never True, so id like to see if we can rip it out

mask = indexer == -1
if mask.any():
raise ValueError(f"{key[mask]} not contained in the index")
Expand All @@ -1096,12 +1091,6 @@ def _set_value(self, label, value, takeable: bool = False):
value : object
Scalar value.
takeable : interpret the index as indexers, default False

Returns
-------
Series
If label is contained, will be reference to calling Series,
otherwise a new object.
"""
try:
if takeable:
Expand All @@ -1115,8 +1104,6 @@ def _set_value(self, label, value, takeable: bool = False):
# set using a non-recursive method
self.loc[label] = value

return self

# ----------------------------------------------------------------------
# Unsorted

Expand Down
23 changes: 11 additions & 12 deletions pandas/tests/frame/indexing/test_indexing.py
Original file line number Diff line number Diff line change
Expand Up @@ -1369,29 +1369,28 @@ def test_set_value(self, float_frame):

def test_set_value_resize(self, float_frame):

res = float_frame._set_value("foobar", "B", 0)
assert res is float_frame
assert res.index[-1] == "foobar"
assert res._get_value("foobar", "B") == 0
float_frame._set_value("foobar", "B", 0)
assert float_frame.index[-1] == "foobar"
Copy link
Contributor

Choose a reason for hiding this comment

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

you could assert the return value is None

assert float_frame._get_value("foobar", "B") == 0

float_frame.loc["foobar", "qux"] = 0
assert float_frame._get_value("foobar", "qux") == 0

res = float_frame.copy()
res3 = res._set_value("foobar", "baz", "sam")
assert res3["baz"].dtype == np.object_
res._set_value("foobar", "baz", "sam")
assert res["baz"].dtype == np.object_

res = float_frame.copy()
res3 = res._set_value("foobar", "baz", True)
assert res3["baz"].dtype == np.object_
res._set_value("foobar", "baz", True)
assert res["baz"].dtype == np.object_

res = float_frame.copy()
res3 = res._set_value("foobar", "baz", 5)
assert is_float_dtype(res3["baz"])
assert isna(res3["baz"].drop(["foobar"])).all()
res._set_value("foobar", "baz", 5)
assert is_float_dtype(res["baz"])
assert isna(res["baz"].drop(["foobar"])).all()
msg = "could not convert string to float: 'sam'"
with pytest.raises(ValueError, match=msg):
res3._set_value("foobar", "baz", "sam")
res._set_value("foobar", "baz", "sam")

def test_set_value_with_index_dtype_change(self):
df_orig = DataFrame(np.random.randn(3, 3), index=range(3), columns=list("ABC"))
Expand Down
12 changes: 4 additions & 8 deletions pandas/tests/series/indexing/test_datetime.py
Original file line number Diff line number Diff line change
Expand Up @@ -73,17 +73,13 @@ def test_series_set_value():
dates = [datetime(2001, 1, 1), datetime(2001, 1, 2)]
index = DatetimeIndex(dates)

s = Series(dtype=object)._set_value(dates[0], 1.0)
s2 = s._set_value(dates[1], np.nan)
s = Series(dtype=object)
s._set_value(dates[0], 1.0)
s._set_value(dates[1], np.nan)

expected = Series([1.0, np.nan], index=index)

tm.assert_series_equal(s2, expected)

# FIXME: dont leave commented-out
# s = Series(index[:1], index[:1])
# s2 = s._set_value(dates[1], index[1])
# assert s2.values.dtype == 'M8[ns]'
tm.assert_series_equal(s, expected)


@pytest.mark.slow
Expand Down
10 changes: 4 additions & 6 deletions pandas/tests/series/indexing/test_indexing.py
Original file line number Diff line number Diff line change
Expand Up @@ -364,16 +364,14 @@ def test_setitem_dtypes():

def test_set_value(datetime_series, string_series):
idx = datetime_series.index[10]
res = datetime_series._set_value(idx, 0)
assert res is datetime_series
datetime_series._set_value(idx, 0)
assert datetime_series[idx] == 0

# equiv
s = string_series.copy()
res = s._set_value("foobar", 0)
assert res is s
assert res.index[-1] == "foobar"
assert res["foobar"] == 0
s._set_value("foobar", 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, assert return value is None

assert s.index[-1] == "foobar"
assert s["foobar"] == 0

s = string_series.copy()
s.loc["foobar"] = 0
Expand Down