Skip to content

Alternative scoring metrics #965

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
Closed
carl-offerfit wants to merge 25 commits into from
Closed

Alternative scoring metrics #965

carl-offerfit wants to merge 25 commits into from

Conversation

@carl-offerfit
Copy link
Contributor

@carl-offerfit carl-offerfit commented Mar 31, 2025

  • Allow specification of an alternative sklearn score function for double ML models
  • Add score_nuisances function

@carl-offerfit
Copy link
Contributor Author

carl-offerfit commented May 16, 2025

@kbattocchi Here's an update:

  1. I fixed the type hints to use the old Union approach rather than the python 3.10 |. That should fix the Python 3.8 & 3.9 errors.
  2. I added a unit test for the ML score signature validation
  3. I researched sklearn make_scorer and as far as I can tell it doesn't do any validation at all - if the function you passed to make_scorer is not valid it just fails when you try to use it. That makes me wonder if my approach was over engineered, and instead we should just let it fail. At the same time, I'm a bit attached to the approach since I spent time on it. ;-) WDYT?

@carl-offerfit
Copy link
Contributor Author

carl-offerfit commented Jun 11, 2025

@kbattocchi I fixed a bug that was cause of the test failures. Please let me know if there are still issues.

kbattocchi
kbattocchi requested changes Jul 7, 2025
View reviewed changes
Copy link
Collaborator

@kbattocchi kbattocchi left a comment

Choose a reason for hiding this comment

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

Please revert the changes to pyproject.toml; also the DCO check is still failing because of commits that weren't signed off correctly.

pyproject.toml Outdated
[tool.pytest.ini_options]
testpaths = ["econml/tests"]
addopts = "--junitxml=junit/test-results.xml -n auto --strict-markers --cov-config=pyproject.toml --cov --import-mode=importlib"
addopts = "--junitxml=junit/test-results.xml -n0 --strict-markers --cov-config=pyproject.toml --import-mode=importlib"
Copy link
Collaborator

@kbattocchi kbattocchi Jul 7, 2025

Choose a reason for hiding this comment

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

Were you just changing these to run locally? We definitely want to use xdist and get coverage results, so please revert this change. If your own test needs to run serially you can use a @pytest.mark.serial attribute to mark your method (or class).

Copy link
Contributor Author

@carl-offerfit carl-offerfit Jul 7, 2025

Choose a reason for hiding this comment

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

Yes, it was to run the tests locally. I did not mean to commit it, sorry!

