Skip to content

Fix scikit-learn 1.7+ FutureWarnings in WeightedLassoCV and WeightedMultiTaskLassoCV - #1031

Merged
kbattocchi merged 1 commit into
py-why:mainfrom
justinchen033:fix-sklearn-warnings
Jun 11, 2026
Merged

Fix scikit-learn 1.7+ FutureWarnings in WeightedLassoCV and WeightedMultiTaskLassoCV#1031
kbattocchi merged 1 commit into
py-why:mainfrom
justinchen033:fix-sklearn-warnings

Conversation

@justinchen033

@justinchen033 justinchen033 commented May 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

This PR proactively addresses compatibility with scikit-learn 1.7+ by resolving deprecation and FutureWarning messages triggered by the n_alphas and alphas=None parameters in LassoCV and MultiTaskLassoCV classes.

On environments running modern versions of scikit-learn (version 1.7+ / 1.8.0), running EconML's test suite generates over 62,000 FutureWarnings in test_linear_model.py alone. This PR resolves all of these warnings and future-proofs the package ahead of the scikit-learn 1.9 release, where the deprecated parameters are scheduled to be removed entirely.

Root Cause

In scikit-learn 1.7, the n_alphas parameter in CV estimators was deprecated in favor of passing an integer directly to the alphas parameter, and setting alphas=None is deprecated. In version 1.9, n_alphas will be removed entirely, which would otherwise cause breaking errors in EconML.

Solution

We modified the constructors of WeightedLassoCV and WeightedMultiTaskLassoCV inside econml/sklearn_extensions/linear_model.py to check the scikit-learn version dynamically:

  • scikit-learn >= 1.7: We pass alphas = alphas if alphas is not None else n_alphas and omit n_alphas when calling super().__init__.
  • scikit-learn < 1.7: We fall back to the original signature to maintain full backward compatibility with older environments.

Verification Results

  1. Warning Elimination: Running pytest econml/tests/test_linear_model.py completed with 21 passed, 0 warnings (compared to 62,538 warnings previously!).
  2. Regression & Correctness: Verified model selection, RScorer, and bootstrap tests (test_rscorer.py, test_model_selection.py, test_bootstrap.py) run successfully and pass with no errors.

Developer Certificate of Origin (DCO)

All commits have been signed off (Signed-off-by) in compliance with DCO guidelines.
@kbattocchi

kbattocchi commented Jun 4, 2026

Copy link
Copy Markdown
Member

Thanks, this seems like a change worth addressing and I'm in favor of it in concept. Unfortunately, your commit includes a lot of formatting (whitespace and parenthesizing) changes that are unrelated to the core change and which make the diff hard to read. Can you create a cleaner commit that only includes the changes to the logic?
@justinchen033
justinchen033 force-pushed the fix-sklearn-warnings branch 2 times, most recently from a9de316 to 2d4cdba Compare June 6, 2026 01:08
@justinchen033
Fix scikit-learn 1.7+ FutureWarnings in WeightedLassoCV and WeightedM…
a5d3766 
…ultiTaskLassoCV

Signed-off-by: RainMaker033 <Justin.yc1818@gmail.com>
@justinchen033
justinchen033 force-pushed the fix-sklearn-warnings branch from 2d4cdba to a5d3766 Compare June 11, 2026 01:34
kbattocchi
kbattocchi approved these changes Jun 11, 2026
View reviewed changes

@kbattocchi kbattocchi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the contribution!
@kbattocchi
kbattocchi enabled auto-merge (rebase) June 11, 2026 20:33
@kbattocchi
kbattocchi merged commit e546416 into py-why:main Jun 11, 2026
114 checks passed
This was referenced Jul 16, 2026
Merged
Merged
kbattocchi added a commit that referenced this pull request Jul 16, 2026
@kbattocchi
Add sklearn-compat test helpers for wrapper invariants
4321c98 
Adds econml/tests/_sklearn_compat_helpers.py with two helpers aimed at the wrapper-around-sklearn-estimator pattern used throughout econml.sklearn_extensions:

