Skip to content

BUG: Add np.uintc to _factorizers in merge.py to fix KeyError when merging DataFrames with uintc columns #58727

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 28 commits into from
Closed

BUG: Add np.uintc to _factorizers in merge.py to fix KeyError when merging DataFrames with uintc columns #58727

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
1774489
Add np.uintc to _factorizers in merge.py to fix KeyError when merging…
Tirthchoksi22 May 15, 2024
5372107
add np.uintc to _factorizers in merge.py
Tirthchoksi22 May 15, 2024
3d75d94
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] May 15, 2024
0f79322
changes according to review
Tirthchoksi22 May 15, 2024
2c18b6b
Merge branch 'main' of https://github.com/Tirthchoksi22/pandas
Tirthchoksi22 May 15, 2024
1373e05
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] May 15, 2024
16adf4b
final commit
Tirthchoksi22 May 15, 2024
523efa4
final commit
Tirthchoksi22 May 16, 2024
f3de1f9
Merge branch 'main' into main
Tirthchoksi22 May 16, 2024
67297bd
doc commit
Tirthchoksi22 May 16, 2024
61ff0e2
Merge branch 'main' of https://github.com/Tirthchoksi22/pandas
Tirthchoksi22 May 16, 2024
c05255a
final commit
Tirthchoksi22 May 16, 2024
87cb28d
Merge branch 'main' into main
Tirthchoksi22 May 16, 2024
2adb5fc
final
Tirthchoksi22 May 16, 2024
88a1618
final
Tirthchoksi22 May 16, 2024
90e0b93
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] May 16, 2024
1b9e3d0
indentation change
Tirthchoksi22 May 16, 2024
4da0b86
indentation error solved
Tirthchoksi22 May 16, 2024
95bca2c
error solved
Tirthchoksi22 May 16, 2024
a37151e
Update pandas/core/reshape/merge.py
Tirthchoksi22 May 16, 2024
c5a3ccc
update
Tirthchoksi22 May 16, 2024
402c33c
Merge branch 'main' of https://github.com/Tirthchoksi22/pandas
Tirthchoksi22 May 16, 2024
7438297
update as said
Tirthchoksi22 May 16, 2024
9105c97
upadte
Tirthchoksi22 May 16, 2024
e3b76a1
Merge branch 'main' into main
Tirthchoksi22 May 16, 2024
8506f78
update
Tirthchoksi22 May 16, 2024
621c8d1
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] May 16, 2024
50fe143
Merge branch 'main' of https://github.com/Tirthchoksi22/pandas
Tirthchoksi22 May 19, 2024
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
1 change: 1 addition & 0 deletions pandas/core/reshape/merge.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@
np.complex64: libhashtable.Complex64Factorizer,
np.complex128: libhashtable.Complex128Factorizer,
np.object_: libhashtable.ObjectFactorizer,
np.uintc: libhashtable.UInt32Factorizer,
}

# See https://github.com/pandas-dev/pandas/issues/52451
Expand Down
7 changes: 5 additions & 2 deletions pandas/tests/reshape/merge/test_merge.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1491,9 +1491,12 @@ def test_different(self, dtype):
result = merge(left, right, on="A")
assert is_object_dtype(result.A.dtype) or is_string_dtype(result.A.dtype)

@pytest.mark.parametrize(
"d1", [np.int64, np.int32, np.intc, np.int16, np.int8, np.uint8]
)
@pytest.mark.parametrize("d2", [np.int64, np.float64, np.float32, np.float16])
def test_join_multi_dtypes(self, any_int_numpy_dtype, d2):
Copy link
Member

Choose a reason for hiding this comment

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

Could you keep this test the same and add a new separate test for np.uintc?

Copy link
Author

Choose a reason for hiding this comment

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

ok sure

dtype1 = np.dtype(any_int_numpy_dtype)
def test_join_multi_dtypes(self, d1, d2):
Copy link
Member

Choose a reason for hiding this comment

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

So it appears that this has been updated since the last fix.

I doubt that this should be changed. it's probably that any_int_numpy_dtype (and therefore tm.UNSIGNED_INT_NUMPY_DTYPES ) should be updated.

However, tm.UNSIGNED_INT_NUMPY_DTYPES does not appear to contain np.intc either. (maybe how the regression was uncaught)

So looks like will need to add both np.intc and np.uintc to tm.UNSIGNED_INT_NUMPY_DTYPES

Copy link
Author

Choose a reason for hiding this comment

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

ok i will add it in tm.UNSIGNED_INT_NUMPY_DTYPES

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 explain the issue to me (as I don't have the platform to test)

Although my previous comment was to align better with the previous fix, I don't know why we need to explicitly cater for np.intc and np.uintc since we don't need to for the other Python API “C-like” names such as numpy.short.

So adding these does not really make sense to me as I don't really understand the issue.

Copy link
Author

Choose a reason for hiding this comment

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

@simonjayhawkins what i think is the issue arises because certain functionalities within pandas were not properly handling np.intc and np.uintc data types. As a result, when these data types were encountered, it led to unexpected keyerrors.that's what i feel but don't know why we don't need to mention this in other Python API's

Copy link
Author

@Tirthchoksi22 Tirthchoksi22 May 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@simonjayhawkins I still didnt understand why regression occur on windows side perhaps compatibility issue ???

Copy link
Author

Choose a reason for hiding this comment

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

@simonjayhawkins if you think to change this according to last fix then tell me as this fix will also work

Copy link
Member

Choose a reason for hiding this comment

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

functionalities within pandas were not properly handling np.intc

well it works on other platforms, so it maybe not a pandas issue?

dtype1 = np.dtype(d1)
dtype2 = np.dtype(d2)

left = DataFrame(
Expand Down