Unify dense and sparse import in FIL #4328

levsnv · 2021-11-04T22:46:12Z

No description provided.

…nse-sparse-import

levsnv · 2021-11-11T20:02:55Z

rerun tests:

cuml.test.test_metrics.test_sparse_pairwise_distances_sklearn_comparison[matrix_size0-0.4-hellinger]
metric = 'hellinger', matrix_size = (1000, 100), density = 0.4
AssertionError: 
Arrays are not almost equal to 9 decimals

Mismatched elements: 529 / 1000000 (0.0529%)
Max absolute difference: 2.35608046e-08
Max relative difference: 1.44948974
cuml.test.test_metrics.test_sparse_pairwise_distances_sklearn_comparison[matrix_size1-0.01-hellinger]
metric = 'hellinger', matrix_size = (20, 10000), density = 0.01
AssertionError: 
Arrays are not almost equal to 9 decimals

Mismatched elements: 13 / 400 (3.25%)
Max absolute difference: 2.58095683e-08
Max relative difference: 1.

canonizer

I like the overall direction where it is going. This pull request is in good shape, but there are still some comments.

Most of the comments are technical, but here are the points I'd like to highlight:

It would be better to combine various compile-time node properties into node_traits<node_t>.
I like the introduction of a separate conversion type tl2fil_t to hold all conversion-related state. I think functions like tree2fil could be methods of this type as well.

canonizer · 2021-11-12T01:20:41Z

cpp/src/fil/common.cuh

