TST: add test_ffill_with_string_column #40557
Conversation
Update fork
There was a problem hiding this comment.
This LGTM @shiv-io, thanks for contributing to pandas! Is there a reason the pr is in draft status?
|
Hey @mzeitlin11, no reason in particular - just thought I'd wait for the CI checks to pass before making it ready for review. It does seem that one of the CI checks is failing (CI / Web and docs), but I have no reason to suspect that the failure has to do with the changes in this PR. Also: it seems that the whatsnew entry may not be needed since this is just testing an already-fixed bug. Let me know if that's the case. I have marked as ready to review! Thank you |
Yep, whatsnew is not necessary. The CI / Web and docs failure is unrelated and should be fixed by #40542 |
mzeitlin11
added
Testing
pandas/tests/groupby/test_missing.py
Outdated
| def test_ffill_with_string_column(): | ||
| # GH 40250 | ||
| result = ( | ||
| DataFrame({"a": pd.array([None, "a"], dtype="string"), "b": [0, 0]}) |
There was a problem hiding this comment.
can you change the example so it also fills a forward value (e.g. make the input '[None, "a", None]'
pandas/tests/groupby/test_missing.py
Outdated
| .groupby("b") | ||
| .ffill() | ||
| ) | ||
| expected = DataFrame({"a": pd.array([None, "a"], dtype="string")}) |
There was a problem hiding this comment.
would also not object to a parameterized version which tests bfill as well.
pandas/tests/groupby/test_missing.py
Outdated
| @@ -43,6 +43,17 @@ def test_ffill_missing_arguments(): | |||
| df.groupby("b").fillna() | |||
|
|
|||
|
|
|||
| def test_ffill_with_string_column(): | |||
There was a problem hiding this comment.
rename to test_ffill_with_string_dtype
There was a problem hiding this comment.
I thought it made sense to rename to test_fillna_with_string_dtype since I have now parameterized the test to test both ffill and bfill.
| expected = DataFrame({"a": pd.array([None, "a"], dtype="string")}) | ||
| tm.assert_frame_equal(result, expected) | ||
|
|
||
|
|
There was a problem hiding this comment.
we may have a similar example with object dtype, if you can co-locate, alternatively can parameterize on the dtype as well.
There was a problem hiding this comment.
after #41343 is merged, can use the any_string_dtype fixture
There was a problem hiding this comment.
Thanks @shiv-io lgtm pending green (param on any_string_dtype could be a follow-up)
|
thanks @shiv-io |
#40250 is an issue which seems to have been fixed in master already through #39446. This PR adds a test for this issue.