Skip to content

CLN: prepare unifying hashtable.factorize and .unique; add doc-strings #22986

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 33 commits into from
Oct 18, 2018
Merged

CLN: prepare unifying hashtable.factorize and .unique; add doc-strings #22986

Changes from 11 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
640162f
Fix ASV import error
h-vetinari Oct 3, 2018
31d0dc5
Add return_inverse to hashtable.unique
h-vetinari Sep 27, 2018
c5e5147
Pure copy/paste: Group unique/factorize functions next to each other
h-vetinari Sep 30, 2018
9918d52
Unify hashtable.factorize and .unique
h-vetinari Oct 3, 2018
52ae84e
Force compilation of different code paths
h-vetinari Oct 4, 2018
dbe4e0e
Add separate functions for return_inverse=False
h-vetinari Oct 4, 2018
8481e19
Finish split in _unique_with_inverse and _unique_no_inverse
h-vetinari Oct 4, 2018
27ceb4d
Add cython.wraparound(False)
h-vetinari Oct 4, 2018
f5cd5e9
Merge remote-tracking branch 'upstream/master' into re_factor_ize
h-vetinari Oct 6, 2018
b1705a9
Unmove unique-implementation (review jreback)
h-vetinari Oct 6, 2018
a6ed5dd
Undo line artefacts
h-vetinari Oct 6, 2018
17752ce
Merge remote-tracking branch 'upstream/master' into re_factor_ize
h-vetinari Oct 7, 2018
19eaf32
Clean up test_algos.test_vector_resize
h-vetinari Oct 7, 2018
ce7626f
Add test for hashtable.unique (esp. for return_inverse=True)
h-vetinari Oct 7, 2018
7b9014f
Review (jreback)
h-vetinari Oct 7, 2018
471c4da
Fix typo
h-vetinari Oct 8, 2018
9d45378
Small fixes
h-vetinari Oct 8, 2018
00b2ccb
Review (jorisvandenbossche)
h-vetinari Oct 8, 2018
8687315
Merge remote-tracking branch 'upstream/master' into re_factor_ize
h-vetinari Oct 11, 2018
a267d4a
Review (jorisvandenbossche)
h-vetinari Oct 11, 2018
7f1bb40
Improve comment
h-vetinari Oct 11, 2018
9593992
Test for writable; expand comments
h-vetinari Oct 12, 2018
08d7f50
Simplify factorize test
h-vetinari Oct 12, 2018
d91be98
Add simple test
h-vetinari Oct 12, 2018
e27ec9a
Tiny fixes
h-vetinari Oct 12, 2018
d825be0
Remove idx_duplicated from test (now unnecessary)
h-vetinari Oct 12, 2018
1a342d0
Review (jreback)
h-vetinari Oct 14, 2018
28e0441
Merge remote-tracking branch 'upstream/master' into re_factor_ize
h-vetinari Oct 15, 2018
d65e4fd
Merge remote-tracking branch 'upstream/master' into re_factor_ize
h-vetinari Oct 15, 2018
bca615c
Merge remote-tracking branch 'upstream/master' into re_factor_ize
h-vetinari Oct 17, 2018
3438727
Review (jreback)
h-vetinari Oct 17, 2018
facc111
Merge remote-tracking branch 'upstream/master' into re_factor_ize
h-vetinari Oct 18, 2018
6d0e86b
Retrigger Circle
h-vetinari Oct 18, 2018
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
143 changes: 103 additions & 40 deletions pandas/_libs/hashtable_class_helper.pxi.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,19 +355,15 @@ cdef class {{name}}HashTable(HashTable):

return np.asarray(locs)

def factorize(self, {{dtype}}_t values):
uniques = {{name}}Vector()
labels = self.get_labels(values, uniques, 0, 0)
return uniques.to_array(), labels

@cython.boundscheck(False)
def get_labels(self, const {{dtype}}_t[:] values, {{name}}Vector uniques,
Py_ssize_t count_prior, Py_ssize_t na_sentinel,
object na_value=None):
@cython.wraparound(False)
def _unique_with_inverse(self, const {{dtype}}_t[:] values,
Copy link
Member

Choose a reason for hiding this comment

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

minor comment: since what this algo does is what we typically call "factorize" in pandas, I would call this function "_factorize"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jorisvandenbossche
Can we keep this for the follow-up please? It will make sense there, because then factorize = unique_with_inverse(ignore_na=True).

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand your point here. hashtable.factorize already exists?

Copy link
Member

Choose a reason for hiding this comment

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

IMO it makes sense to do that here, because it is here you are changing get_labels

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I thought this was a reply to the other comment :-)

