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

Expose the secondary stopping condition for QN solver #3777

Merged
merged 5 commits into from
Apr 26, 2021
Merged

Expose the secondary stopping condition for QN solver #3777

merged 5 commits into from
Apr 26, 2021

Conversation

achirkin
Copy link
Contributor

@achirkin achirkin commented Apr 21, 2021 •
edited
Loading

  • Expose a parameter delta of the QN solver to control the loss value change stopping condition
  • Set a reasonable default for the parameter value that should keep the behavior close to sklearn in most cases

Note, this change does not expose delta to the wrapper class LogisticRegression.

Note, although this change does not break the python API, it does break the C/C++ API.

Contributes to solving #3645

@achirkin achirkin added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Apr 21, 2021
@github-actions github-actions bot added CUDA/C++ Cython / Python Cython or Python issue labels Apr 21, 2021
@achirkin achirkin requested a review from tfeher April 21, 2021 09:31
@achirkin
Copy link
Contributor Author

achirkin commented Apr 21, 2021 •
edited
Loading

The change in convergence (issue #3645 ).

Before (secondary condition disabled):
issue-3645-coefs0

After (delta = tol):
issue-3645-coefs0-fixdelta

NB: these benchmarks are done on top of #3766 and #3774

@achirkin achirkin self-assigned this Apr 21, 2021
@achirkin achirkin marked this pull request as ready for review April 21, 2021 09:42
@achirkin achirkin requested review from a team as code owners April 21, 2021 09:42
@achirkin
Copy link
Contributor Author

We also may need to readjust the default parameter value if we change again the stopping condition logic in #3766

achirkin
achirkin commented Apr 21, 2021
View reviewed changes
python/cuml/solvers/qn.pyx Outdated Show resolved Hide resolved
@achirkin achirkin force-pushed the enh-ext-qn-expose-delta branch 2 times, most recently from d05ac68 to 24684ad Compare April 22, 2021 06:17
@dantegd dantegd added the 3 - Ready for Review Ready for review by team label Apr 22, 2021
dantegd
dantegd requested changes Apr 25, 2021
View reviewed changes
Copy link
Member

@dantegd dantegd left a comment

Choose a reason for hiding this comment

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

Code looks good, just a couple of comments about docstrings

cpp/include/cuml/linear_model/glm.hpp Outdated Show resolved Hide resolved
python/cuml/solvers/qn.pyx Show resolved Hide resolved
@dantegd dantegd added 4 - Waiting on Author Waiting for author to respond to review and removed 3 - Ready for Review Ready for review by team labels Apr 25, 2021
tfeher
tfeher requested changes Apr 26, 2021
View reviewed changes
Copy link
Contributor

@tfeher tfeher left a comment

Choose a reason for hiding this comment

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

Thanks Artem for the PR! Looks good overall, just one suggestion: it would make sense to expose delta in logistic_regression.pyx as well.

python/cuml/solvers/qn.pyx Outdated Show resolved Hide resolved
python/cuml/solvers/qn.pyx Outdated Show resolved Hide resolved
@achirkin
Copy link
Contributor Author

it would make sense to expose delta in logistic_regression.pyx as well.

I can, but I am not 100% sure we should. Are we sure any other solvers in future will have this parameter as well? We do hide some parameters already, e.g. lbfgs_memory. Also sklearn does not expose this parameter too.

@codecov-commenter
Copy link

Codecov Report

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

Impacted file tree graph

@@              Coverage Diff               @@
##             branch-0.20    #3777   +/-   ##
==============================================
  Coverage               ?   85.96%           
==============================================
  Files                  ?      225           
  Lines                  ?    17000           
  Branches               ?        0           
==============================================
  Hits                   ?    14614           
  Misses                 ?     2386           
  Partials               ?        0
Flag Coverage Δ
dask 48.91% <0.00%> (?)
non-dask 77.81% <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 2f0af34...3066fd2. Read the comment docs.

@achirkin achirkin changed the title Expose the secondary stopping condition for QN solver (logistic regression) Expose the secondary stopping condition for QN solver Apr 26, 2021
@achirkin achirkin added 3 - Ready for Review Ready for review by team and removed 4 - Waiting on Author Waiting for author to respond to review labels Apr 26, 2021
@achirkin achirkin requested review from dantegd and tfeher April 26, 2021 11:40
tfeher
tfeher approved these changes Apr 26, 2021
View reviewed changes
Copy link
Contributor

@tfeher tfeher left a comment

Choose a reason for hiding this comment

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

Thanks Artem for updating the PR title and description. Just to note down our offline discussion: you recommend to keep the LogisticRegression API unchanged (not exposing delta there) because:

Sounds fair to me.

@achirkin achirkin mentioned this pull request Apr 26, 2021
Merged
@dantegd
Copy link
Member

dantegd commented Apr 26, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit f71cbc1 into rapidsai:branch-0.20 Apr 26, 2021
@achirkin achirkin mentioned this pull request Apr 27, 2021
Closed
@tfeher tfeher mentioned this pull request May 7, 2021
Closed
vimarsh6739 pushed a commit to vimarsh6739/cuml that referenced this pull request Oct 9, 2023
@achirkin
Expose the secondary stopping condition for QN solver (rapidsai#3777)
93fed4d 
- Expose a parameter `delta` of the `QN` solver to control the loss value change stopping condition
 - Set a reasonable default for the parameter value that should keep the behavior close to sklearn in most cases

Note, this change does not expose `delta` to the wrapper class `LogisticRegression`.

Note, although this change does not break the python API, it does break the C/C++ API.

Contributes to solving rapidsai#3645

Authors:
  - Artem M. Chirkin (https://github.com/achirkin)

Approvers:
  - Tamas Bela Feher (https://github.com/tfeher)
  - Dante Gama Dessavre (https://github.com/dantegd)

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

@tfeher tfeher tfeher approved these changes

@dantegd dantegd dantegd approved these changes

Assignees

@achirkin achirkin

Labels
3 - Ready for Review Ready for review by team 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.
4 participants
@achirkin @codecov-commenter @dantegd @tfeher