[REVIEW] Add get_params() to TargetEncoder #4588
Conversation
sync with upstream
sync with upstream
sync with upstream
Sync with upstream
sync with upstream
sync with upstream
merge with upstream
sync with upstream
daxiongshu
added
non-breaking
|
I ended up not subclassing |
daxiongshu
changed the title
Codecov Report
@@ Coverage Diff @@
## branch-22.04 #4588 +/- ##
===============================================
Coverage ? 83.88%
===============================================
Files ? 250
Lines ? 20061
Branches ? 0
===============================================
Hits ? 16828
Misses ? 3233
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thanks for working on this! It looks good to me. Would be interesting, in a follow-up PR, to make this class an estimator by inheriting from the Base class (see estimator guide).
| def get_params(self): | ||
| """ | ||
| Returns a dict of all params owned by this class. | ||
| """ | ||
| params = dict() | ||
| variables = self.get_param_names() | ||
| for key in variables: | ||
| var_value = getattr(self, key, None) | ||
| params[key] = var_value | ||
| return params |
There was a problem hiding this comment.
Should be inheritable from the cuml.common.base.Base class.
There was a problem hiding this comment.
Thank you for the review. Yes, I tried inheriting base in the first place but I ended up making more changes than necessary to pass test_base.py. I'll try again in another PR.
|
@gpucibot merge |
This is the continuation of PR #4588 to resolve issue #4574 Authors: - Jiwei Liu (https://github.com/daxiongshu) Approvers: - Dante Gama Dessavre (https://github.com/dantegd) URL: #4601
This PR adds `get_params()` member function to `TargetEncoder`. Hopefully it can resolve the issue rapidsai#4574 Authors: - Jiwei Liu (https://github.com/daxiongshu) Approvers: - Victor Lafargue (https://github.com/viclafargue) - Dante Gama Dessavre (https://github.com/dantegd) URL: rapidsai#4588
…#4601) This is the continuation of PR rapidsai#4588 to resolve issue rapidsai#4574 Authors: - Jiwei Liu (https://github.com/daxiongshu) Approvers: - Dante Gama Dessavre (https://github.com/dantegd) URL: rapidsai#4601
This PR adds
get_params()member function toTargetEncoder. Hopefully it can resolve the issue #4574