Refactor struct Model -> class Model + class ModelImpl (Part of #196) #201
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hcho3
force-pushed
the
new_model_rep2
branch
from
253568f to
a56e2d9
Compare
hcho3
changed the title
Codecov Report
@@ Coverage Diff @@
## multi_type_refactor_breakup #201 +/- ##
=================================================================
+ Coverage 82.28% 82.79% +0.50%
- Complexity 54 56 +2
=================================================================
Files 85 85
Lines 5657 5079 -578
Branches 48 48
=================================================================
- Hits 4655 4205 -450
+ Misses 974 845 -129
- Partials 28 29 +1
Continue to review full report at Codecov.
|
canonizer
approved these changes
Sep 17, 2020
This was referenced Sep 18, 2020
Create template classes for model representations to support multiple data types (Part of #196)
#198
Merged
8 tasks
hcho3
added a commit
that referenced
this pull request
Oct 9, 2020
…#201) * Upgrade C++ standard to C++14. * Split struct Model into class Model and class ModelImpl. The ModelImpl class will soon become a template class in order to hold Tree objects with uint32, float32, or float64 type. The Model class will become an abstract class so as to avoid exposing ModelImpl to external interface. (It's very hard to pass template classes through a FFI boundary.) * Change signature of methods that return Model, since Model is now an abstract class. These functions now return std::unique_ptr<Model>. * Move bodies of tiny methods from tree_impl.h to tree.h. This will reduce verbosity once ModelImpl becomes a template class. Co-authored-by: Andy Adinets <aadinets@nvidia.com>
hcho3
added a commit
that referenced
this pull request
Oct 9, 2020
… data types (Part of #196) (#198) * The ModelImpl class (created in #201) becomes the template class ModelImpl<ThresholdType, LeafOutputType>. * Implement template classes ModelImpl<ThresholdType, LeafOutputType> and TreeImpl<ThresholdType, LeafOutputType> that contain the details of the tree ensemble model. The template classes are parameterized by the types of the thresholds and leaf outputs. Currently the following combinations are allowed: | Threshold type | Leaf output type | |----------------|------------------| | float32 | float32 | | float32 | uint32 | | float64 | float64 | | float64 | uint32 | |----------------|------------------| * Revise the zero-copy serialization protocol, to prepend the type information (threshold_type, leaf_output_type) to the serialized types so that the recipient will choose the correct ModelImpl<ThresholdType, LeafOutputType> to deserialize to. * A run-time type dispatching system using the enum type TypeInfo. Users are able to dispatch a correct version of ModelImpl<ThresholdType, LeafOutputType> by specifying a pair of TypeInfo values. We also implement a set of convenient functions, such as InferTypeInfoOf<T> that converts the template arg T into TypeInfo enum. Co-authored-by: William Hicks <wphicks@users.noreply.github.com> Co-authored-by: Andy Adinets <aadinets@nvidia.com>
hcho3
added a commit
that referenced
this pull request
Oct 9, 2020
Addresses #95 and #111. Follow-up to #198, #199, #201 Trying again, since #130 failed. This time, I made the Model class to be polymorphic. This way, the amount of pointer indirection is minimized. Summary: Model is an opaque container that wraps the polymorphic handle ModelImpl<ThresholdType, LeafOutputType>. The handle in turn stores the list of trees Tree<ThresholdType, LeafOutputType>. To unbox the Model container and obtain ModelImpl<ThresholdType, LeafOutputType>, use Model::Dispatch(<lambda expression>). Also, upgrade to C++14 to access the generic lambda feature, which proved to be very useful in the dispatching logic for the polymorphic Model class. * Turn the Model and Tree classes into template classes * Revise the string templates so that correct data types are used in the generated C code * Rewrite the model builder class * Revise the zero-copy serializer * Create an abstract matrix class that supports multiple data types (float32, float64 for now). * Move the DMatrix class to the runtime. * Extend the DMatrix class so that it can hold float32 and float64. * Redesign the C runtime API using the DMatrix class. * Ensure accuracy of scikit-learn models. To achieve the best results, use float32 for the input matrix and float64 for the split thresholds and leaf outputs. * Revise the JVM runtime.
This was referenced Oct 9, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Extracted from #198, which was extracted from #196
Per #198 (comment), I created a separate pull request that just consists of the refactoring of
struct Modelintoclass Modelandclass ModelImpl. I also moved small functions' definitions back totree.hheader.List of changes:
struct Modelintoclass Modelandclass ModelImpl. TheModelImplclass will soon become a template class in order to holdTreeobjects with uint32, float32, or float64 type. TheModelclass will become an abstract class so as to avoid exposingModelImplto external interface. (It's very hard to pass template classes through a FFI boundary.)Model, sinceModelis now an abstract class. These functions now returnstd::unique_ptr<Model>.tree_impl.htotree.h. This will reduce verbosity onceModelImplbecomes a template class.@canonizer