-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG: Dtype was not changed when float was assignt to int column #37680
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
Changes from 6 commits
eafd9ea
c95dcc9
3788c4d
27d9e35
23cc926
cee9fc2
97e6da8
d1bc870
7e16465
4d91e66
d608474
1d332e8
760c6a1
870e927
fa8b331
903be31
566d769
916faa4
b0699a9
33c8424
6e7ee82
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,6 +42,7 @@ | |
|
||
from pandas.core.dtypes.cast import ( | ||
convert_dtypes, | ||
infer_dtype_from_scalar, | ||
maybe_cast_to_extension_array, | ||
validate_numeric_casting, | ||
) | ||
|
@@ -50,10 +51,12 @@ | |
is_bool, | ||
is_categorical_dtype, | ||
is_dict_like, | ||
is_dtype_equal, | ||
is_extension_array_dtype, | ||
is_integer, | ||
is_iterator, | ||
is_list_like, | ||
is_numeric_dtype, | ||
is_object_dtype, | ||
is_scalar, | ||
validate_all_hashable, | ||
|
@@ -1110,6 +1113,10 @@ def _set_value(self, label, value, takeable: bool = False): | |
else: | ||
loc = self.index.get_loc(label) | ||
validate_numeric_casting(self.dtype, value) | ||
dtype, _ = infer_dtype_from_scalar(value, pandas_dtype=True) | ||
if not is_dtype_equal(self.dtype, dtype) and is_numeric_dtype(dtype): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do you need to check if is numeric here? isn't just not dtype equal enough? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have a test calling this function where scalar is a string value, which is expected to raise an ValueError, hence this check there. If the test is somehow erroneous, we could delete it. https://dev.azure.com/pandas-dev/pandas/_build/results?buildId=47080&view=logs&j=ba13898e-1dfb-5ace-9966-8b7af3677790 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you show the test There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Raising lines are There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you integrate this with the higher level if/else, these nested if/else are harder to read, even if it means duplicating some code There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
self.loc[label] = value | ||
return | ||
self._values[loc] = value | ||
except KeyError: | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,7 @@ | |
import numpy as np | ||
import pytest | ||
|
||
from pandas import DataFrame | ||
from pandas import DataFrame, Series | ||
import pandas._testing as tm | ||
|
||
|
||
|
@@ -39,3 +39,19 @@ def test_at_with_duplicate_axes_requires_scalar_lookup(self): | |
df.at[1, ["A"]] = 1 | ||
with pytest.raises(ValueError, match=msg): | ||
df.at[:, "A"] = 1 | ||
|
||
|
||
def test_at_assign_float_to_int_frame(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you parameterize this on .loc for the same examples There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also testing iat/iloc for the same (obviously using the position). if its becomes messy then can do that in a followup. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also we would want to test this for other dtypes as well. so that is for sure a followon, pls open an issue for this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. all this said, we might have some tests for this already (likely we do), prob have to hunt a bit for them. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Parametrized the tests for loc, iloc and iat would be an ugly parametrization. I will open an issue for dtypes and iloc/iat |
||
# GH: 26395 | ||
obj = DataFrame([0, 0, 0], index=["A", "B", "C"], columns=["D"]) | ||
obj.at["C", "D"] = 44.5 | ||
expected = DataFrame([0, 0, 44.5], index=["A", "B", "C"], columns=["D"]) | ||
tm.assert_frame_equal(obj, expected) | ||
|
||
|
||
def test_at_assign_float_to_int_series(): | ||
# GH: 26395 | ||
obj = Series([0, 0, 0], index=["A", "B", "C"]) | ||
obj.at["C"] = 44.5 | ||
expected = Series([0, 0, 44.5], index=["A", "B", "C"]) | ||
tm.assert_series_equal(obj, 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.
why are you discarding
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.
If get_loc raises a
KeyError
we have to catch this here insted of inseries._set_value
but we do not needloc
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.
why bother calling get_loc here at all if its going to be called again in series._set_value?
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.
We want to arrive in the except clause here, when index does not exist instead of the except clause in
series._set_value
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 you elaborate on this? is it bc the except clause here does a takeable check whereas the one in _set_value does not? if so, could just add that check there?
why is the deleted comment about "tests.frame.indexing.test_indexing and tests.indexing.test_partial" no longer relevant?