* assert_sklearn_roundtrip(cls, **kwargs): construct the estimator, then assert that get_params() reports exactly the kwargs the user passed, that clone() preserves all params, and that re-cloning is idempotent. The kwargs-form check is what catches the PR-#1031 class of bug, where a wrapper omits an arg from its super().__init__() call and the parent silently writes a 'deprecated' sentinel onto self.

* no_sklearn_future_warnings(): context manager that promotes sklearn-originated FutureWarning/DeprecationWarning to errors, so wrapper happy-paths fail loudly when an upstream deprecation starts firing instead of contributing to a warning storm.

Both helpers are deliberately one-liner-friendly and intended to be the standard vocabulary for new sklearn-extension tests.

Also strengthens econml/tests/test_linear_model.py::test_can_clone (which previously just called clone() with no assertion) to use the new helper, and adds test_clone_preserves_explicit_kwargs as an explicit kwarg-preservation check across the Weighted*/DebiasedLasso wrappers. Both pass on currently-installed sklearn (1.4.2); on sklearn>=1.7 the latter is the test that would have caught PR #1031.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Keith Battocchi <kebatt@microsoft.com>
kbattocchi added a commit that referenced this pull request Jul 16, 2026
@kbattocchi
Document sklearn cross-version compat conventions
a2d08d6 
Adds the canonical contributor recipe for working with sklearn version differences in two places:

* econml/_sklearn_compat.py module docstring: expanded to (a) state why EconML supports a wide sklearn range (user environments + free CI coverage from the Python matrix) and (b) include a five-step recipe for wrapping a sklearn estimator across versions, with two easy-to-miss steps called out explicitly:

  1. In the newer-sklearn branch, reassign ONLY the specific deprecated arg the parent silently overwrites with a sentinel. Do NOT reassign every constructor argument unconditionally, and in particular do NOT reassign the arg that the parent stored under a translated name -- that clobbers the parent's correct value back to the caller's default. This is the bug PR #1031 introduced and PR #1042 fixed.

  2. If the parent removed the arg entirely (not just deprecated it), add a get_params override that drops the arg from the returned dict on the affected sklearn versions. Without it, sklearn's internal _get_param_names still inspects our wrapper's __init__ signature, so the removed arg leaks into path_params / lasso_path and either warns or errors depending on sklearn version. This is the pattern PR #1046 suggested.

  Includes matching code skeletons for the wrapper and its tests, pointing at the assert_sklearn_roundtrip / no_sklearn_future_warnings helpers.

  Also adds a short 'Forward-compatibility practices' section: treat every new sklearn FutureWarning / DeprecationWarning as a removal timer (the removal version is named in the message), open an issue with that version stamp, and prefer eager migration over silencing.

* README.md, new 'Working with scikit-learn version differences' subsection under 'For Developers': a short framing of the broad-compat philosophy plus a table that surveys the kinds of sklearn changes EconML has had to absorb (renamed kwargs, moved symbols, deprecation sentinels, wrapper-signature leaks, private-helper signature changes, behavior changes) with concrete codebase citations for each. Marked as a placeholder that will be split out to a dedicated CONTRIBUTING.md in a future change.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Keith Battocchi <kebatt@microsoft.com>
kbattocchi added a commit that referenced this pull request Jul 17, 2026
@kbattocchi
Add sklearn-compat test helpers for wrapper invariants
2411dbb 
Adds econml/tests/_sklearn_compat_helpers.py with two helpers aimed at the wrapper-around-sklearn-estimator pattern used throughout econml.sklearn_extensions:

* assert_sklearn_roundtrip(cls, **kwargs): construct the estimator, then assert that get_params() reports exactly the kwargs the user passed, that clone() preserves all params, and that re-cloning is idempotent. The kwargs-form check is what catches the PR-#1031 class of bug, where a wrapper omits an arg from its super().__init__() call and the parent silently writes a 'deprecated' sentinel onto self.

* no_sklearn_future_warnings(): context manager that promotes sklearn-originated FutureWarning/DeprecationWarning to errors, so wrapper happy-paths fail loudly when an upstream deprecation starts firing instead of contributing to a warning storm.

