TST: enforce proper sklearn __init__ -- regressors

[tylerjereddy]

• Fixes TST: enforce init/param mutability requirements for sklearn convention #111. This is the regression equivalent to the classifier-based testing added in TST: enforce proper sklearn __init__ for classifiers #112.

• Similar to that other PR, the regression test added here also causes a failure when introducing the API violation from WIP, ENH: Add support for gamma scaling and iterative solvers. #70 (comment).

• I did not use AI in the prepartion of this PR, though note that the original review comment that triggered all of this work was from the greptile AI reviewer on GitHub in WIP, ENH: Add support for gamma scaling and iterative solvers. #70.

[tylerjereddy]

@sdtemple this should be a straightforward follow-up review for you after handling gh-112

[sdtemple]

This PR is related to #112

Fixes #111 which together involves #112 and this PR

In #112, we discussed with the scikit-learn, who were reluctant to add
regression tests of this type to check_estimator and its related decorator.
They gave the opinion that the developer workflow is well-documented and
clear on this type of bug.

GFDLRegressor and GFDLClassifier and EnsembleGFDLClassifier share the same
arguments that are passed into the test function test_preserve_class_inputs(estimator).

As a result, I suggest the following code change

Line 13 in src/gfdl/tests/test_model.py

+from gfdl.model import EnsembleGFDLClassifier, GFDLClassifier, GFDLRegressor
-from gfdl.model import EnsembleGFDLClassifier, GFDLClassifier

Line 444 in src/gfdl/tests/test_model.py

+    GFDLClassifier, EnsembleGFDLClassifier, GFDLRegressor
-    GFDLClassifier, EnsembleGFDLClassifier

I ran the following command: pytest src/gfdl/tests/test_model.py.

Before the change, 5533 tests passed and none failed.
After the change, 5534 tests passed and none failed.
This is consistent with the decorator change adding 1 more test.

Next, I ran these commands on chicoma:

python -m venv gfdl_env
source ~/gfdl_env/bin/activate
cd GFDL
python -m pip install ".[test]"
python -m pytest src/gfdl/tests/test_model.py -k "test_preserve_class_inputs"

These added tests passed before and after the suggested change
(same behavior as Mac).

[sdtemple]

The above solution is a simple addition to the test function's decorator.
It requires the import of the GFDLRegressor estimator as well.

[sdtemple]

Simple concern as with the solution to #112 :

test_preserve_class_inputs will fail if we ever change the arg names
or remove args from the gfdl estimators, as the test hard codes the args.
I doubt we'll be changing those arg names, though.
And, if we ever add additional args, these would have to have defaults,
which is compliant or advised by the scikit-learn developer workflow.

[tylerjereddy]

I don't think these review comments reflect the current design of the code base. src/gfdl/tests/test_model.py is setup to only test the classifiers in the codebase, as you can see from the fact that we've never added GFDLRegressor tests in there (not even the import). Likewise, src/gfdl/tests/test_regression.py only tests the regressors in the codebase, as you can see from the fact that GFDLClassifier is never used in that module (not even the import). So, I don't think proposing to break that design is in scope of this PR--the different estimator categories have different testing modules.

Your review mostly focuses on confirming that the new test passes, but I don't see confirmation that the desired failure happens when you reintroduce the break we're trying to avoid in the first place? That's our main goal here, guarding against that.

Fixes #111 which together involves #112 and this PR

Agreed, I've adjusted my original PR description to auto-close the matching issue now that the classifier PR has been merged in.

[sdtemple]

Ah, I see now that the test_model.py and test_regression.py files are set up in the repo to test the respective Classifier versus Regressor classifiers.

Okay, the proposed change to the test_regression.py file in the commit is equivalent to what I said about the decorator in test_model.py, but instead consistent with the current design of the repo. I again tested that the new test passes as above.

Next, as in #112, I changed

+ self.hidden_layer_sizes = np.asarray(hidden_layer_sizes)
- self.hidden_layer_sizes = hidden_layer_sizes

and confirmed that the new test fails but that all other tests in test_regression.py pass.
I confirmed this on local Mac and chicoma linux.

Recall from discussion in #112 that the change to hidden_layer_sizes is a niche case in which the
other tests do not fail, whereas the check_estimator decorator detects many failures if you modify
other parameters like seed and activation.

This is what motivated the new test.

I approve of this PR if you @tylerjereddy are satisfied.

[tylerjereddy]

and confirmed that the new test fails but that all other tests in test_regression.py pass.

Thanks, I think we're "ok" then if you don't see any problems. Feel free to merge.