TST: enforce proper sklearn __init__ for classifiers

[tylerjereddy]

• Related to TST: enforce init/param mutability requirements for sklearn convention #111, but only aims to address a subset of it, and only for classifiers for now. If others want to help deal with regressors and other parts of the issue that is welcome.

• The regression test added here should now cause a failure when the violation of sklearn __init__ conventions from WIP, ENH: Add support for gamma scaling and iterative solvers. #70 (comment) is introduced into the source code.

• Although I did not use AI in the preparation of this branch, the original review comment above is from the greptile AI reviewer.

[greptile-apps]

Greptile Summary

This PR adds a regression test (test_preserve_class_inputs) to enforce that GFDLClassifier and EnsembleGFDLClassifier comply with scikit-learn's __init__ convention: constructor parameters must be stored verbatim as instance attributes without any type transformation.

• The test instantiates each classifier, calls get_params(), and verifies both equality and exact type (isinstance(v, type(expected[k]))) against a known-good dict of default values.
• Both the value equality check and the type check are intentional — the value check catches transformations that produce different values (e.g., a tuple becoming a list), while the type check catches silent coercions that still compare equal in Python (e.g., int 0 stored as float 0.0).

Confidence Score: 4/5

Safe to merge — the change is additive (a new test only) and does not touch production code.

The new test correctly catches the sklearn init convention violation it was designed for. The only concern is that the shared expected dict across both parameterized estimators contains class-specific keys (direct_links for GFDLClassifier, voting for EnsembleGFDLClassifier) that are silently skipped for the estimator that doesn't own them, which could mislead future maintainers into thinking all eight keys are validated for both classifiers.

src/gfdl/tests/test_model.py — specifically the shared expected dict and loop structure in test_preserve_class_inputs.

Important Files Changed

Filename	Overview
src/gfdl/tests/test_model.py	Adds test_preserve_class_inputs parametrized over both classifiers; test logic is sound but the shared expected dict includes keys not applicable to both estimators, which may mislead future maintainers.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["test_preserve_class_inputs(estimator)"] --> B["clf = estimator(seed=0)"]
    B --> C["actual = clf.get_params()"]
    C --> D["Loop: for k, v in actual.items()"]
    D --> E{k in expected?}
    E -- Yes --> F["assert v == expected[k]"]
    F --> G["assert isinstance(v, type(expected[k]))"] 
    G --> D
    E -- No --> D
    D -- done --> H[Test passes]

    subgraph Estimators
        I[GFDLClassifier]
        J[EnsembleGFDLClassifier]
    end
    Estimators --> A

    subgraph expected_dict ["expected dict (union of both)"]
        K["activation, hidden_layer_sizes, reg_alpha, rtol, seed, weight_scheme"]
        L["direct_links (GFDLClassifier only)"]
        M["voting (EnsembleGFDLClassifier only)"]
    end

Loading

_{Reviews (1): Last reviewed commit: "TST: enforce proper sklearn `__init__`" | Re-trigger Greptile}

[greptile-apps]

Shared expected dict silently skips class-specific params

The loop iterates over actual.items() and only validates a key when it appears in both actual and expected. Because the same dict is used for both estimators, direct_links (present in GFDLClassifier but not in EnsembleGFDLClassifier) and voting (present in EnsembleGFDLClassifier but not in GFDLClassifier) each sit as a "dead" entry in expected for the other estimator — they will never be reached in that estimator's test run. This means a reader scanning expected cannot tell which keys are actually validated for which estimator without checking both __init__ signatures. Consider using per-estimator expected dicts, or iterating over expected.items() and guarding with if k in actual, so that omissions are explicit rather than silent.

[tylerjereddy]

@eiviani-lanl @nray this PR has been open for 3 weeks and only has a 20 line diff -- that should be enough time for a detailed code review. Try to help keep moving things forward with helpful feedback.

[eiviani-lanl]

Looks like sklearn has a check for this: check_parameters_default_constructible

This passes:

@pytest.mark.parametrize("estimator", [
    GFDLClassifier(), 
    EnsembleGFDLClassifier()
])
def test_preserve(estimator):
    check_parameters_default_constructible(estimator.__class__.__name__, estimator)

and I checked failure by changing __init__() to

def __init__(
        self,
        hidden_layer_sizes: np.typing.ArrayLike = (100,),
        activation: str = "identity",
        weight_scheme: str = "uniform",
        direct_links: bool = True,
        seed: int = None,
        reg_alpha: float = None,
        rtol: float | None = None,
    ):
        self.hidden_layer_sizes = hidden_layer_sizes
        self.activation = activation
        self.direct_links = direct_links
        self.seed = seed
        self.weight_scheme = weight_scheme.lower() # forcing a failure
        self.reg_alpha = reg_alpha
        self.rtol = rtol