Both helpers are deliberately one-liner-friendly and intended to be the standard vocabulary for new sklearn-extension tests.

Also strengthens econml/tests/test_linear_model.py::test_can_clone (which previously just called clone() with no assertion) to use the new helper, and adds test_clone_preserves_explicit_kwargs as an explicit kwarg-preservation check across the Weighted*/DebiasedLasso wrappers. Both pass on currently-installed sklearn (1.4.2); on sklearn>=1.7 the latter is the test that would have caught PR #1031.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Keith Battocchi <kebatt@microsoft.com>
kbattocchi added a commit that referenced this pull request Jul 17, 2026
@kbattocchi
Document sklearn cross-version compat conventions
49e59fc 
Adds the canonical contributor recipe for working with sklearn version differences in two places:

* econml/_sklearn_compat.py module docstring: expanded to (a) state why EconML supports a wide sklearn range (user environments + free CI coverage from the Python matrix) and (b) include a five-step recipe for wrapping a sklearn estimator across versions, with two easy-to-miss steps called out explicitly:

  1. In the newer-sklearn branch, reassign ONLY the specific deprecated arg the parent silently overwrites with a sentinel. Do NOT reassign every constructor argument unconditionally, and in particular do NOT reassign the arg that the parent stored under a translated name -- that clobbers the parent's correct value back to the caller's default. This is the bug PR #1031 introduced and PR #1042 fixed.

  2. If the parent removed the arg entirely (not just deprecated it), add a get_params override that drops the arg from the returned dict on the affected sklearn versions. Without it, sklearn's internal _get_param_names still inspects our wrapper's __init__ signature, so the removed arg leaks into path_params / lasso_path and either warns or errors depending on sklearn version. This is the pattern PR #1046 suggested.

  Includes matching code skeletons for the wrapper and its tests, pointing at the assert_sklearn_roundtrip / no_sklearn_future_warnings helpers.

  Also adds a short 'Forward-compatibility practices' section: treat every new sklearn FutureWarning / DeprecationWarning as a removal timer (the removal version is named in the message), open an issue with that version stamp, and prefer eager migration over silencing.

* README.md, new 'Working with scikit-learn version differences' subsection under 'For Developers': a short framing of the broad-compat philosophy plus a table that surveys the kinds of sklearn changes EconML has had to absorb (renamed kwargs, moved symbols, deprecation sentinels, wrapper-signature leaks, private-helper signature changes, behavior changes) with concrete codebase citations for each. Marked as a placeholder that will be split out to a dedicated CONTRIBUTING.md in a future change.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Keith Battocchi <kebatt@microsoft.com>
kbattocchi added a commit that referenced this pull request Jul 17, 2026
@kbattocchi
Add sklearn-compat test helpers for wrapper invariants
2ff1da9 
Adds econml/tests/_sklearn_compat_helpers.py with two helpers aimed at the wrapper-around-sklearn-estimator pattern used throughout econml.sklearn_extensions:

* assert_sklearn_roundtrip(cls, **kwargs): construct the estimator, then assert that get_params() reports exactly the kwargs the user passed, that clone() preserves all params, and that re-cloning is idempotent. The kwargs-form check is what catches the PR-#1031 class of bug, where a wrapper omits an arg from its super().__init__() call and the parent silently writes a 'deprecated' sentinel onto self.

* no_sklearn_future_warnings(): context manager that promotes sklearn-originated FutureWarning/DeprecationWarning to errors, so wrapper happy-paths fail loudly when an upstream deprecation starts firing instead of contributing to a warning storm.

Both helpers are deliberately one-liner-friendly and intended to be the standard vocabulary for new sklearn-extension tests.

Also strengthens econml/tests/test_linear_model.py::test_can_clone (which previously just called clone() with no assertion) to use the new helper, and adds test_clone_preserves_explicit_kwargs as an explicit kwarg-preservation check across the Weighted*/DebiasedLasso wrappers. Both pass on currently-installed sklearn (1.4.2); on sklearn>=1.7 the latter is the test that would have caught PR #1031.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Keith Battocchi <kebatt@microsoft.com>
kbattocchi added a commit that referenced this pull request Jul 17, 2026
@kbattocchi
Document sklearn cross-version compat conventions
ea12486 
Adds the canonical contributor recipe for working with sklearn version differences in two places:

