support parameter-specific base learners

[sarptandoven]

summary

adds support for passing one base learner per distribution parameter.

this keeps the existing Base=DecisionTreeRegressor(...) behavior, and also allows:

Base=[loc_learner, scale_learner]

for distributions like Normal.

this addresses the constrained-location use case from #338 without changing the default path.

changes

• fit each distribution parameter with its matching base learner
• validate sequence length against Dist.n_params
• support sklearn nested params for both forms:
  • Base__max_depth
  • Base__0__max_depth
• keep feature_importances_ working for all-tree models
• return None for mixed learner feature importances instead of failing

tests

python -m black ngboost/ngboost.py ngboost/api.py tests/test_basic.py
git diff --check
python -m pytest tests/test_basic.py tests/test_pickling.py -q
python -m pytest -q
python -m pytest --slow -q

results:

17 passed
65 passed, 57 skipped
96 passed, 2 xfailed, 22 xpassed

closes #338

[alejandroschuler]

hi @sarptandoven , happy to merge this if you can fix the lint errors (and it looks like this supersedes #410 which should be closed?)

[alejandroschuler]

Here's a possible fix:

1. Silence the two inherent stub warnings on the RecordingRegressor helper. Setting fitted attributes (sample_weight_, prediction_) in fit is the sklearn convention, and fit's X is unused but required by the sklearn API, so neither is worth restructuring. One class-level disable covers both:

class RecordingRegressor(BaseEstimator, RegressorMixin):
+    # pylint: disable=attribute-defined-outside-init,unused-argument
    def fit(self, X, y, sample_weight=None):
        self.sample_weight_ = None if sample_weight is None else sample_weight.copy()
        self.prediction_ = np.average(y, weights=sample_weight)
        return self

2. Replace the empty-list equality checks in test_base_learner_sequence_must_match_distribution_parameter_count

 class RecordingRegressor(BaseEstimator, RegressorMixin):
+    # pylint: disable=attribute-defined-outside-init,unused-argument
     def fit(self, X, y, sample_weight=None):
@@
-    assert ngb.base_models == []
-    assert ngb.scalings == []
-    assert ngb.col_idxs == []
+    assert not ngb.base_models
+    assert not ngb.scalings
+    assert not ngb.col_idxs

After this: pylint 10.00/10 (exit 0), black --check and isort --check-only unchanged, and make lint would pass alongside the already-green make test (65 passed, 57 skipped).

[ryan-wolbeck]

@alejandroschuler, I'll run a couple tests after linting gets fixed and pub a release but will review in the next day

[ryan-wolbeck]

Here's a possible fix:

1. Silence the two inherent stub warnings on the RecordingRegressor helper. Setting fitted attributes (sample_weight_, prediction_) in fit is the sklearn convention, and fit's X is unused but required by the sklearn API, so neither is worth restructuring. One class-level disable covers both:

class RecordingRegressor(BaseEstimator, RegressorMixin):
+    # pylint: disable=attribute-defined-outside-init,unused-argument
    def fit(self, X, y, sample_weight=None):
        self.sample_weight_ = None if sample_weight is None else sample_weight.copy()
        self.prediction_ = np.average(y, weights=sample_weight)
        return self

2. Replace the empty-list equality checks in test_base_learner_sequence_must_match_distribution_parameter_count

 class RecordingRegressor(BaseEstimator, RegressorMixin):
+    # pylint: disable=attribute-defined-outside-init,unused-argument
     def fit(self, X, y, sample_weight=None):
@@
-    assert ngb.base_models == []
-    assert ngb.scalings == []
-    assert ngb.col_idxs == []
+    assert not ngb.base_models
+    assert not ngb.scalings
+    assert not ngb.col_idxs

After this: pylint 10.00/10 (exit 0), black --check and isort --check-only unchanged, and make lint would pass alongside the already-green make test (65 passed, 57 skipped).

I did a simple local test and these suggestions do cause it to pass

[alejandroschuler]

this is good, thanks for the contribution. Also fixes a previously unidentified bug that tol and verbose_eval weren't in get_params and so didn't surivvie cloning

[ryan-wolbeck]

Looks good to me as well, the build times on 3.14 are running longer than it should but I made an issue to investigate but shouldn't block this PR.

Thanks!