TST: ro_replace changing dtype of series #50664
Conversation
|
👋 @pooja-subramaniam
If you want, you can have a look at the details link to see what the errors look like, so you can identify them next time 😉. In the future, I recommend making a feature branch in your fork, rather than making a PR from your fork's main branch. You can find guidance for this in our documentation (here). |
|
thanks for the points @noatamir! |
|
@noatamir I don't understand the reason for the canceled check here. Do you know why that happened? |
| def test_replace_change_dtype_series(self): | ||
| # GH#25797 | ||
| df = pd.DataFrame.from_dict({"Test": ["0.5", True, "0.6"]}) | ||
| print(df["Test"].dtype) |
There was a problem hiding this comment.
Some pointers: Please remove the print statements.
Generally, you can remove your first assert statements per block, but we want to compare the whole df with the second assert statement, e.g. define your expected object and then call assert_frame_equal
expected = ...
tm.assert_frame_equal(df, expected)
if expected is the same as your initial df, you can define expected through
expected = df.copy()
in the beginning
There was a problem hiding this comment.
Thanks! Made the changes.
Correct me if I am wrong, but here the assert needs to check if the series dtype remains 'object' and not if the dataframes are equal. I've made changes accordingly.
There was a problem hiding this comment.
tm.assert_frame_equal raises if your dtypes are different. So this is covered as well. In general, we want to check that the DataFrame is as expected (values, columns, index, dtype, ...). That is why we are using assert_frame_equal
The functions could modify the values unexpectedly and the test would not fail, if you only check the dtypes
There was a problem hiding this comment.
Ah okay! Thanks for explaining :)
| # GH#25797 | ||
| df = pd.DataFrame.from_dict({"Test": ["0.5", True, "0.6"]}) | ||
| df["Test"] = df["Test"].replace([True], [np.nan]) | ||
| expected = pd.DataFrame.from_dict({"Test": ["0.5", np.nan, "0.6"]}) |
There was a problem hiding this comment.
One last comment: Could you define expected only once after line 681 and reuse it for all assert_frame_equal statements? It's not necessary to define it multiple times when it's the same object :)
There was a problem hiding this comment.
Right! How did I miss that one! Thanks again ;)
phofl
changed the title
@phofl and @noatamir