* econml/_sklearn_compat.py module docstring: expanded to (a) state why EconML supports a wide sklearn range (user environments + free CI coverage from the Python matrix) and (b) include a five-step recipe for wrapping a sklearn estimator across versions, with two easy-to-miss steps called out explicitly:

  1. In the newer-sklearn branch, reassign ONLY the specific deprecated arg the parent silently overwrites with a sentinel. Do NOT reassign every constructor argument unconditionally, and in particular do NOT reassign the arg that the parent stored under a translated name -- that clobbers the parent's correct value back to the caller's default. This is the bug PR #1031 introduced and PR #1042 fixed.

  2. If the parent removed the arg entirely (not just deprecated it), add a get_params override that drops the arg from the returned dict on the affected sklearn versions. Without it, sklearn's internal _get_param_names still inspects our wrapper's __init__ signature, so the removed arg leaks into path_params / lasso_path and either warns or errors depending on sklearn version. This is the pattern PR #1046 suggested.

  Includes matching code skeletons for the wrapper and its tests, pointing at the assert_sklearn_roundtrip / no_sklearn_future_warnings helpers.

  Also adds a short 'Forward-compatibility practices' section: treat every new sklearn FutureWarning / DeprecationWarning as a removal timer (the removal version is named in the message), open an issue with that version stamp, and prefer eager migration over silencing.

* README.md, new 'Working with scikit-learn version differences' subsection under 'For Developers': a short framing of the broad-compat philosophy plus a table that surveys the kinds of sklearn changes EconML has had to absorb (renamed kwargs, moved symbols, deprecation sentinels, wrapper-signature leaks, private-helper signature changes, behavior changes) with concrete codebase citations for each. Marked as a placeholder that will be split out to a dedicated CONTRIBUTING.md in a future change.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Keith Battocchi <kebatt@microsoft.com>
kbattocchi added a commit that referenced this pull request Jul 17, 2026
@kbattocchi
Add sklearn-compat test helpers for wrapper invariants
6670c1d 
Adds econml/tests/_sklearn_compat_helpers.py with two helpers aimed at the wrapper-around-sklearn-estimator pattern used throughout econml.sklearn_extensions:

* assert_sklearn_roundtrip(cls, **kwargs): construct the estimator, then assert that get_params() reports exactly the kwargs the user passed, that clone() preserves all params, and that re-cloning is idempotent. The kwargs-form check is what catches the PR-#1031 class of bug, where a wrapper omits an arg from its super().__init__() call and the parent silently writes a 'deprecated' sentinel onto self.

* no_sklearn_future_warnings(): context manager that promotes sklearn-originated FutureWarning/DeprecationWarning to errors, so wrapper happy-paths fail loudly when an upstream deprecation starts firing instead of contributing to a warning storm.

Both helpers are deliberately one-liner-friendly and intended to be the standard vocabulary for new sklearn-extension tests.

Also strengthens econml/tests/test_linear_model.py::test_can_clone (which previously just called clone() with no assertion) to use the new helper, and adds test_clone_preserves_explicit_kwargs as an explicit kwarg-preservation check across the Weighted*/DebiasedLasso wrappers. Both pass on currently-installed sklearn (1.4.2); on sklearn>=1.7 the latter is the test that would have caught PR #1031.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Keith Battocchi <kebatt@microsoft.com>
kbattocchi added a commit that referenced this pull request Jul 17, 2026
@kbattocchi
Document sklearn cross-version compat conventions
2b38e80 
Adds the canonical contributor recipe for working with sklearn version differences in two places:

