-
Notifications
You must be signed in to change notification settings - Fork 514
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
Refactor QN solver: pass parameters via a POD struct #4511
Conversation
@achirkin since burn down started last Thursday, would you mind targeting this PR to 22.04 branch? Thanks! |
Sure, sorry! |
rerun tests |
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.
The resulting code looks very clean. Mostly small things from my end, but we should strongly consider templating the indexing types sooner rather than later (we are planning to have this for all of the cuml estimators eventually but incremental improvement helps).
rerun tests |
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.
Thanks @achirkin for this PR, it is great to see these simplifications in the QN solver interface!
I have one request about improving our tests w.r.t penalty_normalized
, otherwise it looks good.
Co-authored-by: Tamas Bela Feher <tfeher@nvidia.com>
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.
Thanks for the update, looks good to me.
rerun tests |
rerun tests |
Codecov Report
@@ Coverage Diff @@
## branch-22.04 #4511 +/- ##
===============================================
Coverage ? 85.55%
===============================================
Files ? 236
Lines ? 19402
Branches ? 0
===============================================
Hits ? 16600
Misses ? 2802
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
@gpucibot merge |
This PR packs the arguments of `qnFit` and others into a C-compatible structure and changes a few other things to reduce the amount of repeated code. In addition, it exposes one new parameter `penalty_normalized`, which is needed to improve compatibility with other solvers (inside and outside cuml). Authors: - Artem M. Chirkin (https://github.com/achirkin) Approvers: - Tamas Bela Feher (https://github.com/tfeher) - Corey J. Nolet (https://github.com/cjnolet) URL: rapidsai#4511
This PR packs the arguments of
qnFit
and others into a C-compatible structure and changes a few other things to reduce the amount of repeated code. In addition, it exposes one new parameterpenalty_normalized
, which is needed to improve compatibility with other solvers (inside and outside cuml).