@@ -114,6 +114,16 @@ struct sparse_storage : storage_base {
 typedef sparse_storage<sparse_node16> sparse_storage16;
 typedef sparse_storage<sparse_node8> sparse_storage8;

+template <typename fil_node_t>
+struct node2storage {


There are several types like this in this pull request. It would be better to unify them into a single node_traits type. E.g.

// for sparse nodes template <typename node_t> struct node_traits { using storage = sparse_storage<node_t>; using forest = sparse_forest<node_t>; static const bool IS_DENSE = false; }; // for dense nodes template<> struct node_traits<dense_node> { using forest = dense_forest; using storage = dense_storage; static const bool IS_DENSE = true; };

It is also possible to include the check() method that checks whether the number of nodes fits into sparse_node8, but that's up to you.

definitely! I assume putting them all into the dense_node, sparse_node* themselves will not be as neat?

Though possible, it won't be as neat, as it would mix the node data structure with unrelated information.

A separate node_traits data type looks better.

cpp/src/fil/fil.cu

canonizer · 2021-11-12T01:25:47Z

cpp/src/fil/fil.cu

-  check_params(params, false);
-  sparse_forest<fil_node_t>* f = new sparse_forest<fil_node_t>(h);
+  check_params(params, is_dense<fil_node_t>());
+  auto f = new typename node2forest<fil_node_t>::T(h);


It's better to split this line:

using forest_type = typename node_traits<fil_node_t>::forest; forest f = new forest_type(h);

wrote forest_type* f = new forest_type(h); because you chose to make void init(...) non-virtual and not part of struct forest base class. Did you intend for me to make it virtual?

Feel free to make init() virtual if it would simplify things.

cpp/src/fil/internal.cuh

cpp/src/fil/treelite_import.cu

Co-authored-by: Andy Adinets <adinetz@gmail.com>

…nse-sparse-import

…to unify-dense-sparse-import

cpp/src/fil/common.cuh

cpp/src/fil/treelite_import.cu

levsnv · 2021-11-24T07:12:12Z

rerun tests:

Test Result (2 failures / +2)
cuml.test.test_nearest_neighbors.test_nearest_neighbors_rbc[10000-4-euclidean]
cuml.test.test_nearest_neighbors.test_nearest_neighbors_rbc[10000-25-euclidean]
--
distance = 'euclidean', n_neighbors = 4, nrows = 10000

    @pytest.mark.parametrize('distance', ["euclidean", "haversine"])
    @pytest.mark.parametrize('n_neighbors', [4, 25])
    @pytest.mark.parametrize('nrows', [unit_param(10000), stress_param(70000)])
    def test_nearest_neighbors_rbc(distance, n_neighbors, nrows):
        X, y = make_blobs(n_samples=nrows,
                          centers=25,
                          shuffle=True,
                          n_features=2,
                          cluster_std=3.0,
                          random_state=42)
    
        knn_cu = cuKNN(metric=distance, algorithm="rbc")
        knn_cu.fit(X)
    
        query_rows = int(nrows/2)
    
        rbc_d, rbc_i = knn_cu.kneighbors(X[:query_rows, :],
                                         n_neighbors=n_neighbors)
    
        if distance == 'euclidean':
            # Need to use unexpanded euclidean distance
            pw_dists = cuPW(X, metric="l2")
            brute_i = cp.argsort(pw_dists, axis=1)[:query_rows, :n_neighbors]
            brute_d = cp.sort(pw_dists, axis=1)[:query_rows, :n_neighbors]
        else:
            knn_cu_brute = cuKNN(metric=distance, algorithm="brute")
            knn_cu_brute.fit(X)
    
            brute_d, brute_i = knn_cu_brute.kneighbors(
                X[:query_rows, :], n_neighbors=n_neighbors)
    
        rbc_i = cp.sort(rbc_i, axis=1)
        brute_i = cp.sort(brute_i, axis=1)
    
        # TODO: These are failing with 1 or 2 mismatched elements
        # for very small values of k:
        # https://github.com/rapidsai/cuml/issues/4262
>       assert len(brute_d[brute_d != rbc_d]) <= 1
E       assert 139 <= 1
E        +  where 139 = len(array([0.26635543, 0.14040843, 0.15865709, 0.224747  , 0.22284727,\n       0.31209642, 0.196584  , 0.4371513 , 0.459564... 0.09977522, 0.20649612, 0.20396307, 0.18100378,\n       0.36238492, 0.41610897, 0.14166015, 0.17231807], dtype=float32))

cuml/test/test_nearest_neighbors.py:556: AssertionError

levsnv · 2021-11-25T07:16:53Z

same test error

levsnv · 2021-12-02T00:26:30Z

rerun tests

…nse-sparse-import

levsnv · 2021-12-03T19:13:12Z

C++ test failed: 20:02:37 [ FAILED ] UMAPParametrizableTest.Result

dantegd · 2021-12-13T16:41:36Z

rerun tests

levsnv · 2021-12-15T01:04:39Z

lots of C++ tests fail with
C++ exception with description "CUDA error encountered at: file=_deps/raft-src/cpp/include/raft/handle.hpp line=250: call='cudaEventCreateWithFlags(&event_, cudaEventDisableTiming)', Reason=cudaErrorIllegalAddress:an illegal memory access was encountered

levsnv · 2021-12-15T01:22:30Z

rerun tests

wphicks

Beautiful work! Just a couple stray C types and one spot where we can avoid a raw loop. Other than that, it looks perfect.

cpp/src/fil/treelite_import.cu

wphicks · 2021-12-17T00:18:59Z

@gpucibot merge

codecov-commenter · 2021-12-17T07:07:24Z

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.02@1b46ec4). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff               @@
##             branch-22.02    #4328   +/-   ##
===============================================
  Coverage                ?   85.72%           
===============================================
  Files                   ?      236           
  Lines                   ?    19326           
  Branches                ?        0           
===============================================
  Hits                    ?    16568           
  Misses                  ?     2758           
  Partials                ?        0

Flag	Coverage Δ
dask	`46.51% <0.00%> (?)`
non-dask	`78.62% <0.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b46ec4...fe38a39. Read the comment docs.

wphicks · 2021-12-17T21:30:08Z

@gpucibot merge

The pull request includes changes from #4328. To view only changes pertinent to unifying tests, see https://github.com/levsnv/cuml/pull/3/files?diff=unified&w=1 Authors: - Levs Dolgovs (https://github.com/levsnv) Approvers: - Andy Adinets (https://github.com/canonizer) - William Hicks (https://github.com/wphicks) URL: #4417

Authors: - Levs Dolgovs (https://github.com/levsnv) Approvers: - Andy Adinets (https://github.com/canonizer) - William Hicks (https://github.com/wphicks) URL: rapidsai#4328

The pull request includes changes from rapidsai#4328. To view only changes pertinent to unifying tests, see https://github.com/levsnv/cuml/pull/3/files?diff=unified&w=1 Authors: - Levs Dolgovs (https://github.com/levsnv) Approvers: - Andy Adinets (https://github.com/canonizer) - William Hicks (https://github.com/wphicks) URL: rapidsai#4417

unified tree2fil

701956e

github-actions bot added the CUDA/C++ label Nov 4, 2021

caryr35 added this to PR-WIP in v21.12 Release via automation Nov 8, 2021

levsnv added 5 commits November 9, 2021 20:44

unified init_dense, init_sparse

97dc049

drafted tl2fil as class for from_treelite

84bdc1e

Merge branch 'branch-21.12' of github.com:rapidsai/cuml into unify-de…

e1219bb

…nse-sparse-import

fixed a bug

c1b5d54

stray changes

6e8e463

levsnv requested a review from canonizer November 11, 2021 08:27

levsnv added non-breaking Non-breaking change Tech Debt Issues related to debt 3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function labels Nov 11, 2021

levsnv marked this pull request as ready for review November 11, 2021 08:28

levsnv requested a review from a team as a code owner November 11, 2021 08:28

canonizer suggested changes Nov 12, 2021

View reviewed changes

v21.12 Release automation moved this from PR-WIP to PR-Needs review Nov 12, 2021

levsnv and others added 7 commits November 11, 2021 19:06

Apply suggestions from code review

d6d0ece

Co-authored-by: Andy Adinets <adinetz@gmail.com>

apply suggestions from code review

42c1c2e

Merge branch 'branch-21.12' of github.com:rapidsai/cuml into unify-de…

c002e26

…nse-sparse-import

Merge branch 'unify-dense-sparse-import' of github.com:levsnv/cuml in…

9d304e6

…to unify-dense-sparse-import

style

6967714

made tree2fil a method of tl2fil_t, misc comments

dfdfff1

tl2fil_t:: init_object(), init_forest()

58044af

levsnv requested a review from canonizer November 17, 2021 05:50

tracking tokens

8377aee

canonizer suggested changes Nov 17, 2021

View reviewed changes

addressed review comments

e170290

levsnv requested a review from canonizer November 18, 2021 00:30

levsnv added 2 commits November 23, 2021 16:13

typo

5f5ce4e

style

eea9bab

levsnv mentioned this pull request Nov 25, 2021

Universal Treelite tree walk function for FIL #4407

Merged

levsnv mentioned this pull request Dec 2, 2021

Unify dense and sparse tests #4417

Merged

dantegd removed this from PR-Needs review in v21.12 Release Dec 2, 2021

dantegd added this to PR-WIP in v22.02 Release via automation Dec 2, 2021

Merge branch 'branch-22.02' of github.com:rapidsai/cuml into unify-de…

e610175

…nse-sparse-import

wphicks requested changes Dec 16, 2021

View reviewed changes

cpp/src/fil/treelite_import.cu Outdated Show resolved Hide resolved

cpp/src/fil/treelite_import.cu Outdated Show resolved Hide resolved

cpp/src/fil/treelite_import.cu Outdated Show resolved Hide resolved

cpp/src/fil/treelite_import.cu Outdated Show resolved Hide resolved

v22.02 Release automation moved this from PR-WIP to PR-Needs review Dec 16, 2021

addressed review comments

fe38a39

v22.02 Release automation moved this from PR-Needs review to PR-Reviewer approved Dec 17, 2021

wphicks approved these changes Dec 17, 2021

View reviewed changes

levsnv added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels Dec 17, 2021

rapids-bot bot merged commit 3ae44b9 into rapidsai:branch-22.02 Dec 17, 2021

v22.02 Release automation moved this from PR-Reviewer approved to Done Dec 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify dense and sparse import in FIL #4328

Unify dense and sparse import in FIL #4328

levsnv commented Nov 4, 2021

levsnv commented Nov 11, 2021

canonizer left a comment

canonizer Nov 12, 2021

levsnv Nov 12, 2021

canonizer Nov 17, 2021

canonizer Nov 12, 2021

levsnv Nov 17, 2021

canonizer Nov 17, 2021 •

edited

levsnv commented Nov 24, 2021

levsnv commented Nov 25, 2021

levsnv commented Dec 2, 2021

levsnv commented Dec 3, 2021

dantegd commented Dec 13, 2021

levsnv commented Dec 15, 2021

levsnv commented Dec 15, 2021

wphicks left a comment

wphicks commented Dec 17, 2021

codecov-commenter commented Dec 17, 2021

wphicks commented Dec 17, 2021

Unify dense and sparse import in FIL #4328

Unify dense and sparse import in FIL #4328

Conversation

levsnv commented Nov 4, 2021

levsnv commented Nov 11, 2021

canonizer left a comment

Choose a reason for hiding this comment

canonizer Nov 12, 2021

Choose a reason for hiding this comment

levsnv Nov 12, 2021

Choose a reason for hiding this comment

canonizer Nov 17, 2021

Choose a reason for hiding this comment

canonizer Nov 12, 2021

Choose a reason for hiding this comment

levsnv Nov 17, 2021

Choose a reason for hiding this comment

canonizer Nov 17, 2021 • edited

Choose a reason for hiding this comment

levsnv commented Nov 24, 2021

levsnv commented Nov 25, 2021

levsnv commented Dec 2, 2021

levsnv commented Dec 3, 2021

dantegd commented Dec 13, 2021

levsnv commented Dec 15, 2021

levsnv commented Dec 15, 2021

wphicks left a comment

Choose a reason for hiding this comment

wphicks commented Dec 17, 2021

codecov-commenter commented Dec 17, 2021

Codecov Report

wphicks commented Dec 17, 2021

canonizer Nov 17, 2021 •

edited