* econml/_sklearn_compat.py module docstring: expanded to (a) state why EconML supports a wide sklearn range (user environments + free CI coverage from the Python matrix) and (b) include a five-step recipe for wrapping a sklearn estimator across versions, with two easy-to-miss steps called out explicitly:

  1. In the newer-sklearn branch, reassign ONLY the specific deprecated arg the parent silently overwrites with a sentinel. Do NOT reassign every constructor argument unconditionally, and in particular do NOT reassign the arg that the parent stored under a translated name -- that clobbers the parent's correct value back to the caller's default. This is the bug PR #1031 introduced and PR #1042 fixed.

  2. If the parent removed the arg entirely (not just deprecated it), add a get_params override that drops the arg from the returned dict on the affected sklearn versions. Without it, sklearn's internal _get_param_names still inspects our wrapper's __init__ signature, so the removed arg leaks into path_params / lasso_path and either warns or errors depending on sklearn version. This is the pattern PR #1046 suggested.

  Includes matching code skeletons for the wrapper and its tests, pointing at the assert_sklearn_roundtrip / no_sklearn_future_warnings helpers.

  Also adds a short 'Forward-compatibility practices' section: treat every new sklearn FutureWarning / DeprecationWarning as a removal timer (the removal version is named in the message), open an issue with that version stamp, and prefer eager migration over silencing.

* README.md, new 'Working with scikit-learn version differences' subsection under 'For Developers': a short framing of the broad-compat philosophy plus a table that surveys the kinds of sklearn changes EconML has had to absorb (renamed kwargs, moved symbols, deprecation sentinels, wrapper-signature leaks, private-helper signature changes, behavior changes) with concrete codebase citations for each. Marked as a placeholder that will be split out to a dedicated CONTRIBUTING.md in a future change.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Keith Battocchi <kebatt@microsoft.com>
kbattocchi added a commit that referenced this pull request Jul 17, 2026
@kbattocchi
Add sklearn-compat test helpers for wrapper invariants
34cefd8 
Adds econml/tests/_sklearn_compat_helpers.py with two helpers aimed at the wrapper-around-sklearn-estimator pattern used throughout econml.sklearn_extensions:

* assert_sklearn_roundtrip(cls, **kwargs): construct the estimator, then assert that get_params() reports exactly the kwargs the user passed, that clone() preserves all params, and that re-cloning is idempotent. The kwargs-form check is what catches the PR-#1031 class of bug, where a wrapper omits an arg from its super().__init__() call and the parent silently writes a 'deprecated' sentinel onto self.

* no_sklearn_future_warnings(): context manager that promotes sklearn-originated FutureWarning/DeprecationWarning to errors, so wrapper happy-paths fail loudly when an upstream deprecation starts firing instead of contributing to a warning storm.

Both helpers are deliberately one-liner-friendly and intended to be the standard vocabulary for new sklearn-extension tests.

Also strengthens econml/tests/test_linear_model.py::test_can_clone (which previously just called clone() with no assertion) to use the new helper, and adds test_clone_preserves_explicit_kwargs as an explicit kwarg-preservation check across the Weighted*/DebiasedLasso wrappers. Both pass on currently-installed sklearn (1.4.2); on sklearn>=1.7 the latter is the test that would have caught PR #1031.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Keith Battocchi <kebatt@microsoft.com>
kbattocchi added a commit that referenced this pull request Jul 17, 2026
@kbattocchi
Document sklearn cross-version compat conventions
62491ea 
Adds the canonical contributor recipe for working with sklearn version differences in two places:

* econml/_sklearn_compat.py module docstring: expanded to (a) state why EconML supports a wide sklearn range (user environments + free CI coverage from the Python matrix) and (b) include a five-step recipe for wrapping a sklearn estimator across versions, with two easy-to-miss steps called out explicitly:

  1. In the newer-sklearn branch, reassign ONLY the specific deprecated arg the parent silently overwrites with a sentinel. Do NOT reassign every constructor argument unconditionally, and in particular do NOT reassign the arg that the parent stored under a translated name -- that clobbers the parent's correct value back to the caller's default. This is the bug PR #1031 introduced and PR #1042 fixed.

  2. If the parent removed the arg entirely (not just deprecated it), add a get_params override that drops the arg from the returned dict on the affected sklearn versions. Without it, sklearn's internal _get_param_names still inspects our wrapper's __init__ signature, so the removed arg leaks into path_params / lasso_path and either warns or errors depending on sklearn version. This is the pattern PR #1046 suggested.

  Includes matching code skeletons for the wrapper and its tests, pointing at the assert_sklearn_roundtrip / no_sklearn_future_warnings helpers.

  Also adds a short 'Forward-compatibility practices' section: treat every new sklearn FutureWarning / DeprecationWarning as a removal timer (the removal version is named in the message), open an issue with that version stamp, and prefer eager migration over silencing.