It will make sense there, because then factorize = unique_with_inverse(ignore_na=True)

Yes, and otherwise the other way around unique(return_inverse=True) = factorize(ignore_na=False). The implemenation of that function is what factorize does in pandas (and not unique), so it makes sense to me to use that name.

{{name}}Vector uniques, bint ignore_na=False,
Py_ssize_t count_prior=0,
Py_ssize_t na_sentinel=-1, object na_value=None):
cdef:
Py_ssize_t i, n = len(values)
Py_ssize_t i, idx, count = count_prior, n = len(values)
int64_t[:] labels
Py_ssize_t idx, count = count_prior
int ret = 0
{{dtype}}_t val, na_value2
khiter_t k
Expand All @@ -392,16 +388,19 @@ cdef class {{name}}HashTable(HashTable):
for i in range(n):
val = values[i]

if val != val or (use_na_value and val == na_value2):
if ignore_na and (val != val
or (use_na_value and val == na_value2)):
labels[i] = na_sentinel
continue

k = kh_get_{{dtype}}(self.table, val)

if k != self.table.n_buckets:
# k falls into a previous bucket
idx = self.table.vals[k]
labels[i] = idx
else:
# k hasn't been seen yet
k = kh_put_{{dtype}}(self.table, val, &ret)
self.table.vals[k] = count

Expand All @@ -416,7 +415,26 @@ cdef class {{name}}HashTable(HashTable):
labels[i] = count
count += 1

return np.asarray(labels)
return uniques.to_array(), np.asarray(labels)

def unique(self, const {{dtype}}_t[:] values, bint return_inverse=False):
if return_inverse:
return self._unique_with_inverse(values, uniques={{name}}Vector(),
ignore_na=False)
return self._unique_no_inverse(values)

def factorize(self, {{dtype}}_t[:] values):
return self._unique_with_inverse(values, uniques={{name}}Vector(),
ignore_na=True)

def get_labels(self, const {{dtype}}_t[:] values, {{name}}Vector uniques,
Py_ssize_t count_prior=0, Py_ssize_t na_sentinel=-1,
object na_value=None):
_, labels = self._unique_with_inverse(values, uniques, ignore_na=True,
count_prior=count_prior,
na_sentinel=na_sentinel,
na_value=na_value)
Copy link
Member

Choose a reason for hiding this comment

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

Is it actually needed to still define this? From a quick search it seems get_labels was only used in factorize ?

Copy link
Contributor Author

@h-vetinari h-vetinari Oct 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@jorisvandenbossche
From a first try of removing it, it wasn't straight-forward (doesn't just appear for .factorize, IIRC). Definitely something for a follow-up. :)

Copy link
Member

Choose a reason for hiding this comment

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

The only use case is in _factorize_array:

pandas/pandas/core/algorithms.py

Line 475 in c8ce3d0

