Skip to content

fix: complete sklearn 1.9 support — drop self.alphas overwrite (closes #1032) - #1042

Merged
kbattocchi merged 1 commit into
py-why:mainfrom
immu4989:fix-1032-sklearn-1.9-n_alphas
Jul 16, 2026
Merged

fix: complete sklearn 1.9 support — drop self.alphas overwrite (closes #1032)#1042
kbattocchi merged 1 commit into
py-why:mainfrom
immu4989:fix-1032-sklearn-1.9-n_alphas

Conversation

@immu4989

Copy link
Copy Markdown
Contributor

Closes #1032.

What's happening

#1032 (filed by @jakevdp) reports that import econml.orf errors on scikit-learn 1.9 because n_alphas was removed from LassoCV.__init__.

The recent commit e546416 added a >= 1.7 version dispatch in WeightedLassoCV.__init__ / WeightedMultiTaskLassoCV.__init__ that translates n_alphas=<int> into alphas=<int> on the super().__init__() call. That fixes the import-time TypeError jakevdp reported.

But e546416 also added self.alphas = alphas at the end of the dispatch, which clobbers the value sklearn's __init__ had correctly recorded back to the constructor's alphas kwarg (None by default):

if parse(sklearn.__version__) >= parse("1.7"):
    super().__init__(
        eps=eps, alphas=alphas if alphas is not None else n_alphas,  # alphas=100 ✓
        ...)
    self.n_alphas = n_alphas
    self.alphas = alphas    # ← overwrites sklearn's alphas=100 back to None

On sklearn 1.7–1.8 the loose param-validation tolerated the resulting self.alphas = None. On sklearn 1.9 the stricter _param_validation rejects it, so SparseLinearDML.fit (via _DebiasedLasso.fitWeightedLassoCV) and DebiasedLasso.fit raise:

sklearn.utils._param_validation.InvalidParameterError:
The 'alphas' parameter of WeightedLassoCV must be an int in the range [1, inf)
or an array-like. Got None instead.

Fix

Drop the overwrite. super().__init__(...) already records self.alphas from the translated value. self.n_alphas is still set so callers can introspect the original wrapper kwarg.

Same edit applied to both WeightedLassoCV.__init__ and WeightedMultiTaskLassoCV.__init__.

Also bumps pyproject.toml scikit-learn >= 1.0, < 1.9< 1.10 so users can actually install sklearn 1.9.

Verification (locally, sklearn 1.9.0 + narwhals)

  • import econml.orf ✓ (jakevdp's exact repro)
  • Smoke fits all succeed on sklearn 1.9: LinearDML, SparseLinearDML, CausalForestDML, DMLOrthoForest, LinearDRLearner, SparseLinearDRLearner
  • New test_default_alphas_fits_on_strict_sklearn regression test in TestLassoExtensions:
    • Asserts WeightedLassoCV().alphas is not None and WeightedMultiTaskLassoCV().alphas is not None (the dispatched value must survive)
    • Calls .fit(...) on each, verifying sklearn 1.9 strict validation accepts the result
    • Pre-fix on sklearn 1.9: fails with AssertionError: WeightedLassoCV.alphas was clobbered to None by __init__ (#1032)
    • Post-fix: passes
  • Broader sweep: 48 passed, 0 failed across test_linear_model.py + test_dml.py + test_treatment_featurization.py on sklearn 1.9.

…y#1032)

py-why#1032 reports econml errors on import with scikit-learn 1.9. e546416
("Fix scikit-learn 1.7+ FutureWarnings ...") added a >=1.7 dispatch in
WeightedLassoCV / WeightedMultiTaskLassoCV that translates
n_alphas=<int> into alphas=<int> on the super().__init__() call, which
fixes the import-time TypeError. But it then ran self.alphas = alphas
at the end of the dispatch, overwriting the value sklearn's __init__
had correctly recorded back to the constructor's alphas kwarg (None by
default).

On sklearn 1.7-1.8 the loose param-validation tolerated the resulting
self.alphas = None. sklearn 1.9's stricter _param_validation rejects it,
so SparseLinearDML.fit (via _DebiasedLasso.fit -> WeightedLassoCV) and
DebiasedLasso.fit raise InvalidParameterError.

Drop the overwrite. super().__init__(...) already records self.alphas
from the translated value. self.n_alphas is still set so callers can
introspect the original wrapper kwarg.

Also bump scikit-learn pin from < 1.9 to < 1.10 so 1.9 installs.
Adds test_default_alphas_fits_on_strict_sklearn covering both
WeightedLassoCV and WeightedMultiTaskLassoCV; verifies on sklearn 1.9
locally that 48 tests pass across test_linear_model, test_dml, and
test_treatment_featurization (no regressions).

Signed-off-by: Imran Ahamed <immu4989@gmail.com>

@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 clean root-cause analysis and the regression test — this is the smallest correct fix for #1032, and I appreciate that you traced the failure back to the self.alphas = alphas overwrite introduced by #1031 rather than just papering over the InvalidParameterError.

Two follow-ups I'm planning as separate commits (on top of this) so I'm not blocking the merge here:

  1. Factor out the shared kwargs dict so both __init__ methods stop duplicating the whole super().__init__(...) body across the version dispatch.

  2. Add a get_params() override on both wrappers to drop n_alphas on sklearn >= 1.7. Without this, self.get_params() still returns n_alphas (because sklearn's _get_param_names inspects our wrapper's __init__ signature, not the parent's), which leaks into lasso_path(**path_params) inside LinearModelCV.fit. On 1.7-1.10 that emits a FutureWarning storm (once per CV fold); on 1.11 n_alphas will be removed and it will become a TypeError. Proactively silencing shields us from both.

Approving.

@kbattocchi

Copy link
Copy Markdown
Member

Reopening to trigger a fresh CI run — the previous run is >30 days old, so GitHub's "Re-run failed jobs" is unavailable and manual workflow_dispatch runs don't update the PR's check status.

@kbattocchi kbattocchi closed this Jul 16, 2026
@kbattocchi kbattocchi reopened this Jul 16, 2026
@kbattocchi
kbattocchi merged commit 988d8c9 into py-why:main Jul 16, 2026
342 of 348 checks passed
kbattocchi added a commit that referenced this pull request Jul 16, 2026
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 16, 2026
…has FutureWarnings

Layered on top of #1042 (which fixed the immediate `InvalidParameterError`
on sklearn 1.9). Two follow-on changes here, both self-contained:

1. Replace the inline `parse(sklearn.__version__) >= parse('1.7')` version
   dispatch in `WeightedLassoCV.__init__` and
   `WeightedMultiTaskLassoCV.__init__` with the `SKLEARN_GE_17` flag from
   `econml/_sklearn_compat.py`, and factor the shared kwargs into a
   `common_kwargs` dict so only the `(alphas, n_alphas)` positions branch
   across the version dispatch. Removes ~20 lines of duplicated
   `super().__init__(...)` body per class.

2. Add a `get_params()` override on both wrappers that drops `n_alphas`
   from the returned dict on sklearn >= 1.7. Without this override,
   `self.get_params()` still returns `n_alphas` because sklearn's
   `_get_param_names` inspects our wrapper's `__init__` signature (which
   still lists `n_alphas` for backwards compat), not the parent's. That
   leaks into `lasso_path(**path_params)` inside sklearn's
   `LinearModelCV.fit` and triggers a per-CV-fold `FutureWarning` on
   sklearn 1.7-1.10; on sklearn 1.11 `n_alphas` will be removed from
   `lasso_path` and it will become a hard `TypeError`. Silencing here is
   both cosmetic (no warning storm) and forward-looking (protects us from the
   eventual removal).

The `get_params` override idea was originally suggested in #1046 (closed
as superseded by #1042 for the crash fix); credit to @genrichez for
identifying it.

Also adds `test_no_sklearn_future_warnings_on_happy_path_fit` to
`TestLassoExtensions`, using the `no_sklearn_future_warnings()` helper.
Without the `get_params` override this test would fail on any sklearn
between 1.7 and 1.10 inclusive; without the `self.alphas = alphas` fix in
#1042 it would fail on sklearn 1.9 with the `InvalidParameterError`. With
both in place it passes on the current pinned sklearn (1.4.2) and on a
scratch venv with scikit-learn 1.9.0.

Verified manually on sklearn 1.9.0 in a scratch venv:
- `import econml.orf` succeeds (jakevdp's exact repro from #1032)
- `WeightedLassoCV(cv=3).fit(X, y)` emits 0 FutureWarnings
- `WeightedMultiTaskLassoCV(cv=3).fit(X, y_2d)` emits 0 FutureWarnings
- `clone(WeightedLassoCV(cv=3, n_alphas=5))` produces a functionally
  equivalent estimator (clone.alphas=5; clone.n_alphas resets to the default
  100, but that attribute is no longer used at fit time)

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
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
…has FutureWarnings

Layered on top of #1042 (which fixed the immediate `InvalidParameterError`
on sklearn 1.9). Two follow-on changes here, both self-contained:

1. Replace the inline `parse(sklearn.__version__) >= parse('1.7')` version
   dispatch in `WeightedLassoCV.__init__` and
   `WeightedMultiTaskLassoCV.__init__` with the `SKLEARN_GE_17` flag from
   `econml/_sklearn_compat.py`, and factor the shared kwargs into a
   `common_kwargs` dict so only the `(alphas, n_alphas)` positions branch
   across the version dispatch. Removes ~20 lines of duplicated
   `super().__init__(...)` body per class.

2. Add a `get_params()` override on both wrappers that drops `n_alphas`
   from the returned dict on sklearn >= 1.7. Without this override,
   `self.get_params()` still returns `n_alphas` because sklearn's
   `_get_param_names` inspects our wrapper's `__init__` signature (which
   still lists `n_alphas` for backwards compat), not the parent's. That
   leaks into `lasso_path(**path_params)` inside sklearn's
   `LinearModelCV.fit` and triggers a per-CV-fold `FutureWarning` on
   sklearn 1.7-1.10; on sklearn 1.11 `n_alphas` will be removed from
   `lasso_path` and it will become a hard `TypeError`. Silencing here is
   both cosmetic (no warning storm) and forward-looking (protects us from the
   eventual removal).

The `get_params` override idea was originally suggested in #1046 (closed
as superseded by #1042 for the crash fix); credit to @genrichez for
identifying it.

Also adds `test_no_sklearn_future_warnings_on_happy_path_fit` to
`TestLassoExtensions`, using the `no_sklearn_future_warnings()` helper.
Without the `get_params` override this test would fail on any sklearn
between 1.7 and 1.10 inclusive; without the `self.alphas = alphas` fix in
#1042 it would fail on sklearn 1.9 with the `InvalidParameterError`. With
both in place it passes on the current pinned sklearn (1.4.2) and on a
scratch venv with scikit-learn 1.9.0.

Verified manually on sklearn 1.9.0 in a scratch venv:
- `import econml.orf` succeeds (jakevdp's exact repro from #1032)
- `WeightedLassoCV(cv=3).fit(X, y)` emits 0 FutureWarnings
- `WeightedMultiTaskLassoCV(cv=3).fit(X, y_2d)` emits 0 FutureWarnings
- `clone(WeightedLassoCV(cv=3, n_alphas=5))` produces a functionally
  equivalent estimator (clone.alphas=5; clone.n_alphas resets to the default
  100, but that attribute is no longer used at fit time)

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
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
…cy n_alphas compatibility

Layered on top of #1042, which fixed sklearn 1.9's immediate
InvalidParameterError. This follow-up centralizes and completes the
cross-version handling for WeightedLassoCV and WeightedMultiTaskLassoCV.

Replace inline sklearn version parsing with SKLEARN_GE_17 and factor the
shared parent-constructor kwargs so only the alphas/n_alphas translation
branches across versions.

Keep the wrappers' legacy n_alphas argument usable without leaking it into
sklearn internals:

- Translate n_alphas=<int> to alphas=<int> on sklearn >= 1.7.
- Preserve sklearn 1.7-1.8's "deprecated" sentinel in self.n_alphas so the
  parent's fit-time deprecation check stays quiet.
- Restore that same sentinel on sklearn 1.9+, where the parent removed the
  attribute, because BaseEstimator.get_params() still inspects our static
  backwards-compatible __init__ signature and expects the attribute to
  exist.
- Filter n_alphas from get_params() before sklearn's internal path_params
  can pass it to lasso_path.
- Pair that filter with a set_params() override that maps the legacy name
  to alphas, preserving GridSearchCV and Pipeline parameter grids.
- Emit one EconML FutureWarning when users explicitly select the legacy
  alias, nudging them toward alphas=<int> without allowing sklearn to emit
  one warning per CV fold.

The get_params/set_params pattern was originally suggested in #1046;
credit to @genrichez for identifying it.

Tests cover warning-free default fits, the clone/set_params/fit path,
wrapper-level deprecation warnings, and #1042's strict-sklearn regression.
The implementation was verified against sklearn 1.4.2, 1.8.0, and 1.9.0.

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
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
…cy n_alphas compatibility

Layered on top of #1042, which fixed sklearn 1.9's immediate
InvalidParameterError. This follow-up centralizes and completes the
cross-version handling for WeightedLassoCV and WeightedMultiTaskLassoCV.

Replace inline sklearn version parsing with SKLEARN_GE_17 and factor the
shared parent-constructor kwargs so only the alphas/n_alphas translation
branches across versions.

Keep the wrappers' legacy n_alphas argument usable without leaking it into
sklearn internals:

- Translate n_alphas=<int> to alphas=<int> on sklearn >= 1.7.
- Preserve sklearn 1.7-1.8's "deprecated" sentinel in self.n_alphas so the
  parent's fit-time deprecation check stays quiet.
- Restore that same sentinel on sklearn 1.9+, where the parent removed the
  attribute, because BaseEstimator.get_params() still inspects our static
  backwards-compatible __init__ signature and expects the attribute to
  exist.
- Filter n_alphas from get_params() before sklearn's internal path_params
  can pass it to lasso_path.
- Pair that filter with a set_params() override that maps the legacy name
  to alphas, preserving GridSearchCV and Pipeline parameter grids.
- Emit one EconML FutureWarning when users explicitly select the legacy
  alias, nudging them toward alphas=<int> without allowing sklearn to emit
  one warning per CV fold.

The get_params/set_params pattern was originally suggested in #1046;
credit to @genrichez for identifying it.

Tests cover warning-free default fits, the clone/set_params/fit path,
wrapper-level deprecation warnings, and #1042's strict-sklearn regression.
The implementation was verified against sklearn 1.4.2, 1.8.0, and 1.9.0.

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
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
…cy n_alphas compatibility

Layered on top of #1042, which fixed sklearn 1.9's immediate
InvalidParameterError. This follow-up centralizes and completes the
cross-version handling for WeightedLassoCV and WeightedMultiTaskLassoCV.

Replace inline sklearn version parsing with SKLEARN_GE_17 and factor the
shared parent-constructor kwargs so only the alphas/n_alphas translation
branches across versions.

Keep the wrappers' legacy n_alphas argument usable without leaking it into
sklearn internals:

- Translate n_alphas=<int> to alphas=<int> on sklearn >= 1.7.
- Preserve sklearn 1.7-1.8's "deprecated" sentinel in self.n_alphas so the
  parent's fit-time deprecation check stays quiet.
- Restore that same sentinel on sklearn 1.9+, where the parent removed the
  attribute, because BaseEstimator.get_params() still inspects our static
  backwards-compatible __init__ signature and expects the attribute to
  exist.
- Filter n_alphas from get_params() before sklearn's internal path_params
  can pass it to lasso_path.
- Pair that filter with a set_params() override that maps the legacy name
  to alphas, preserving GridSearchCV and Pipeline parameter grids.
- Emit one EconML FutureWarning when users explicitly select the legacy
  alias, nudging them toward alphas=<int> without allowing sklearn to emit
  one warning per CV fold.

The get_params/set_params pattern was originally suggested in #1046;
credit to @genrichez for identifying it.

Tests cover warning-free default fits, the clone/set_params/fit path,
wrapper-level deprecation warnings, and #1042's strict-sklearn regression.
The implementation was verified against sklearn 1.4.2, 1.8.0, and 1.9.0.

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
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
…cy n_alphas compatibility

Layered on top of #1042, which fixed sklearn 1.9's immediate
InvalidParameterError. This follow-up centralizes and completes the
cross-version handling for WeightedLassoCV and WeightedMultiTaskLassoCV.

Replace inline sklearn version parsing with SKLEARN_GE_17 and factor the
shared parent-constructor kwargs so only the alphas/n_alphas translation
branches across versions.

Keep the wrappers' legacy n_alphas argument usable without leaking it into
sklearn internals:

- Translate n_alphas=<int> to alphas=<int> on sklearn >= 1.7.
- Preserve sklearn 1.7-1.8's "deprecated" sentinel in self.n_alphas so the
  parent's fit-time deprecation check stays quiet.
- Restore that same sentinel on sklearn 1.9+, where the parent removed the
  attribute, because BaseEstimator.get_params() still inspects our static
  backwards-compatible __init__ signature and expects the attribute to
  exist.
- Filter n_alphas from get_params() before sklearn's internal path_params
  can pass it to lasso_path.
- Pair that filter with a set_params() override that maps the legacy name
  to alphas, preserving GridSearchCV and Pipeline parameter grids.
- Emit one EconML FutureWarning when users explicitly select the legacy
  alias, nudging them toward alphas=<int> without allowing sklearn to emit
  one warning per CV fold.

The get_params/set_params pattern was originally suggested in #1046;
credit to @genrichez for identifying it.

Tests cover warning-free default fits, the clone/set_params/fit path,
wrapper-level deprecation warnings, and #1042's strict-sklearn regression.
The implementation was verified against sklearn 1.4.2, 1.8.0, and 1.9.0.

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
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 30, 2026
…cy n_alphas compatibility

Layered on top of #1042, which fixed sklearn 1.9's immediate
InvalidParameterError. This follow-up centralizes and completes the
cross-version handling for WeightedLassoCV and WeightedMultiTaskLassoCV.

Replace inline sklearn version parsing with SKLEARN_GE_17 and factor the
shared parent-constructor kwargs so only the alphas/n_alphas translation
branches across versions.

Keep the wrappers' legacy n_alphas argument usable without leaking it into
sklearn internals:

- Translate n_alphas=<int> to alphas=<int> on sklearn >= 1.7.
- Preserve sklearn 1.7-1.8's "deprecated" sentinel in self.n_alphas so the
  parent's fit-time deprecation check stays quiet.
- Restore that same sentinel on sklearn 1.9+, where the parent removed the
  attribute, because BaseEstimator.get_params() still inspects our static
  backwards-compatible __init__ signature and expects the attribute to
  exist.
- Filter n_alphas from get_params() before sklearn's internal path_params
  can pass it to lasso_path.
- Pair that filter with a set_params() override that maps the legacy name
  to alphas, preserving GridSearchCV and Pipeline parameter grids.
- Emit one EconML FutureWarning when users explicitly select the legacy
  alias, nudging them toward alphas=<int> without allowing sklearn to emit
  one warning per CV fold.

The get_params/set_params pattern was originally suggested in #1046;
credit to @genrichez for identifying it.

Tests cover warning-free default fits, the clone/set_params/fit path,
wrapper-level deprecation warnings, and #1042's strict-sklearn regression.
The implementation was verified against sklearn 1.4.2, 1.8.0, and 1.9.0.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Scikit-learn v1.9 drops n_alphas from LassoCV and related estimators, causing econml to error on import

2 participants