* README.md, new 'Working with scikit-learn version differences' subsection under 'For Developers': a short framing of the broad-compat philosophy plus a table that surveys the kinds of sklearn changes EconML has had to absorb (renamed kwargs, moved symbols, deprecation sentinels, wrapper-signature leaks, private-helper signature changes, behavior changes) with concrete codebase citations for each. Marked as a placeholder that will be split out to a dedicated CONTRIBUTING.md in a future change.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Keith Battocchi <kebatt@microsoft.com>
kbattocchi added a commit that referenced this pull request Jul 23, 2026
@kbattocchi
Add sklearn-compat test helpers for wrapper invariants
3ba2b4d 
Adds econml/tests/_sklearn_compat_helpers.py with two helpers aimed at the wrapper-around-sklearn-estimator pattern used throughout econml.sklearn_extensions:

* assert_sklearn_roundtrip(cls, **kwargs): construct the estimator, then assert that get_params() reports exactly the kwargs the user passed, and that clone() preserves all params. The kwargs-form check is what catches the PR-#1031 class of bug, where a wrapper omits an arg from its super().__init__() call and the parent silently writes a 'deprecated' sentinel onto self.

* no_sklearn_future_warnings(): context manager that promotes sklearn-originated FutureWarning/DeprecationWarning to errors, so wrapper happy-paths fail loudly when an upstream deprecation starts firing instead of contributing to a warning storm.

Both helpers are deliberately one-liner-friendly and intended to be the standard vocabulary for new sklearn-extension tests.

Also strengthens econml/tests/test_linear_model.py::test_can_clone (which previously just called clone() with no assertion) to use the new helper, and adds test_clone_preserves_explicit_kwargs as an explicit kwarg-preservation check across the Weighted*/DebiasedLasso wrappers. Both pass on currently-installed sklearn (1.4.2); on sklearn>=1.7 the latter is the test that would have caught PR #1031.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Keith Battocchi <kebatt@microsoft.com>
kbattocchi added a commit that referenced this pull request Jul 23, 2026
@kbattocchi
Document sklearn cross-version compat conventions
56bbdea 
Adds the canonical contributor recipe for working with sklearn version differences in two places:

* econml/_sklearn_compat.py module docstring: expanded to (a) state why EconML supports a wide sklearn range (user environments + free CI coverage from the Python matrix) and (b) include a five-step recipe for wrapping a sklearn estimator across versions, with two easy-to-miss steps called out explicitly:

  1. In the newer-sklearn branch, reassign ONLY the specific deprecated arg the parent silently overwrites with a sentinel. Do NOT reassign every constructor argument unconditionally, and in particular do NOT reassign the arg that the parent stored under a translated name -- that clobbers the parent's correct value back to the caller's default. This is the bug PR #1031 introduced and PR #1042 fixed.

  2. If the parent removed the arg entirely (not just deprecated it), add a get_params override that drops the arg from the returned dict on the affected sklearn versions. Without it, sklearn's internal _get_param_names still inspects our wrapper's __init__ signature, so the removed arg leaks into path_params / lasso_path and either warns or errors depending on sklearn version. This is the pattern PR #1046 suggested.

  Includes matching code skeletons for the wrapper and its tests, pointing at the assert_sklearn_roundtrip / no_sklearn_future_warnings helpers.

  Also adds a short 'Forward-compatibility practices' section: treat every new sklearn FutureWarning / DeprecationWarning as a removal timer (the removal version is named in the message), open an issue with that version stamp, and prefer eager migration over silencing.