labels = table.get_labels(values, uniques, 0, na_sentinel,

Which can directly use the hashtable.factorize method. So I think this would be easy to remove, and would actually simplify further the diff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jorisvandenbossche
I'd prefer not loading up this PR with more things. The removal of get_labels in favour of factorize could be a separate PR, no?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it could. But it is here that you are changing get_labels, and actually doing this would make the diff a lot smaller. So I think that is an indication that it naturally fits here.

return labels

@cython.boundscheck(False)
def get_labels_groupby(self, const {{dtype}}_t[:] values):
Expand Down Expand Up @@ -464,7 +482,9 @@ cdef class {{name}}HashTable(HashTable):
return np.asarray(labels), arr_uniques

@cython.boundscheck(False)
def unique(self, const {{dtype}}_t[:] values):
@cython.wraparound(False)
def _unique_no_inverse(self, const {{dtype}}_t[:] values):
# define separate functions without inverse for performance
cdef:
Py_ssize_t i, n = len(values)
int ret = 0
Expand Down Expand Up @@ -567,7 +587,9 @@ cdef class StringHashTable(HashTable):
return labels

@cython.boundscheck(False)
def unique(self, ndarray[object] values):
@cython.wraparound(False)
def _unique_no_inverse(self, ndarray[object] values):
# define separate functions without inverse for performance
cdef:
Py_ssize_t i, count, n = len(values)
int64_t[:] uindexer
Expand Down Expand Up @@ -602,11 +624,6 @@ cdef class StringHashTable(HashTable):
uniques.append(values[uindexer[i]])
return uniques.to_array()

def factorize(self, ndarray[object] values):
uniques = ObjectVector()
labels = self.get_labels(values, uniques, 0, 0)
return uniques.to_array(), labels

@cython.boundscheck(False)
def lookup(self, ndarray[object] values):
cdef:
Expand Down Expand Up @@ -669,34 +686,34 @@ cdef class StringHashTable(HashTable):
free(vecs)

@cython.boundscheck(False)
def get_labels(self, ndarray[object] values, ObjectVector uniques,
Py_ssize_t count_prior, int64_t na_sentinel,
object na_value=None):
@cython.wraparound(False)
def _unique_with_inverse(self, ndarray[object] values,
ObjectVector uniques, bint ignore_na=False,
Py_ssize_t count_prior=0,
Py_ssize_t na_sentinel=-1, object na_value=None):
cdef:
Py_ssize_t i, n = len(values)
Py_ssize_t i, idx, count = count_prior, n = len(values)
int64_t[:] labels
int64_t[:] uindexer
Py_ssize_t idx, count = count_prior
int ret = 0
object val
const char *v
const char **vecs
khiter_t k
bint use_na_value

# these by-definition *must* be strings
labels = np.zeros(n, dtype=np.int64)
uindexer = np.empty(n, dtype=np.int64)
use_na_value = na_value is not None

# pre-filter out missing
# and assign pointers
# assign pointers and pre-filter out missing (if ignore_na)
vecs = <const char **> malloc(n * sizeof(char *))
for i in range(n):
val = values[i]

if ((PyUnicode_Check(val) or PyString_Check(val)) and
not (use_na_value and val == na_value)):
if not ignore_na or ((PyUnicode_Check(val) or PyString_Check(val))
and not (use_na_value and val == na_value)):
# if ignore_na is False, we also stringify NaN/None/etc.
v = util.get_c_string(val)
vecs[i] = v
else:
Expand All @@ -705,15 +722,17 @@ cdef class StringHashTable(HashTable):
# compute
with nogil:
for i in range(n):
if labels[i] == na_sentinel:
if ignore_na and labels[i] == na_sentinel:
continue

v = vecs[i]
k = kh_get_str(self.table, v)
if k != self.table.n_buckets:
# k falls into a previous bucket
idx = self.table.vals[k]
labels[i] = <int64_t>idx
else:
# k hasn't been seen yet
k = kh_put_str(self.table, v, &ret)
self.table.vals[k] = count
uindexer[count] = i
Expand All @@ -726,7 +745,26 @@ cdef class StringHashTable(HashTable):
for i in range(count):
uniques.append(values[uindexer[i]])

return np.asarray(labels)
return uniques.to_array(), np.asarray(labels)

def unique(self, ndarray[object] values, bint return_inverse=False):
if return_inverse:
return self._unique_with_inverse(values, uniques=ObjectVector(),
ignore_na=False)
return self._unique_no_inverse(values)

def factorize(self, ndarray[object] values):
return self._unique_with_inverse(values, uniques=ObjectVector(),
ignore_na=True)

def get_labels(self, ndarray[object] values, ObjectVector uniques,
Py_ssize_t count_prior=0, Py_ssize_t na_sentinel=-1,
object na_value=None):
_, labels = self._unique_with_inverse(values, uniques, ignore_na=True,
count_prior=count_prior,
na_sentinel=na_sentinel,
na_value=na_value)
return labels


cdef class PyObjectHashTable(HashTable):
Expand Down Expand Up @@ -814,7 +852,10 @@ cdef class PyObjectHashTable(HashTable):

return np.asarray(locs)

def unique(self, ndarray[object] values):
@cython.boundscheck(False)
@cython.wraparound(False)
def _unique_no_inverse(self, ndarray[object] values):
Copy link
Contributor

Choose a reason for hiding this comment

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

i am bothered by the naming, call these just _unique, you can leave _unique_with_inverse, though add a comment that makes it clear this is less performant as its doing more ops. bonus for doc-strings on these.

# define separate functions without inverse for performance
cdef:
Py_ssize_t i, n = len(values)
int ret = 0
Expand All @@ -832,13 +873,15 @@ cdef class PyObjectHashTable(HashTable):

return uniques.to_array()

def get_labels(self, ndarray[object] values, ObjectVector uniques,
Py_ssize_t count_prior, int64_t na_sentinel,
object na_value=None):
@cython.boundscheck(False)
@cython.wraparound(False)
def _unique_with_inverse(self, ndarray[object] values,
ObjectVector uniques, bint ignore_na=False,
Py_ssize_t count_prior=0,
Py_ssize_t na_sentinel=-1, object na_value=None):
cdef:
Py_ssize_t i, n = len(values)
Py_ssize_t i, idx, count = count_prior, n = len(values)
int64_t[:] labels
Py_ssize_t idx, count = count_prior
int ret = 0
object val
khiter_t k
Expand All @@ -851,20 +894,40 @@ cdef class PyObjectHashTable(HashTable):
val = values[i]
hash(val)

if ((val != val or val is None) or
(use_na_value and val == na_value)):
if ignore_na and ((val != val or val is None)
Copy link
Contributor

Choose a reason for hiding this comment

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

this keyword is completely untested

Copy link
Contributor Author

@h-vetinari h-vetinari Oct 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I would argue that it is (at least for ignore_na=True) because all the tests for factorize need to run through this.

Do we explicitly test cython code? I can if necessary. This PR does the required preparations on cython level to continue with return_inverse later on. It will be excessively tested there (because unique(return_inverse=True) calls _unique_with_inverse(ignore_na=False)).

Copy link
Contributor

Choose a reason for hiding this comment

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

yes of course, you added a keyword.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am talking about the ignore_na keyword

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know that I added this keyword to switch between .unique/.factorize behaviour.

Can you point me to where the cython hashtable code is tested? I could add some tests there. That's why I asked "Do we explicitly test cython code?", because I haven't come across it yet.

Otherwise, I would add copious tests for the behaviour as soon as the keyword makes its way into the pandas API, but this is just the preparation for that.

Copy link
Member

Choose a reason for hiding this comment

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

@h-vetinari : IMO tests/generic/test_generic.py or tests/generic/test_frame.py could be good places to put any kinds of tests for this refactoring for now.

If it is possible to hit those keyword arguments from Python land, then do add some tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

let's drop the ignore_na for now. and add it when I see that you actually need it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jreback
This aspect is the whole point of the PR - to not duplicate the code between .factorize (=hashtable._unique_with_inverse(ignore_na=True)) and the not-yet-implemented-because-this-PR-is-starting-the-process .unique(return_inverse=True) (=hashtable._unique_with_inverse(ignore_na=False))
hashtable._unique_with_inverse(ignore_na=False)

or (use_na_value and val == na_value)):
labels[i] = na_sentinel
continue

k = kh_get_pymap(self.table, <PyObject*>val)
if k != self.table.n_buckets:
# k falls into a previous bucket
idx = self.table.vals[k]
labels[i] = idx
else:
# k hasn't been seen yet
k = kh_put_pymap(self.table, <PyObject*>val, &ret)
self.table.vals[k] = count
uniques.append(val)
labels[i] = count
count += 1

return np.asarray(labels)
return uniques.to_array(), np.asarray(labels)

def unique(self, ndarray[object] values, bint return_inverse=False):
if return_inverse:
return self._unique_with_inverse(values, uniques=ObjectVector(),
ignore_na=False)
return self._unique_no_inverse(values)

def factorize(self, ndarray[object] values):
return self._unique_with_inverse(values, uniques=ObjectVector(), ignore_na=True)

def get_labels(self, ndarray[object] values, ObjectVector uniques,
Py_ssize_t count_prior=0, Py_ssize_t na_sentinel=-1,
object na_value=None):
_, labels = self._unique_with_inverse(values, uniques, ignore_na=True,
count_prior=count_prior,
na_sentinel=na_sentinel,
na_value=na_value)
return labels