BUG: Series[int].loc setitem with Series[int] results in Series[float] #41644
Conversation
simonjayhawkins
added
Indexing
| @@ -158,7 +158,7 @@ def test_setitem_series_object_dtype(self, indexer, ser_index): | |||
| expected = Series([Series([42], index=[ser_index]), 0], dtype="object") | |||
| tm.assert_series_equal(ser, expected) | |||
|
|
|||
| @pytest.mark.parametrize("index, exp_value", [(0, 42.0), (1, np.nan)]) | |||
| @pytest.mark.parametrize("index, exp_value", [(0, 42), (1, np.nan)]) | |||
| def test_setitem_series(self, index, exp_value): | |||
There was a problem hiding this comment.
this seems weird. wouldnt we expect an object-dtype Series with ser.loc[0] itself being a Series?
There was a problem hiding this comment.
that's unrelated to the changes here. so will not be changing the indexing code in this PR. This PR is a followon to #40555 to change can_hold_element, which I'm uncomfortable doing on 1.2.x since we may not have another release after the next one.
There was a problem hiding this comment.
OK. cc @phofl thoughts on desired behavior?
There was a problem hiding this comment.
I think the behavior after this pr is correct.
ser.loc[0] = Series([42], index=[0])
This assigns 42 to the first element in the Series, so would not expect a type cast here
There was a problem hiding this comment.
so you expect to get the same result from ser.loc[0] = ser2 as you would from ser.loc[[0]] = ser2?
There was a problem hiding this comment.
did this resolve? (is this a separate patch / issue s well)?
There was a problem hiding this comment.
we need a separate issue/patch if any other changes. @jbrockmendel @phofl
if another patch is done, this PR is not urgent as it becomes just a refactor.
if another patch is not done, we should merge this before 1.3.
There was a problem hiding this comment.
@jbrockmendel @phofl is there a plan to change the return type. otherwise we should probably merge this for 1.3
waiting and we would need a release note and decide on backport (which i'd be -1 on, for the same reason I didn't change can_hold_element in 1.2.5)
There was a problem hiding this comment.
Lgtm too. Even if we agree that we should return something else, the int result is more consistent than the float result
There was a problem hiding this comment.
The correct-behavior question may be related to #16864
|
milestoned as 1.3, but if the indexing is to be changed in another PR this becomes less urgent. |
|
let's move off the milestone |
|
No what's new needed. Only changes master.
…On Fri, 4 Jun 2021, 21:29 Jeff Reback, ***@***.***> wrote:
***@***.**** requested changes on this pull request.
can you add whatsnew
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#41644 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADEMUXI7MC47NEW25ZF47KLTREZRTANCNFSM45NOJ7QQ>
.
|
|
LGTM |
|
@jreback OK to merge? |
|
yep |
Does not need a release note, this issue is only on master as on 1.2.4 this raises
ValueError: No axis named 1 for object type Serieswhich was fixed in #39358draft, since if we merge and backport #40555 first, we can also remove the patch in this PR.