Skip to content
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

test: Add test for loc assignment changes datetime dtype #50037

Merged

Conversation

alonme
Copy link
Contributor

@alonme alonme commented Dec 3, 2022

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks @alonme

@MarcoGorelli MarcoGorelli added the Needs Tests Unit test(s) needed to prevent regressions label Dec 3, 2022
@MarcoGorelli MarcoGorelli added this to the 2.0 milestone Dec 3, 2022
Copy link
Member

@phofl phofl left a comment

Choose a reason for hiding this comment

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

small comments

pandas/tests/indexing/test_loc.py Outdated Show resolved Hide resolved
expected = df.copy(deep=True)

update_df = df[df["update"]]
df.loc[df["update"], ["date"]] = update_df["date"]
Copy link
Member

Choose a reason for hiding this comment

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

Can you test for df.loc[df["update"], ["date"]] and df.loc[df["update"], "date"]? This takes significantly different code paths.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, actually the bug didn't occur in the second one.
will add.

BTW - is this the right place for the test?

Copy link
Member

Choose a reason for hiding this comment

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

good point, better place is probably frame/indexing/test_indexing

@alonme alonme force-pushed the 49837_testing_add_loc_test_for_dt_dtype_change branch from 788cd14 to ba9b41d Compare December 3, 2022 19:06
@alonme
Copy link
Contributor Author

alonme commented Dec 3, 2022

@phofl ping
Thanks for the review!

@@ -1454,6 +1455,32 @@ def test_loc_bool_multiindex(self, dtype, indexer):
)
tm.assert_frame_equal(result, expected)

@pytest.mark.parametrize("utc", [False, True])
@pytest.mark.parametrize("column_label_array", [False, True])
Copy link
Member

Choose a reason for hiding this comment

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

You can do @pytest.mark.parametrize("indexer", ["date", ["date]]), then you don't need if on your test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@alonme alonme requested a review from phofl December 4, 2022 18:37
@phofl phofl merged commit 2411042 into pandas-dev:main Dec 5, 2022
@phofl
Copy link
Member

phofl commented Dec 5, 2022

thx @alonme

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Tests Unit test(s) needed to prevent regressions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: Assignment to a .loc view of a naive datetime column changes its dtype to object
3 participants