KuanHaoHuang and others added 25 commits July 7, 2025 10:42
@KuanHaoHuang
Fix typos in the Customer Segmentation case study (py-why#957)
7549f76 
Signed-off-by: Kuan-Hao Huang <kuan.hao.huang.b02@gmail.com>
Signed-off-by: Carl Gold <carl.goldd@braze.com>
@kgao
update datasets host url for notebooks (py-why#959)
f231d61 
Signed-off-by: kgao <kevin.leo.gao@gmail.com>
Signed-off-by: Carl Gold <carl.goldd@braze.com>
@carl-offerfit
Copy everything from the other branch, make sure to use a --sign-off …
47c0134 
…commit

Signed-off-by: Carl Gold <carl@offerfit.ai>
Signed-off-by: Carl Gold <carl.goldd@braze.com>
@carl-offerfit
Fix: No default y,t scoring so it defaults to the models built in
214c510 
Signed-off-by: Carl Gold <carl@offerfit.ai>
Signed-off-by: Carl Gold <carl.goldd@braze.com>
@carl-offerfit
Switch to passing non sklearn scoring as a function argument
3881da5 
Use a function to verify the scoring function, whether sklearn or otherwise

Signed-off-by: Carl Gold <carl@offerfit.ai>
Signed-off-by: Carl Gold <carl.goldd@braze.com>
@carl-offerfit
Fix imperative nature of docstring
395fec7 
Signed-off-by: Carl Gold <carl@offerfit.ai>
Signed-off-by: Carl Gold <carl.goldd@braze.com>
@carl-offerfit
Update tests to use pearsonr function in the test
d9813fe 
Signed-off-by: Carl Gold <carl@offerfit.ai>
Signed-off-by: Carl Gold <carl.goldd@braze.com>
@carl-offerfit
Fix for naming of return result in test
4a4f430 
Signed-off-by: Carl Gold <carl@offerfit.ai>
Signed-off-by: Carl Gold <carl.goldd@braze.com>
@carl-offerfit
Cleaner handling of the scorer name when it is a function
a28827b 
Signed-off-by: Carl Gold <carl@offerfit.ai>
Signed-off-by: Carl Gold <carl.goldd@braze.com>
@carl-offerfit
Correct the docstring to include the other alternatives
3c5c233 
Signed-off-by: Carl Gold <carl@offerfit.ai>
Signed-off-by: Carl Gold <carl.goldd@braze.com>
@carl-offerfit
Fix type hints to be compatible with earlier Python versions
71a7a6d 
Signed-off-by: Carl Gold <carl.goldd@braze.com>
@carl-offerfit
Fix for if a scoring function is partial to np.array vs. np.ndarray (…
2d26998 
…N,1), as older pearsonr is

Signed-off-by: Carl Gold <carl.goldd@braze.com>
@carl-offerfit
Add comment
1028c72 
Signed-off-by: Carl Gold <carl.goldd@braze.com>
@carl-offerfit
Adding tests of score function validation
a309559 
Signed-off-by: Carl Gold <carl.goldd@braze.com>
@carl-offerfit
Fix the tests to pass
3946e38 
Signed-off-by: Carl Gold <carl.goldd@braze.com>
@fverac
Add sensitivity analysis methods (py-why#967)
992eca0 
Signed-off-by: Carl Gold <carl.goldd@braze.com>
Remove the validation of the score function.
01b8115 
Signed-off-by: Carl Gold <carl.goldd@braze.com>
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
28c12bc 
for more information, see https://pre-commit.ci

Signed-off-by: Carl Gold <carl.goldd@braze.com>
@fverac
minor readme fix (py-why#974)
52e5099 
Signed-off-by: Fabio Vera <fabiovera@microsoft.com>
Signed-off-by: Carl Gold <carl.goldd@braze.com>
Bugfix rscorer discrete outcome treatment (py-why#977)
5d485e3 
Signed-off-by: maartenvanhooft <mjgvanhooft@gmail.com>
Signed-off-by: Carl Gold <carl.goldd@braze.com>
Fix bug in else condition on application of squeeze
bd85963 
Signed-off-by: Carl Gold <carl.goldd@braze.com>
@fverac
add Validation docs (py-why#975)
7e7b306 
Signed-off-by: Carl Gold <carl.goldd@braze.com>
@fverac
allow missing values in X when inferencing for specific ests (py-why#982
b2eb70f 
)

Signed-off-by: Carl Gold <carl.goldd@braze.com>
@atharva-novi
ENH: _check_sample_weight dtype made kwarg in sklearn 1.7.x (py-why#980)
0164ae6 
Signed-off-by: Atharva Kelkar <akelkar@novilabs.com>
Signed-off-by: Carl Gold <carl.goldd@braze.com>
Revert the change to pyproject.toml for local unit test runs
149b10c 
Signed-off-by: Carl Gold <carl.goldd@braze.com>
@carl-offerfit
Copy link
Contributor Author

carl-offerfit commented Jul 7, 2025

@kbattocchi I think I fixed the issues with the pyproject.toml and the sign-off. Let me know if you see any other issues.

@kbattocchi
Copy link
Collaborator

kbattocchi commented Jul 9, 2025

Closing in favor of #988

@kbattocchi kbattocchi closed this Jul 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kbattocchi kbattocchi kbattocchi requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@carl-offerfit @kbattocchi @KuanHaoHuang @kgao @fverac @atharva-novi