Repository navigation
BUG: fix "boolean value of NA is ambiguous" in iloc setitem with nullable Series RHS - #68312
IMGillusion wants to merge 1 commit into
Conversation
| # is a Series with a non-default index, ``original[idx]`` would do | ||
| # a label lookup and can land on a missing element even when | ||
| # ``values[idx]`` is concrete (GH#62473). Access positionally. | ||
| original_idx = getattr(original, "iloc", original)[idx] |
There was a problem hiding this comment.
instead of getattr, can you use a more explicit isinstance
| else: | ||
| idx = np.nanargmax(values) | ||
| if int(values[idx]) != original[idx]: | ||
| # `values` is positionally aligned with the input; when the input |
There was a problem hiding this comment.
Please ask your AI to make comments "maximally concise"
|
Pushed 84c6bc5 to address both comments: the getattr is replaced with an explicit |
| else: | ||
| original_idx = original[idx] | ||
| if ( | ||
| not libmissing.checknull(original_idx) |
There was a problem hiding this comment.
dont we know we have floats here, so we can just use np.isnan?
There was a problem hiding this comment.
actually, is this even reachable?
| - Bug in setitem (e.g. :meth:`Series.iloc`) silently storing wrong values when assigning a nullable or :class:`ArrowDtype` extension array into a NumPy-dtype column that could not hold it losslessly, such as a negative value into an unsigned-integer column (``-1`` became ``18446744073709551615``) or a value overflowing a narrower float dtype (:issue:`47776`) | ||
| - Bug in setitem on nullable masked arrays (``Int64``, ``Float64``, ``boolean``, ...) silently coercing list-like values that the equivalent scalar setitem would reject — e.g. ``arr[[0]] = ['1']`` parsed the string into the numeric dtype, ``arr[[0]] = [True]`` on ``Int64``/``Float64`` coerced the boolean to ``1``, and an out-of-bounds value such as ``arr[[0]] = [1000]`` on ``Int8`` silently wrapped instead of raising (:issue:`45404`) | ||
| - Fixed bug in :meth:`DataFrame.loc` where assigning an iterable to a single cell in an ``object`` dtype column incorrectly raised a ``ValueError`` (:issue:`26333`, :issue:`57962`) | ||
| - Bug in :meth:`Series.iloc` setitem raising ``TypeError`` ("boolean value of NA is ambiguous") when the right-hand side is a nullable :class:`Series` (e.g. ``Int64``) containing a missing value with a non-default index, because the coercion compared the value positionally against the original by label (:issue:`62473`) |
There was a problem hiding this comment.
i suspect this also addresses cases without a missing value, e.g.
ser = pd.Series([1, 2], dtype="Int64")
ser.iloc[[0, 1]] = pd.Series([1.0, 2.0], index=["a", "b"])
?
There was a problem hiding this comment.
if so a test would be worthwhile too
|
Addressed in ed45f9e. To answer the second question first: the |
|
Yes, confirmed — and added the test. |
|
This pull request has had no activity from its author for 14 days, so I've marked it stale. If you've already addressed the feedback, re-request a review (the ↻ next to the reviewer) to move it back into the review queue. If you're still working on it, push a commit, reply to a review comment, or leave a comment — here or on the linked issue — and the Note: labels update via a once-a-day job, so |
|
Thanks for the heads-up. The review feedback is fully addressed (84c6bc5: explicit isinstance check; ed45f9e: removed the unreachable checknull guard, added the no-NA regression test, broadened the whatsnew entry; 8502428: ruff format fix). CI is green on the current head. A re-review would be appreciated when there is time. |
|
This pull request has had no activity from its author for 14 days, so I've marked it stale. If you've already addressed the feedback, re-request a review (the ↻ next to the reviewer) to move it back into the review queue. If you're still working on it, push a commit, reply to a review comment, or leave a comment — here or on the linked issue — and the Note: labels update via a once-a-day job, so |
|
Pinging again ahead of the stale auto-close. All of jbrockmendel's review feedback from 09-14 and 09-28 is addressed: isinstance check instead of getattr, the unreachable checknull guard removed, a no-NA regression test added, the whatsnew entry broadened to cover both the NA (TypeError) and no-NA (KeyError) shapes, and the follow-up ruff-format fix (head 8502428). CI is fully green on the current head. A re-review would be appreciated when there is time. |
|
|
||
| def test_setitem_iloc_rhs_series_non_default_index_with_na(self): | ||
| # GH#62473 | ||
| # Assigning a nullable Int64 Series with an NA as the RHS of an iloc |
There was a problem hiding this comment.
can this comment be any more concise?
| - Bug in setitem (e.g. :meth:`Series.iloc`) silently storing wrong values when assigning a nullable or :class:`ArrowDtype` extension array into a NumPy-dtype column that could not hold it losslessly, such as a negative value into an unsigned-integer column (``-1`` became ``18446744073709551615``) or a value overflowing a narrower float dtype (:issue:`47776`) | ||
| - Bug in setitem on nullable masked arrays (``Int64``, ``Float64``, ``boolean``, ...) silently coercing list-like values that the equivalent scalar setitem would reject — e.g. ``arr[[0]] = ['1']`` parsed the string into the numeric dtype, ``arr[[0]] = [True]`` on ``Int64``/``Float64`` coerced the boolean to ``1``, and an out-of-bounds value such as ``arr[[0]] = [1000]`` on ``Int8`` silently wrapped instead of raising (:issue:`45404`) | ||
| - Fixed bug in :meth:`DataFrame.loc` where assigning an iterable to a single cell in an ``object`` dtype column incorrectly raised a ``ValueError`` (:issue:`26333`, :issue:`57962`) | ||
| - Bug in :meth:`Series.iloc` setitem when the right-hand side is a nullable :class:`Series` (e.g. ``Int64``) with a non-default index, raising ``TypeError`` ("boolean value of NA is ambiguous") when the right-hand side contains a missing value or ``KeyError`` otherwise, because the coercion looked the assigned values up in the original by label instead of by position (:issue:`62473`) |
There was a problem hiding this comment.
needs to move to 3.2, also avoid discussing implementation details
| # by label, which raised (or misfired) for a non-default index. | ||
| ser = pd.Series([1, 2], dtype="Int64") | ||
| result = ser.copy() | ||
| result.iloc[[0, 1]] = pd.Series([1.0, 2.0], index=["a", "b"]) |
There was a problem hiding this comment.
i think this still passes even if the assignment totally failed? could we set values that dont happen to already match whats there?
…able Series RHS (GH#62473)
1ade738 to
9ee9677
Compare
xref: #62473
Issue and Reproducible Example
Assigning a nullable (e.g.
Int64)Seriescontaining a missing value asthe right-hand side of a
Series.ilocsetitem raisedwhen the RHS had a non-default index:
Root Cause
In
_coerce_to_data_and_mask(pandas/core/arrays/numeric.py), theprecision-loss check compares the positionally-coerced float value against the
original:
valuesis positionally aligned with the input, but when the input is aSerieswith a non-default index,original[idx]performs a label lookupand can land on a missing (NA) element even when the value at position
idxis concrete. Comparing a concrete value against NA yields NA, and the
ifthen raises "boolean value of NA is ambiguous".
Fix
Access the original value positionally (a
Seriesis read via.iloc;arrays and lists are already read positionally) and skip the check when the
original value is null.
np.nanargmaxalready guaranteesvalues[idx]isconcrete, so the positional original is not the missing element in practice; the
null guard is defensive.
Tests
test_setitem_iloc_rhs_series_non_default_index_with_na(new, inpandas/tests/series/indexing/test_setitem.py) reproduces the issue andasserts the outcome is positional: a non-default-indexed RHS yields the same
result as the equivalent default-indexed RHS.
This contribution was developed with the assistance of an AI coding assistant.