Although I'm fairly certain this should be caught in parametrize_with_checks...

[eiviani-lanl]

Ah nevermind, I understand now. This line in check_parameters_default_constructible means they're only checking the values of the input array, so assert_array_equal(np.array([100]), (100,)) passes.

if isinstance(param_value, np.ndarray):
    assert_array_equal(param_value, init_param.default)

[tylerjereddy]

@eiviani-lanl @nray what's the status here? I still don't see reviewer comments that provide a clear analysis of the situation and a path forward for us, and this 20 line PR has been open for more than a month now.

Some things that reviewers may want to check:

• if you pick an sklearn estimator like RandomForestClassifier and modify its source to perform the violation we're trying to guard against here, does the sklearn testsuite itself fail?
• may need to check for upstream issues/docs related to this and possibly engage with sklearn team proper to see if they have a view on this in terms of what conformant projects should be doing (and possibly it might even be something missing from their parametrize_with_checks or other testing implementations?)

[sdtemple]

Overall, I suggest that this test would be redundant if the estimators were incorporated into sklearn. The suggested test serves to enforce the instantiation rules of __init__ for the new ELM/RVFL estimator classes.
https://scikit-learn.org/stable/developers/develop.html#instantiation

sklearn already has tests that enforce these rules. I forked the sklearn repo and started to modify/break the BaseDecisionTree class object under the tree submodule.

I focus on modifying the random_state variable, as modifying other variables creates many other test errors not relevant to the current proposal.

Throughout I will check tests with pytest sklearn/tests/test_common.py -v -k "DecisionTree" which appears to check API compliance.

If you comment out # self.random_state = random_state, then we get AttributeError: 'DecisionTreeClassifier' object has no attribute 'random_state'.

If you hard-code self.random_state = 100395, then we get AssertionError: Parameter random_state was mutated on init.

If you define another attribute self.random_state_ = random_state, which is not in the __init__ arguments, then you get AssertionError: Estimator DecisionTreeClassifier should not set any attribute apart from parameters during init.

If you try to perform some operation on the attribute in __init__:

        if type(random_state) is int:
            self.random_state += 1

then you get "RuntimeError: Cannot clone object DecisionTreeRegressor(max_depth=3, random_state=1), as the constructor either does not set or modifies parameter random_state".

In these toy examples, I have demonstrated existing tests on __init__ of estimators that:

• Make sure arguments are immutable
• The class attributes are stored exactly as instantiated
• Only the passed arguments are stored as attributes
• The parameters are not modified

[tylerjereddy]

Overall, I suggest that this test would be redundant if the estimators were incorporated into sklearn

Which specific public sklearn testing utility could we use in our project here then, instead of having to write our own to enforce this? The point is that this repo is not sklearn, so how do we enforce this property appropriately on our end? At the moment, we can make such a change and the testsuite here will not fail, so how do we enforce that without needing to write a custom test? The route forward still isn't clear to me based on your analysis.

Is there a public sklearn testing utility to help us here or not? If yes, then tell us which one it is and make sure it causes the failure when we introduce the problem described above. If no, then how else do we enforce this property, since our current testsuite is not sensitive per the analysis in the comments prior to yours.

[sdtemple]

I think the function check_estimator in sklearn.utils.check_estimators is a public utility we could use to enforce that parameters are set correctly, as well as other aspects of the sklearn estimator conventions.
https://github.com/scikit-learn/scikit-learn/blob/3c5db7378c68b3477e121138d0f657da1d1136df/sklearn/utils/estimator_checks.py#L745

The following code will give a list of 55 dictionaries that define a test performed (e.g., check_set_params, check_no_attributes_set_in_init).

from sklearn.utils.estimator_checks import check_estimator
from gfdl.model import GFDLClassifier, GFDLRegressor, EnsembleGFDLClassifier, GFDL
clf = GFDLClassifier(seed=0)
check_estimator(clf)

All checks pass in the current version for Classifier and Regressor. If I modify the class GFDL base class similarly to in my previous comment, we get similar errors to as in my previous comment.

[tylerjereddy]

I think the function check_estimator in sklearn.utils.check_estimators is a public utility we could use to enforce that parameters are set correctly

@sdtemple the project currently uses parametrize_with_checks -- for example, on the main branch if you do git grep -E -i "parametrize_with_checks", you'll see that we're using that sklearn public testing utility in a few places:

src/gfdl/tests/test_model.py:from sklearn.utils.estimator_checks import parametrize_with_checks
src/gfdl/tests/test_model.py:@parametrize_with_checks([GFDLClassifier(), EnsembleGFDLClassifier()])
src/gfdl/tests/test_regression.py:from sklearn.utils.estimator_checks import parametrize_with_checks
src/gfdl/tests/test_regression.py:@parametrize_with_checks([GFDLRegressor()])

