Skip to content

TST: test for inconsistency due to dtype=string #46512 #47793

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.

Sign up for GitHub

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

Closed
wants to merge 40 commits into from
Closed

TST: test for inconsistency due to dtype=string #46512 #47793

Changes from 17 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
5b39d02
TST
Shadimrad Jul 14, 2022
e0a27b2
TST
Shadimrad Jul 14, 2022
bd32b97
TST
Shadimrad Jul 14, 2022
48a27f8
updated strign formatting
Shadimrad Jul 14, 2022
dceccf0
updated formatting
Shadimrad Jul 14, 2022
1904094
updating the test place within files
Shadimrad Jul 15, 2022
7270b34
removing an additional parentheses
Shadimrad Jul 15, 2022
7fb362d
removing the buggy file that was pushed
Shadimrad Jul 15, 2022
e48a63d
removing the buggy file that was pushed
Shadimrad Jul 15, 2022
15be2f5
fixing initial file
Shadimrad Jul 15, 2022
204affb
fixing order of import
Shadimrad Jul 15, 2022
22843cc
fixing space
Shadimrad Jul 15, 2022
d1d0601
fixing space
Shadimrad Jul 15, 2022
c795d5d
fixing space
Shadimrad Jul 15, 2022
da73d0e
issue2
Shadimrad Jul 20, 2022
91979a4
Merge branch 'main' into issue2
Shadimrad Jul 20, 2022
d002ce0
Update test_arithmetic.py
Shadimrad Jul 20, 2022
a8146e9
test
Shadimrad Jul 20, 2022
1d3e914
Update test_where.py
Shadimrad Jul 20, 2022
e92bb7c
Update test_where.py
Shadimrad Jul 20, 2022
6d446f9
Update generic.py
Shadimrad Jul 20, 2022
8d60097
Update generic.py
Shadimrad Jul 20, 2022
007612f
Update generic.py
Shadimrad Jul 20, 2022
c655774
Update test_where.py
Shadimrad Jul 20, 2022
6df1d30
Merge branch 'main' into issue2
Shadimrad Jul 20, 2022
7cf318a
test
Shadimrad Jul 20, 2022
3452783
Update test_where.py
Shadimrad Aug 4, 2022
bcdff67
Update test_where.py
Shadimrad Aug 4, 2022
39e53f6
Merge branch 'issue2' of https://github.com/Shadimrad/pandas into issue2
Shadimrad Aug 4, 2022
ca7a705
Update test_where.py
Shadimrad Aug 4, 2022
4e9b374
do
Shadimrad Aug 4, 2022
b78896f
Update test_where.py
Shadimrad Aug 4, 2022
775f979
branch
Shadimrad Aug 4, 2022
f4dec29
fix
Shadimrad Aug 4, 2022
86b6b9a
relocate
Shadimrad Aug 4, 2022
e6a73a8
Merge branch 'main' into issue2
Shadimrad Aug 4, 2022
cfeb393
Merge branch 'main' into issue2
Shadimrad Aug 16, 2022
acffd23
Merge branch 'main' into issue2
Shadimrad Aug 17, 2022
d4ec9b5
Update test_string.py
Shadimrad Aug 18, 2022
b2a8c1f
Merge branch 'main' into issue2
Shadimrad Aug 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions pandas/core/generic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9500,6 +9500,11 @@ def _where(
self._check_inplace_setting(other)
new_data = self._mgr.putmask(mask=cond, new=other, align=align)
result = self._constructor(new_data)
for i in range(len(result.dtypes)):
Copy link
Member

Choose a reason for hiding this comment

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

This is the wrong place for this. We can not special case this here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you mind elaborating on what you mean? I believe it is not a special case since it just affects the type in the case that the inplace is True. Do you mean I should put it within the putmask? @phofl

Copy link
Member

Choose a reason for hiding this comment

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

Probably, but this looks already correct on main, so no need to fix I think.

1.5.0.dev0+1180.g8c3a2f2ba7
<StringArray>
['1', <NA>, '3']
Length: 3, dtype: string
<class 'pandas._libs.missing.NAType'>
      A
0     1
1  <NA>
2     3
<StringArray>
['1', <NA>, '3']
Length: 3, dtype: string
<class 'pandas._libs.missing.NAType'>

Could you simply add a test?

cc @simonjayhawkins

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh! sure.

Copy link
Member

Choose a reason for hiding this comment

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

Probably, but this looks already correct on main, so no need to fix I think.

see #46512 (comment)

I'll confirm the commit where the fix occurred and if we agree that this is the correct behavior, then indeed we just need a test.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is essentially the same
As #47628

Copy link
Member

Choose a reason for hiding this comment

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

yes from #47628 (comment)

But the strange thing is that doing this on the Series level doesn't end up calling StringArray.setitem, it seems to go through Series._where and eventually BlockManager.putmask, and ExtensionArray._putmask, and that last one is not correctly implemented for StringArray.

but we should probably have tests for DataFrame.where also incase the implementation of __setitem__ changes to no longer go through Series._where

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just out of curiosity, how would it be implemented if we should not have special cased it? Is there a link to the change that fixed it on the main by any chance? @phofl

if result.dtypes[i] != 'object':
result[result.columns[i]] = (
result[result.columns[i]].convert_dtypes()
)
return self._update_inplace(result)

else:
Expand Down