* README.md, new 'Working with scikit-learn version differences' subsection under 'For Developers': a short framing of the broad-compat philosophy plus a table that surveys the kinds of sklearn changes EconML has had to absorb (renamed kwargs, moved symbols, deprecation sentinels, wrapper-signature leaks, private-helper signature changes, behavior changes) with concrete codebase citations for each. Marked as a placeholder that will be split out to a dedicated CONTRIBUTING.md in a future change.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Keith Battocchi <kebatt@microsoft.com>
@kbattocchi kbattocchi mentioned this pull request Jul 23, 2026
Closed
kbattocchi added a commit that referenced this pull request Jul 30, 2026
@kbattocchi
Add sklearn-compat test helpers for wrapper invariants
8fda413 
Adds econml/tests/_sklearn_compat_helpers.py with two helpers aimed at the wrapper-around-sklearn-estimator pattern used throughout econml.sklearn_extensions:

* assert_sklearn_roundtrip(cls, **kwargs): construct the estimator, then assert that get_params() reports exactly the kwargs the user passed, and that clone() preserves all params. The kwargs-form check is what catches the PR-#1031 class of bug, where a wrapper omits an arg from its super().__init__() call and the parent silently writes a 'deprecated' sentinel onto self.

* no_sklearn_future_warnings(): context manager that promotes sklearn-originated FutureWarning/DeprecationWarning to errors, so wrapper happy-paths fail loudly when an upstream deprecation starts firing instead of contributing to a warning storm.

Both helpers are deliberately one-liner-friendly and intended to be the standard vocabulary for new sklearn-extension tests.

Also strengthens econml/tests/test_linear_model.py::test_can_clone (which previously just called clone() with no assertion) to use the new helper, and adds test_clone_preserves_explicit_kwargs as an explicit kwarg-preservation check across the Weighted*/DebiasedLasso wrappers. Both pass on currently-installed sklearn (1.4.2); on sklearn>=1.7 the latter is the test that would have caught PR #1031.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Keith Battocchi <kebatt@microsoft.com>
kbattocchi added a commit that referenced this pull request Jul 30, 2026
@kbattocchi
Document sklearn cross-version compat conventions
eb7b5c4 
Adds the canonical contributor recipe for working with sklearn version differences in two places:

* econml/_sklearn_compat.py module docstring: expanded to (a) state why EconML supports a wide sklearn range (user environments + free CI coverage from the Python matrix) and (b) include a five-step recipe for wrapping a sklearn estimator across versions, with two easy-to-miss steps called out explicitly:

  1. In the newer-sklearn branch, reassign ONLY the specific deprecated arg the parent silently overwrites with a sentinel. Do NOT reassign every constructor argument unconditionally, and in particular do NOT reassign the arg that the parent stored under a translated name -- that clobbers the parent's correct value back to the caller's default. This is the bug PR #1031 introduced and PR #1042 fixed.

  2. If the parent removed the arg entirely (not just deprecated it), add a get_params override that drops the arg from the returned dict on the affected sklearn versions. Without it, sklearn's internal _get_param_names still inspects our wrapper's __init__ signature, so the removed arg leaks into path_params / lasso_path and either warns or errors depending on sklearn version. This is the pattern PR #1046 suggested.

  Includes matching code skeletons for the wrapper and its tests, pointing at the assert_sklearn_roundtrip / no_sklearn_future_warnings helpers.

  Also adds a short 'Forward-compatibility practices' section: treat every new sklearn FutureWarning / DeprecationWarning as a removal timer (the removal version is named in the message), open an issue with that version stamp, and prefer eager migration over silencing.

* README.md, new 'Working with scikit-learn version differences' subsection under 'For Developers': a short framing of the broad-compat philosophy plus a table that surveys the kinds of sklearn changes EconML has had to absorb (renamed kwargs, moved symbols, deprecation sentinels, wrapper-signature leaks, private-helper signature changes, behavior changes) with concrete codebase citations for each. Marked as a placeholder that will be split out to a dedicated CONTRIBUTING.md in a future change.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Keith Battocchi <kebatt@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants

@justinchen033 @kbattocchi