Skip to content

CLN: Remove unused code in Factorizer classes #49547

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

Merged
merged 4 commits into from
Nov 6, 2022
Merged

CLN: Remove unused code in Factorizer classes #49547

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
58b6e50
CLN: Remove unused code in Factorizer classes
phofl Nov 5, 2022
18e3d35
Fix test
phofl Nov 5, 2022
de20329
Adjust test
phofl Nov 5, 2022
b094a6c
Fix tests
phofl Nov 5, 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
21 changes: 2 additions & 19 deletions pandas/_libs/hashtable.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ cdef class ObjectFactorizer(Factorizer):
self.uniques = ObjectVector()

def factorize(
self, ndarray[object] values, sort=False, na_sentinel=-1, na_value=None
self, ndarray[object] values, na_sentinel=-1, na_value=None
) -> np.ndarray:
"""

Expand All @@ -115,14 +115,6 @@ cdef class ObjectFactorizer(Factorizer):
self.uniques = uniques
labels = self.table.get_labels(values, self.uniques,
self.count, na_sentinel, na_value)
mask = (labels == na_sentinel)
# sort on
if sort:
sorter = self.uniques.to_array().argsort()
reverse_indexer = np.empty(len(sorter), dtype=np.intp)
reverse_indexer.put(sorter, np.arange(len(sorter)))
labels = reverse_indexer.take(labels, mode='clip')
labels[mask] = na_sentinel
self.count = len(self.uniques)
return labels

Expand All @@ -136,7 +128,7 @@ cdef class Int64Factorizer(Factorizer):
self.table = Int64HashTable(size_hint)
self.uniques = Int64Vector()

def factorize(self, const int64_t[:] values, sort=False,
def factorize(self, const int64_t[:] values,
na_sentinel=-1, na_value=None) -> np.ndarray:
"""
Returns
Expand All @@ -161,14 +153,5 @@ cdef class Int64Factorizer(Factorizer):
labels = self.table.get_labels(values, self.uniques,
self.count, na_sentinel,
na_value=na_value)

# sort on
if sort:
sorter = self.uniques.to_array().argsort()
reverse_indexer = np.empty(len(sorter), dtype=np.intp)
reverse_indexer.put(sorter, np.arange(len(sorter)))

labels = reverse_indexer.take(labels)

self.count = len(self.uniques)
return labels
16 changes: 3 additions & 13 deletions pandas/tests/test_algos.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,21 +230,11 @@ def test_factorize_nan(self):
key = np.array([1, 2, 1, np.nan], dtype="O")
rizer = ht.ObjectFactorizer(len(key))
for na_sentinel in (-1, 20):
ids = rizer.factorize(key, sort=True, na_sentinel=na_sentinel)
expected = np.array([0, 1, 0, na_sentinel], dtype="int32")
Copy link
Member

@rhshadrach rhshadrach Nov 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Just to make sure I'm understanding - the dtype specified for expected in main currently is wrong, but the old test never actually checked it. Is that right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct. Honestly, I am not sure what the old test was supposed to test at all.

We get int32 on 32-bit systems, so I disabled the dtype check, it's not relevant here anyway

ids = rizer.factorize(key, na_sentinel=na_sentinel)
expected = np.array([0, 1, 0, na_sentinel], dtype="int64")
assert len(set(key)) == len(set(expected))
tm.assert_numpy_array_equal(pd.isna(key), expected == na_sentinel)

# nan still maps to na_sentinel when sort=False
key = np.array([0, np.nan, 1], dtype="O")
na_sentinel = -1

# TODO(wesm): unused?
ids = rizer.factorize(key, sort=False, na_sentinel=na_sentinel) # noqa

expected = np.array([2, -1, 0], dtype="int32")
assert len(set(key)) == len(set(expected))
tm.assert_numpy_array_equal(pd.isna(key), expected == na_sentinel)
tm.assert_numpy_array_equal(ids, expected, check_dtype=False)
Copy link
Member

Choose a reason for hiding this comment

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

why check_dtype=False here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because 32 bit systems return 32 bit and the dtype does not matter here at all. Can add an if else to specify the int32/int64 if you prefer though

Copy link
Member

Choose a reason for hiding this comment

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

so use np.intp?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, good idea. thx


@pytest.mark.parametrize(
"data, expected_codes, expected_uniques",
Expand Down