Skip to content
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

Jump to bottom

float64 support in treelite->FIL import and Python layer #4690

Merged
merged 58 commits into from
Apr 13, 2022
Merged

float64 support in treelite->FIL import and Python layer #4690

merged 58 commits into from
Apr 13, 2022

Conversation

canonizer
Copy link
Contributor

@canonizer canonizer commented Apr 7, 2022 •
edited
Loading

float64 support in treelite->FIL import and Python layer

levsnv and others added 30 commits February 8, 2022 19:59
@levsnv
templatized node, forest and storage types
86bc6b5
@levsnv
compiles?
abfb605
@levsnv
Merge branch 'branch-22.04' of github.com:rapidsai/cuml into fp64
a5d970f
@levsnv
simplify leaf_output_t
96e2ae6
@levsnv
Merge branch 'branch-22.04' of github.com:rapidsai/cuml into fp64
ead832d
@levsnv
draft
35ad5d9
@levsnv
Merge branch 'branch-22.04' of github.com:rapidsai/cuml into fp64
df001e7
@levsnv
fixed extra/missing instantiations
f6efb88
@levsnv
Merge branch 'fp64' into func64
4bc4f99
@levsnv
style
1bebcca
@levsnv
style
aab158b
@levsnv
Merge branch 'fp64' into func64
c3f2d4f
@levsnv
removed ML::fil::init templatization, added KeyValuePair templatization
1236c52
@levsnv
style
cc946cd
@levsnv
add the old instantiations back
91ecb20
@levsnv
fixed
67f900e
@levsnv
style
c3c4deb
@canonizer
base_node::output() now compiles.
68e7f65
@canonizer
Merge branch 'branch-22.04' into dev-fil-fp64
fe2cf92
@canonizer
Fixed style.
b31069d
@canonizer
F -> real_t.
c250653
@canonizer
Small fixes.
2f658b5
@canonizer
Updated alignment.
6fb105b
@canonizer
static_assert(real_t == float) in a number of places.
f1a10be
@canonizer
noinline -> forceinline.
ce3624e
@canonizer
Updated comment.
63fadd1
@canonizer
Merge branch 'dev-fil-fp64' into enh-fil-func64
986a50d
@canonizer
Fixed many compiler errors.
fda7669
@canonizer
Multiple changes.
40f7a23
@canonizer
Fixed compilation errors; now it compiles.
12cc051
@github-actions github-actions bot added the Cython / Python Cython or Python issue label Apr 7, 2022
@canonizer canonizer changed the title float64 support in treelite->FIL import float64 support in treelite->FIL import and Python layer Apr 7, 2022
@canonizer canonizer marked this pull request as ready for review April 7, 2022 23:48
@canonizer canonizer requested review from a team as code owners April 7, 2022 23:48
hcho3
hcho3 reviewed Apr 8, 2022
View reviewed changes
Copy link
Contributor

@hcho3 hcho3 left a comment

Choose a reason for hiding this comment

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

LGTM overall. I appreciate how we avoided cluttering the API with float32 and float64 flavors by using std::variant instead.

cpp/include/cuml/fil/fil.h Outdated Show resolved Hide resolved
cpp/src/fil/infer.cu Outdated Show resolved Hide resolved
@canonizer
Copy link
Contributor Author

rerun tests

@codecov-commenter
Copy link

Codecov Report

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

@@               Coverage Diff               @@
##             branch-22.06    #4690   +/-   ##
===============================================
  Coverage                ?   84.06%           
===============================================
  Files                   ?      252           
  Lines                   ?    20356           
  Branches                ?        0           
===============================================
  Hits                    ?    17113           
  Misses                  ?     3243           
  Partials                ?        0
Flag Coverage Δ
dask 44.98% <0.00%> (?)
non-dask 77.36% <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 45ecf63...0d2c512. Read the comment docs.

@canonizer
Copy link
Contributor Author

rerun tests

@hcho3 hcho3 added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Apr 12, 2022
wphicks
wphicks reviewed Apr 13, 2022
View reviewed changes
cpp/test/sg/fil_test.cu
std::vector<std::uint32_t> left_categories;
if (dense_node.is_leaf()) {
switch (ps.leaf_algo) {
switch (this->ps.leaf_algo) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this-> necessary here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the next comment.

cpp/test/sg/fil_test.cu Show resolved Hide resolved
@wphicks
Copy link
Contributor

wphicks commented Apr 13, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 57124ce into rapidsai:branch-22.06 Apr 13, 2022
vimarsh6739 pushed a commit to vimarsh6739/cuml that referenced this pull request Oct 9, 2023
@canonizer
float64 support in treelite->FIL import and Python layer (rapidsai#4690)
94a3048 
`float64` support in treelite->FIL import and Python layer

Authors:
  - Andy Adinets (https://github.com/canonizer)
  - Levs Dolgovs (https://github.com/levsnv)

Approvers:
  - Philip Hyunsu Cho (https://github.com/hcho3)
  - William Hicks (https://github.com/wphicks)

URL: rapidsai#4690
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hcho3 hcho3 hcho3 approved these changes

@wphicks wphicks wphicks approved these changes

Assignees
No one assigned
Labels
CUDA/C++ Cython / Python Cython or Python issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@canonizer @codecov-commenter @wphicks @hcho3 @levsnv