If we look at the docs for the function you mention: https://scikit-learn.org/stable/modules/generated/sklearn.utils.estimator_checks.check_estimator.html

it notes:

scikit-learn also provides a pytest specific decorator, parametrize_with_checks, making it easier to test multiple estimators.

so it seems to me like we're already following your suggestion.

What's really confusing to me is that you keep talking about the general case of bad input, which we already verify/test against, vs. a very specific case of bad input which is described in the links above. As far as I can tell, you're just providing us with general guidance, rather than investigating the very specific scenario that is of concern here.

Here is a diff that adds your suggested testing to one of our current API tests (I don't think this is useful, I think it is just duplicating the testing, but let's try it anyway):

--- a/src/gfdl/tests/test_model.py
+++ b/src/gfdl/tests/test_model.py
@@ -6,7 +6,7 @@ from sklearn.datasets import load_breast_cancer, load_digits, make_classificatio
 from sklearn.metrics import accuracy_score, roc_auc_score
 from sklearn.model_selection import StratifiedKFold, train_test_split
 from sklearn.preprocessing import OneHotEncoder, StandardScaler
-from sklearn.utils.estimator_checks import parametrize_with_checks
+from sklearn.utils.estimator_checks import parametrize_with_checks, check_estimator
 from ucimlrepo import fetch_ucirepo
 
 from gfdl.activations import ACTIVATIONS
@@ -438,6 +438,7 @@ def test_classification_against_grafo(hidden_layer_sizes, n_classes, activation,
 @parametrize_with_checks([GFDLClassifier(), EnsembleGFDLClassifier()])
 def test_sklearn_api_conformance(estimator, check):
     check(estimator)
+    check_estimator(estimator)

That test currently passes on latest main:

python -m pytest -k "test_sklearn_api_conformance" -n 10

Now, let's introduce the specific API problem this PR is focused on, which is clearly linked to above (but which, confusingly, you've not mentioned at all yet... why?):

--- a/src/gfdl/model.py
+++ b/src/gfdl/model.py
@@ -35,7 +35,7 @@ class GFDL(BaseEstimator):
         reg_alpha: float = None,
         rtol: float | None = None,
     ):
-        self.hidden_layer_sizes = hidden_layer_sizes
+        self.hidden_layer_sizes = np.asarray(hidden_layer_sizes)
         self.activation = activation
         self.direct_links = direct_links

The testsuite still passes with your suggested additional modification. On the other hand, the new test added in this PR does properly fail with that problem diff.

So I think there are some important questions to answer here:

1. Why are you so focused on the general case of bad input, which we already test for, and not the specific case of bad input that this PR is all about? Did you actually identify the exact problem we're trying to solve?
2. What's your proof that your suggested check actually works for the specific case we're concerned about here. My analysis indicates that your check is not useful, is redundant with our current testing, and really has nothing to do with the review of this PR. Perhaps if you can confirm that both general/public approaches to testing the API conformance fail to catch the problem we are concerned with, you can start the actual review of what the problem is here.

[tylerjereddy]

Did you actually identify the exact problem we're trying to solve?

In particular, the first comment in this PR #112 (comment) links to it. Also, several of the claims about input validation redundancy made in the review comments above could be pretty quickly disproven by simply running our testsuite against the "general" failure cases vs. the special failure case that is the focus here--make sure you're actually running the testsuite.

[sdtemple]

Sorry for my confusion earlier on if the PR concerned general versus specific behavior of np.asarray in __init__. I'll describe below how some sklearn estimators fail with this specific change because of internal checks of fit(). From my perspective this violation of the API makes more sense to be checked within an API compliance test, like in check_estimator, than indirect TypeError instances for estimator-specific tests. As this is my first code review, would the appropriate action be to make an issue on sklearn about testing for public attribute type preservation?

I transformed array-like inputs in VotingClassifier and MLPRegressor and MLPClassifier from sklearn with np.asarray, specifically weights and hidden_layer_sizes.

Running the tests in sklearn/utils/tests passed for all cases which is the testsuite where check_estimator lies. Many of the dtype checks focus on the public attributes not changing after fitting and predicting. There are checks of equality for the public attributes, which a case like np.asarray(None)==None passes despite the difference in type.

For pytest sklearn/ensemble/tests and pytest sklearn/neural_network/tests we fail at various places due to TypeError when fit() is run with specific edge cases values.

In _voting.py lines 83 - 87:

if self.weights is not None and len(self.weights) != len(self.estimators):
    raise ValueError(
        "Number of `estimators` and weights must be equal; got"
        f" {len(self.weights)} weights, {len(self.estimators)} estimators"
    )

The tests that fail here have None as the default for weights, resulting in np.asarray(None) being an unsized object.

In _neural_network.py lines 462 - 467:

def _fit(self, X, y, sample_weight=None, incremental=False):
    # Make sure self.hidden_layer_sizes is a list
    hidden_layer_sizes = self.hidden_layer_sizes
    if not hasattr(hidden_layer_sizes, "__iter__"):
        hidden_layer_sizes = [hidden_layer_sizes]
    hidden_layer_sizes = list(hidden_layer_sizes)

The tests that fail here have hidden_layer_sizes=10. The if clause is not triggered and then list(np.asarray(10)) fails as an iteration over a 0-d array.

[tylerjereddy]

this violation of the API makes more sense to be checked within an API compliance test, like in check_estimator

Agreed, it would be great if parametrize_with_checks (the pytest-specific equivalent of check_estimator) would just do that for us--then we wouldn't even need this PR nor the maintenance burden associated with it.

As this is my first code review, would the appropriate action be to make an issue on sklearn about testing for public attribute type preservation?

Probably--you'd likely want to word/frame it carefully, after double checking there's no issue already open about this (they have a lot of issues already open, so opening duplicates is an annoyance).

There are checks of equality for the public attributes, which a case like np.asarray(None)==None passes despite the difference in type.

Right, I think your analysis is identical to Emma's a few months back/above at: #112 (comment)

From your description, it sounds like sklearn does protect against the behavior we're trying to avoid here, but it is almost accidental in the sense that they don't have custom/public testing utilities to enforce it--the failures only happen "by accident" in tests focused on other things? If that's correct, and there's no issue already open about this in their repo, then yes--a well written issue describing this situation and asking them to confirm that the behavior is undesired and if they're be interested in extending check_estimator and parametrize_with_checks to specifically cover this situation might be a good start?

Then, as we discussed, we may have to make a decision here on how long that process will take to play out, and decide if we'll just move forward with this for now and then open an issue on our end to remind us to update our testing if they ever make such a utility public.

[sdtemple]

Okay, I made an issue to start dialogue with the scikit-learn team concerning this attribute type test.
scikit-learn/scikit-learn#34715

[sdtemple]

The scikit-learn team responded to my issue.
scikit-learn/scikit-learn#34715 (comment)

To their first question, I agree with the documentation on instantiation rules and the good and bad examples are clear that you should not transform the inputs in any way in __init__. Additionally, under the "Rolling your own estimator" section they mention that in BaseEstimator they tend to use "duck typing" instead of checking for isinstance when inheriting from sklearn-specific classes, which would suggests a general practice they follow.

We tend to use “duck typing” instead of checking for isinstance, which means it’s technically possible to implement an estimator without inheriting from scikit-learn classes.

To their second question, they refer me to a long DOC issue where people discuss confusion surrounding developing sklearn-compatible estimators. @eiviani-lanl @tylerjereddy @nray do you all know the story behind why the code made a type transformation in __init__? They seem interested in better understanding pain points for estimator developers if you all have any experiences to share.

scikit-learn/scikit-learn#33003

[tylerjereddy]

clear that you should not transform the inputs in any way in init

Sounds about right, so you should be able to suggest a clear path forward here and in gh-114 now that you have upstream confirmation that this is not a desirable thing to do.

do you all know the story behind why the code made a type transformation in init

The background is already explained above, with Navamita making a rough draft PR in gh-70 where an __init__ parameter was accidentally given a type change that our testsuite did not detect, so these new PRs were opened to prevent that from accidentally slipping through in the future. I suspect Navamita was just busy when she was prototyping and I'm sure there's a trivial workaround if we decide to continue on that specific PR. Nonetheless, the original risk remains in a broad sense.

From Tim's response, it seems clear that they agree the mutations in __init__ should not happen, but mostly suggest that folks should read the docs rather than them adjusting their public testing tools at this stage. So, you could try to argue that with him, but they are busy so even if you "win" it could take quite a while.

[sdtemple]

I approve of this merge request that checks equality and type for values passed to __init__.

All tests pass (including this one) or are skipped as is. The following are edited examples that would trigger this test in python -m pytest. In particular, these are examples that are detected by assert isinstance(v, type(expected[k])) but not assert v == expected[k].

+ self.hidden_layer_sizes = np.asarray(hidden_layer_sizes)
- self.hidden_layer_sizes = hidden_layer_sizes
+ self.direct_links = int(direct_links)
- self.direct_links = direct_links
+ self.seed = float(seed)
- self.seed = seed

Based on the sklearn response, I don't plan to argue for an addition to their testing suite. As noted previously, there are already, albeit less intuitive and indirect, tests that can detect type transformations in __init__.

I concur with Tim that this np.asarray transformation is a niche case and that more focus should be on user clarity and experience on the sklearn developer workflow documentation, which the broader sklearn team remains undecided on their path forward.

[tylerjereddy]

Ok, I believe you have permission to merge if you're happy then.