-
Notifications
You must be signed in to change notification settings - Fork 527
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
Split treelite fil import from forest
object definition
#4306
Split treelite fil import from forest
object definition
#4306
Conversation
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.
CMake changes LGTM
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.
Beautiful! 😄
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.
Great work!
Approved, provided that the comments (few and technical) are addressed.
|
||
#include "common.cuh" | ||
#include "common.cuh" // for predict_params, sparse_storage, dense_storage |
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.
Great work tracking down the symbols used from each include file!
I wonder, though, how much extra work would it be to maintain these lists?
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.
If we have this in place, it provides a starting place for later integration of automated tooling to maintain it. IWYU does this, and (assuming nothing changes with IWYU cuda support), we might slowly build up scripts with cppclean and ctags (similar to the work that @levsnv has already begun) that would provide similar functionality in the long term. I very much appreciate having them there given our efforts to clean up includes and improve build time, and I'm happy to manually maintain them in any of my changes until we have better tooling.
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.
Since I've shared the scripts, this is how a typical output looks like: fil.cu tag-header pair candidates
135 lines for fil.cu. After merging branch-21.12, the (dwdiff --color) highlights only two new terms:
cats_present common.cuh 44;" function line:44 struct:ML::fil::storage_base signature:() const
cats_present internal.cuh 329;" function line:329 struct:ML::fil::categorical_sets signature:() const
The rest are line number changes. Someone just needs to wrap this into a bash one-liner.
Co-authored-by: Andy Adinets <adinetz@gmail.com>
…eelite-fil-import
…to split-treelite-fil-import
Codecov Report
@@ Coverage Diff @@
## branch-21.12 #4306 +/- ##
===============================================
Coverage ? 86.06%
===============================================
Files ? 231
Lines ? 18702
Branches ? 0
===============================================
Hits ? 16095
Misses ? 2607
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
Blocked because cannot merge other PRs to FIL as that would complicate verifying that this PR mostly moves lines of code around. |
removed "Blocked" due to build issues on this PR |
@gpucibot merge |
) To improve readability of the code, split `fil.cu` in two files. Clean up transitive includes along the way. This change only moves code (LoC) around and then edits header include list to comply with IWYU. To verify that no other lines have been altered by this PR, one can: ``` git remote add levs git@github.com:levsnv/cuml.git git fetch levs cd cpp/src/fil git checkout 33e14c8 fil.cu cp fil.cu old-fil.cu git reset --hard git checkout levs/split-treelite-fil-import diff <(cat old-fil.cu | sort) <(cat treelite_import.cu fil.cu | sort) ``` This will also show renaming `4` to `MAX_N_ITEMS`, which happened in a different PR in the meantime. Authors: - Levs Dolgovs (https://github.com/levsnv) Approvers: - Robert Maynard (https://github.com/robertmaynard) - William Hicks (https://github.com/wphicks) - Andy Adinets (https://github.com/canonizer) - Dante Gama Dessavre (https://github.com/dantegd) URL: rapidsai#4306
To improve readability of the code, split
fil.cu
in two files. Clean up transitive includes along the way.This change only moves code (LoC) around and then edits header include list to comply with IWYU.
To verify that no other lines have been altered by this PR, one can:
This will also show renaming
4
toMAX_N_ITEMS
, which happened in a different PR in the meantime.