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.

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

Support categorical splits in in TreeExplainer #4473

Merged
merged 44 commits into from
Jan 28, 2022

Conversation

hcho3
Copy link
Contributor

@hcho3 hcho3 commented Jan 8, 2022

Stacked on top of #4447. Do not merge until #4447 is merged first.

If you'd like to review before #4447 is merged, look at the net diff at hcho3#2.

  • Test XGBoost models with categorical splits
  • Test LightGBM models with categorical splits

@hcho3 hcho3 requested review from a team as code owners January 8, 2022 00:22
@hcho3 hcho3 marked this pull request as draft January 8, 2022 00:22
@github-actions github-actions bot added CUDA/C++ Cython / Python Cython or Python issue labels Jan 8, 2022
@RAMitchell
Copy link
Contributor

Hypothesis is already in the rapids environment, although this is maybe not the same as us being allowed to use it :)

I wrote almost all my tests for #4492 in hypothesis.

@dantegd
Copy link
Member

dantegd commented Jan 24, 2022

@hcho3 Rory's advice is correct, hypothesis already being used by cuDF, so it is already in our testing environments (and developer envs for that matter), so it should be fine to use in pytests

@hcho3
Copy link
Contributor Author

hcho3 commented Jan 24, 2022

@RAMitchell I agree that property-based testing is a good idea. Given that code freeze is in 3 days, let me add hypothesis based testing in a follow-up PR targeting 22.04.

@RAMitchell
Copy link
Contributor

Good idea, it's not urgent.

@github-actions github-actions bot removed conda conda issue gpuCI gpuCI issue CMake labels Jan 25, 2022
@ajschmidt8 ajschmidt8 removed the request for review from a team January 26, 2022 19:03
@ajschmidt8
Copy link
Member

Removing ops-codeowners from the required reviews since it doesn't seem there are any file changes that we're responsible for. Feel free to add us back if necessary.

@hcho3
Copy link
Contributor Author

hcho3 commented Jan 26, 2022

@RAMitchell @wphicks Can you take another quick look? I suggest that we merge this as long as there isn't any big blocker. We can address remaining issues in the following release, in which we'd move TreeExplainer out of experimental namespace.

@hcho3
Copy link
Contributor Author

hcho3 commented Jan 27, 2022

Rerun tests

@dantegd
Copy link
Member

dantegd commented Jan 27, 2022

rerun tests

@codecov-commenter
Copy link

Codecov Report

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

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-22.02    #4473   +/-   ##
===============================================
  Coverage                ?   85.69%           
===============================================
  Files                   ?      236           
  Lines                   ?    19338           
  Branches                ?        0           
===============================================
  Hits                    ?    16572           
  Misses                  ?     2766           
  Partials                ?        0           
Flag Coverage Δ
dask 46.52% <0.00%> (?)
non-dask 78.59% <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 3ccf77f...f97106a. Read the comment docs.

@dantegd
Copy link
Member

dantegd commented Jan 28, 2022

@RAMitchell will go ahead and merge this PR so that it makes it to release 22.02, @hcho3 could you address any pending comments and further testing in a follow up PR? Thanks!

@dantegd
Copy link
Member

dantegd commented Jan 28, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 63e738c into rapidsai:branch-22.02 Jan 28, 2022
@hcho3 hcho3 deleted the categorical_support branch January 28, 2022 00:42
vimarsh6739 pushed a commit to vimarsh6739/cuml that referenced this pull request Oct 9, 2023
Stacked on top of rapidsai#4447. Do not merge until rapidsai#4447 is merged first.

~If you'd like to review before rapidsai#4447 is merged, look at the net diff at hcho3#2.~

- [x] Test XGBoost models with categorical splits
- [x] Test LightGBM models with categorical splits

Authors:
  - Philip Hyunsu Cho (https://github.com/hcho3)

Approvers:
  - Dante Gama Dessavre (https://github.com/dantegd)

URL: rapidsai#4473
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CUDA/C++ Cython / Python Cython or Python issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants