-
Notifications
You must be signed in to change notification settings - Fork 151
Add kd-tree option to knn. #344
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.
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
Conversation
This commits add the a partial kd-tree implementation to be used for knn operations. This function is designed to work independently in case some future modules require its functionality.
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
Fixes import and keyword related bugs as well.
|
Refer to this link for build results (access rights to CI server needed): |
|
|
||
|
|
||
| def kd_tree(schema_madlib, source_table, output_table, point_column_name, depth, **kwargs): | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a docstring with info on function objective and arguments.
| with MinWarning("error"): | ||
|
|
||
| validate_kd_tree(source_table, output_table, point_column_name, depth) | ||
| num_features = plpy.execute("SELECT array_upper({point_column_name},1) AS num FROM {source_table}".format(**locals()))[0]['num'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please replace with num_features() from utilities.py_in
| # Traverse the clauses list in reverse order because we only want the leaf conditions | ||
| for i in reversed(range(n_leaves)): | ||
| # Remove the first 14 characters to get rid of the "WHERE 1=1 AND " segment | ||
| cond = clauses.pop()[14:] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove the [14:] completely? It looks like the WHERE is the problem since 1=1 is a valid condition and would be TRUE for each row. The WHERE can be moved to within the query itself (line 182) instead of part of all the clauses.
|
|
||
|
|
||
| def kd_tree(schema_madlib, source_table, output_table, point_column_name, depth, **kwargs): | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we please add docstrings for this and the knn_tree function describing the purpose and meaning of each argument?
| format(output_table_tree, | ||
| " ,".join(map(str, cutoffs)))) | ||
|
|
||
| case_when_clause = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming we remove the WHERE out of the clauses, this can be built in a list comprehension:
case_when_clause = ["WHEN {0} THEN {1}::INTEGER".format(cond, i)
for i, cond in enumerate(clauses[-n_leaves:])]
This commits add the a partial kd-tree implementation to be used for knn
operations. This function is designed to work independently in case some
future modules require its functionality.