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

Add causalml support #3273

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

alexander-pv
Copy link
Contributor

@alexander-pv alexander-pv commented Sep 23, 2023

Overview

Hi!
This PR is reopened #2654

Checklist

  • All pre-commit checks pass.
  • Unit tests added (if fixing a bug or adding a new feature)

@connortann
Copy link
Collaborator

connortann commented Sep 26, 2023

The tests are failing due to an unrelated HuggingFace error: apologies. We'll get that sorted asap (on #3286), and then we can get this PR passing.

Edit: done.

@codecov
Copy link

codecov bot commented Sep 27, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8130ec4) 57.79% compared to head (61a74f0) 57.75%.
Report is 71 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3273      +/-   ##
==========================================
- Coverage   57.79%   57.75%   -0.05%     
==========================================
  Files          89       89              
  Lines       12540    12528      -12     
==========================================
- Hits         7248     7236      -12     
  Misses       5292     5292              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@connortann
Copy link
Collaborator

connortann commented Sep 27, 2023

This PR has quite a large diff, as there are many unrelated style changes alongside the functional changes. Please would you remove the style changes, leaving just the functional changes?

We plan to introduce python black soon to remove this kind of hassle for contributors.

Nb. I inadvertently briefly closed this PR (currently on mobile), apologies.

@connortann connortann closed this Sep 27, 2023
@connortann connortann reopened this Sep 27, 2023
@alexander-pv
Copy link
Contributor Author

alexander-pv commented Sep 28, 2023

Apologies for style change. I will remove it.

Python black usage would be great!

@thatlittleboy
Copy link
Collaborator

Hi @alexander-pv , do you think you could also add in some tests to test_tree.py (just a simple function or two to demonstrate that the proposed changes here work as expected?)

Thanks!

@connortann connortann added the enhancement Indicates new feature requests label Oct 4, 2023
@connortann connortann added the awaiting feedback Indicates that further information is required from the issue creator label Dec 4, 2023
@connortann
Copy link
Collaborator

I'll add the "stale" label for now as there hasn't been recent activity. Please feel free to comment if you're still interested in the PR and are happy to work on adding a minimal unit test.

@connortann connortann added stale Indicates that there has been no recent activity on an issue and removed stale Indicates that there has been no recent activity on an issue labels Dec 4, 2023
@alexander-pv
Copy link
Contributor Author

Hi, @connortann! I added several test to check if shap works as intended with causalml trees.

@alexander-pv
Copy link
Contributor Author

About current scikit-learn building issue in recent tests: scikit-learn/scikit-learn#26858

@connortann
Copy link
Collaborator

Apologies that we haven't had time to look further at this PR, as many maintainers are away over the Christmas period. Thanks again for the PR - I hope to get to this early in the new year.

@connortann connortann added this to the 0.45.0 milestone Dec 21, 2023
@github-actions github-actions bot removed the stale Indicates that there has been no recent activity on an issue label Feb 13, 2024
@connortann connortann modified the milestones: 0.45.0, 0.46.0 Mar 4, 2024
@alexander-pv
Copy link
Contributor Author

Hi, @connortann! I updated the PR resolving a couple of conflicts. Tests work as expected on my computer.
So, awaiting feedback from label can be removed now. I will be glad to contribute if you think anything else is needed for this PR!

Thanks!

@jeongyoonlee
Copy link

Hi, @connortann. Can you take a look at this PR? This has been a long-standing issue (e.g., uber/causalml#265, uber/causalml#527, uber/causalml#615, uber/causalml#727, uber/causalml#735) for the causalml package, and we hope to resolve it soon.

Your support will be greatly appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting feedback Indicates that further information is required from the issue creator enhancement Indicates new feature requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants