WIP, ENH: Add support for gamma scaling and iterative solvers. - #70
Conversation
| return out | ||
|
|
||
|
|
||
| class GFDLRegressor(RegressorMixin, MultiOutputMixin, GFDL): |
There was a problem hiding this comment.
Has something changed about GFDLRegressor? The diff seems quite messy here, making it hard to judge what has actually changed and what hasn't. This class was deleted below and then pasted here?
There was a problem hiding this comment.
Yes, the class was at the end of the file, but I moved it after GFDLClassifier to be have similar estimators one after the other. The only other change is the extra argument "gamma" to the constructor and "solver" to fit method.
There was a problem hiding this comment.
It would be best not to reorganize the classes for now I think, so we can focus on what actually changed in the diff.
There was a problem hiding this comment.
Reverted to the old organization.
| return np.random.default_rng(seed) | ||
|
|
||
|
|
||
| def stochastic_gradient_descent(self, X, y): |
There was a problem hiding this comment.
What's the motivation for rolling our own SGD instead of using upstream SGDRegressor from sklearn, which has learning_rate, eta0, etc.? Does its verbose argument not provide enough output?
You mention it briefly in the matching issue, but my intuition would always be to try the upstream/well-tested solution first before investing time on a hand-rolled version.
|
|
||
| return self | ||
|
|
||
| def gradient_descent(self, X, y): |
There was a problem hiding this comment.
Would also be good to clarify why we can't use i.e., scipy.optimize.minimize (which has some output options and custom callback support), and if we ever need mini-batch/partial_fit() support for these experiments or not.
| seed: int = None, | ||
| reg_alpha: float = None, | ||
| rtol: float | None = None, | ||
| gamma: float = None, |
There was a problem hiding this comment.
Kostas' email on March 29/2026 suggests that there may be interest in the ability to provide different scaling values for different layers and/or to only apply scaling to a subset of layers, so the API design may need careful consideration.
There was a problem hiding this comment.
The api can now pass different gamma for each layer.
|
Beyond the various cleanups and clarifications noted above, this will also need regression tests before it is seriously ready for review. Given the amount of confusion I've seen around gamma scaling, the documentation should also explain how it works with crystal clarity. |
| Z = H_prev @ w.T + b # (n_samples, n_hidden) | ||
| H_prev = self._activation_fn(Z) | ||
| if self.gamma is not None: | ||
| H_prev *= 1.0 / (H_prev.shape[1] ** self.gamma) |
There was a problem hiding this comment.
@shahyadk-bu, @eiviani-lanl : I would appreciate some feedback about the gamma scaling api implemented here. To my understanding, this is equivalent to Shahyad's implementation here, but still it would nice to get a confirmation from both of you.
|
Hi Ray,
This seems correct. I also have begun using the non-training method of
solving the RVFL directly and have that code in my Git as well if you
wanted to look at that reference as well. It is the folder called
"RVFL_Solved" on my Git and there is a python script called "RVFL_model.py"
which holds my RVFL class and the scaling is implemented there as well.
Sincerely,
Shahyad
…On Wed, Apr 8, 2026 at 4:23 AM Navamita Ray ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/gfdl/model.py
<#70 (comment)>:
> Z = H_prev @ w.T + b # (n_samples, n_hidden)
H_prev = self._activation_fn(Z)
+ if self.gamma is not None:
+ H_prev *= 1.0 / (H_prev.shape[1] ** self.gamma)
@shahyadk-bu <https://github.com/shahyadk-bu>, @eiviani-lanl
<https://github.com/eiviani-lanl> : I would appreciate some feedback
about the gamma scaling api implemented here. To my understanding, this is
equivalent to Shahyad's implementation here
<https://github.com/shahyadk-bu/RVFL_Research/blob/main/RVFL_Trained/RVFL_Model.py#L45>,
but still it would nice to get a confirmation from both of you.
—
Reply to this email directly, view it on GitHub
<#70 (review)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/BZ2CZWVMIEDQOE5CSISBSJD4UWZ3ZAVCNFSM6AAAAACWYC7PESVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DANZSGQZDENRXGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Hi Shahyad, |
| seed: int = None, | ||
| reg_alpha: float = None, | ||
| rtol: float | None = None, | ||
| gamma: float | np.typing.ArrayLike | None = None, |
There was a problem hiding this comment.
here and elsewhere, note that ArrayLike already includes floats and other scalars: https://numpy.org/devdocs/reference/typing.html#numpy.typing.ArrayLike
| before inputting to the next layer. None implies no | ||
| scaling is applied. A single value implies applying | ||
| the scaling each layer with the same value. For different | ||
| gammas per layer, pass an array like type. |
There was a problem hiding this comment.
a float is also an array-like in NumPy speak, so we should probably reword a bit
tylerjereddy
left a comment
tylerjereddy
left a comment
There was a problem hiding this comment.
I provided a more detailed review now that the diff is much cleaner (thanks).
I suppose the amount of refinement we want to apply here depends on the empirical proof of #71 (comment). At least I feel like I'm clearer on the criterion we have on the value of gamma scaling now.
|
|
||
|
|
||
| @pytest.mark.parametrize("gamma, weight_scheme, expected_acc, expected_roc", [ | ||
| (None, "normal", 0.9472222222222222, 0.9889187266963562), |
There was a problem hiding this comment.
minor: spacing vs. below
| @pytest.mark.parametrize("gamma, weight_scheme, expected_acc, expected_roc", [ | ||
| (None, "normal", 0.9472222222222222, 0.9889187266963562), | ||
| (0.5, "normal", 0.9638888888888889, 0.9936661671672866), | ||
| ([0.5, 0.5, 0.5, 0.5], "he_normal", 0.975, 0.9952345941275027), |
There was a problem hiding this comment.
minor: maybe [0.5] * 4 ; [100] * 4 is done below for concision; hardly matters though
|
|
||
| @pytest.mark.parametrize("gamma, weight_scheme, expected_acc, expected_roc", [ | ||
| (None, "normal", 0.9472222222222222, 0.9889187266963562), | ||
| (0.5, "normal", 0.9638888888888889, 0.9936661671672866), |
There was a problem hiding this comment.
Suggestion for a useful test case here is the same case with [0.5] * 4, just to show that the array and the scalar have the same effect in this case, when all gammas are equal across layers.
Sample suggestion from playing with this locally, that passes tests and includes a small comment:
--- a/src/gfdl/tests/test_model.py
+++ b/src/gfdl/tests/test_model.py
@@ -521,6 +521,8 @@ def test_rtol_ensemble(reg_alpha, rtol, expected_acc, expected_roc):
@pytest.mark.parametrize("gamma, weight_scheme, expected_acc, expected_roc", [
(None, "normal", 0.9472222222222222, 0.9889187266963562),
(0.5, "normal", 0.9638888888888889, 0.9936661671672866),
+ # estimator performance unchanged with constant gamma array:
+ ([0.5] * 4 , "normal", 0.9638888888888889, 0.9936661671672866),
([0.5, 0.5, 0.5, 0.5], "he_normal", 0.975, 0.9952345941275027),
])
def test_gamma_scaling_classifier(gamma, weight_scheme, expected_acc, expected_roc):There was a problem hiding this comment.
Good suggestion, added it.
| roc_cur = roc_auc_score(y_test, y_hat_cur_proba, multi_class="ovo") | ||
|
|
||
| np.testing.assert_allclose(acc_cur, expected_acc) | ||
| np.testing.assert_allclose(roc_cur, expected_roc) |
There was a problem hiding this comment.
nice test!
Of course you know I'm going to ask about other tests. It may depend on how far we go with gamma scaling, and how valuable it proves to be in the absence of gradient descent.
I'm thinking about the status of implementations and testing, and whether it even makes sense for:
EnsembleGFDLClassifierGFDLRegressor
If they do make sense to include, we should probably open issues about that if we don't do it here (and I'm all for keeping the scope small initially, especially for the sanity of the reviewers).
|
|
||
|
|
||
| def test_gamma_scaling_size_mismatch_classifier(): | ||
| # Test size mismatch exception between list of gamma's |
There was a problem hiding this comment.
nit: gamma's -> gammas
| scaling is applied. A single value implies applying | ||
| the scaling each layer with the same value. For different | ||
| gammas per layer, pass an array like type. | ||
|
|
There was a problem hiding this comment.
See my suggestion to add a versionadded directive for new features in Emma's PR at: #47 (comment)
Of course, we don't yet know if we'll add this feature just yet.
Also, if we do add the feature, it would be good to add a reference that I think @shahyadk-bu was using for some of the work in the Notes section of the docstring. But only matters if we find this useful without gradients and actually want to add it into gfdl for real.
| reg_alpha: float = None, | ||
| rtol: float = None | ||
| rtol: float = None, | ||
| gamma: float | np.typing.ArrayLike | None = None, |
There was a problem hiding this comment.
can fuse the float into array-like, since they're the same
| reg_alpha=reg_alpha, | ||
| rtol=rtol) | ||
| rtol=rtol, | ||
| gamma=gamma) |
There was a problem hiding this comment.
If you're going to hook these into GFDLRegressor here then it probably is fair game to ask GFDLRegressor to be tested with gamma as well.
Assuming we find that gamma is actually useful in the absence of gradients.
| scaling is applied. A single value implies applying | ||
| the scaling each layer with the same value. For different | ||
| gammas per layer, pass an array like type. | ||
|
|
There was a problem hiding this comment.
versionadded directive here as well, if we find that gamma is helpful in absence of gradients and should be included for GFDLRegressor
| if np.isscalar(self.gamma): | ||
| H_prev *= 1.0 / (H_prev.shape[1] ** gamma) | ||
| else: | ||
| H_prev *= 1.0 / (H_prev.shape[1] ** self.gamma[i]) |
There was a problem hiding this comment.
On the topic of input validation, do we really want to allow arbitrary floating point values of gamma without restriction? When I set gamma to -1000 in one of your tests I get a zero division error for obvious reasons.
Perhaps we should bound gamma a bit and raise ValueError when outside those bounds? Can we define sensible bounds? And then add a test for that of course.
(assuming we find gamma to be useful of course)
Greptile SummaryThis PR adds gamma scaling support to normalize layer outputs before feeding into the next layer, and refactors hidden-layer construction into two new methods (
Confidence Score: 3/5The core gamma scaling logic is sound, but two changes in model.py directly contradict the sklearn estimator contract in ways that affect cloning, serialization, and CV utilities. Converting src/gfdl/model.py needs attention for the two sklearn API violations; test_model.py is straightforward but should gain GFDLRegressor gamma coverage. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant GFDLClassifier
participant GFDL
participant _init_weights
participant _construct_Hs
User->>GFDLClassifier: fit(X, y)
GFDLClassifier->>GFDL: fit(X, Y_encoded)
GFDL->>GFDL: "validate gamma & hidden_layer_sizes"
Note over GFDL: scalar gamma expanded to array (mutates self.gamma)
GFDL->>_init_weights: _init_weights(X)
_init_weights-->>GFDL: self.W_, self.b_ populated
GFDL->>_construct_Hs: _construct_Hs(X)
loop each layer i
_construct_Hs->>_construct_Hs: "Z = H_prev @ W[i].T + b[i]"
_construct_Hs->>_construct_Hs: "H = activation(Z)"
alt gamma is not None
_construct_Hs->>_construct_Hs: "H *= 1 / (H.shape[1] ** gamma[i])"
end
end
_construct_Hs-->>GFDL: Hs list
GFDL->>GFDL: build D_ (design matrix)
GFDL->>GFDL: solve for coeff_ (pinv or Ridge)
GFDL-->>User: fitted estimator
User->>GFDLClassifier: predict(X_test)
GFDLClassifier->>GFDL: predict(X_test)
GFDL->>_construct_Hs: _construct_Hs(X_test)
Note over _construct_Hs: reuses same gamma scaling
_construct_Hs-->>GFDL: Hs list
GFDL-->>User: predictions
Reviews (1): Last reviewed commit: "MAINT: Fix lint errors" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| gamma: np.typing.ArrayLike | None = None, | ||
| ): | ||
| self.hidden_layer_sizes = hidden_layer_sizes | ||
| self.hidden_layer_sizes = np.asarray(hidden_layer_sizes) |
There was a problem hiding this comment.
__init__
BaseEstimator requires __init__ parameters to be stored verbatim (without transformation) so that get_params() and clone() return the original values. Calling np.asarray() here means a user who passes a Python list [100, 100] will get back array([100, 100]) from get_params(), and clone() will create a new estimator with a numpy array instead of the original list. This can break sklearn utilities like GridSearchCV and Pipeline, and will likely fail the existing parametrize_with_checks suite which verifies this property. The conversion belongs in _init_weights or fit(), as it was before this PR.
There was a problem hiding this comment.
This LLM code review comment does seem valid. It is still undecided if we'll move forward with gamma scaling in the core library, but it would probably be useful if we could capture this requirement in our testsuite. Maybe an issue should be opened about that. It would be nice if we could use a testing utility from sklearn proper to enforce this, but it would also be straightforward to write our own test to do it.
| if self.gamma is not None: | ||
| if not np.isscalar(self.gamma): | ||
| if len(self.gamma) != len(self.hidden_layer_sizes): | ||
| raise ValueError("Mismatch between number of gamma values passed " | ||
| "and the total number of hidden layers. Expect them to be equal.") | ||
| else: | ||
| self.gamma = np.ones(len(self.hidden_layer_sizes)) * self.gamma | ||
|
|
||
| self.gamma = np.asarray(self.gamma, dtype=float) | ||
| if np.any((self.gamma > 1.0) | (self.gamma < 0.5)): | ||
| raise ValueError("Out of range gamma. Expected range : " | ||
| "None or [0.5, 1.0].") |
There was a problem hiding this comment.
self.gamma permanently mutated during fit(), breaking sklearn conventions
Lines 105 and 107 overwrite self.gamma — a constructor parameter — with a derived value. After the first fit() call with gamma=0.5, self.gamma becomes np.array([0.5, 0.5, 0.5, 0.5]). Any subsequent clone() call on the fitted estimator (e.g. during cross-validation via clone(fitted_estimator)) will initialise the clone with the array version, silently discarding the user's original scalar intent. The standard sklearn pattern is to leave constructor parameters unchanged and store the processed value under a separate fitted attribute (e.g. self.gamma_), or perform the expansion at use-time inside _construct_Hs.
There was a problem hiding this comment.
Similar to above, it would be good if we could capture this kind of requirement in a regression test, even if we don't ultimately move forward with gamma scaling in core lib.
| with pytest.raises(ValueError, match="must be > 0"): | ||
| clf.fit(X, y) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("gamma, weight_scheme, expected_acc, expected_roc", [ | ||
| (None, "normal", 0.9472222222222222, 0.9889187266963562), | ||
| (0.5, "normal", 0.9638888888888889, 0.9936661671672866), | ||
| # estimator performance unchanged with constant gamma array: | ||
| ([0.5] * 4, "normal", 0.9638888888888889, 0.9936661671672866), | ||
| ([0.5] * 4, "he_normal", 0.975, 0.9952345941275027), | ||
| ]) | ||
| def test_gamma_scaling_classifier(gamma, weight_scheme, expected_acc, expected_roc): | ||
| # Tests that gamma scaling does improve the accuracy score for the digits | ||
| # dataset. | ||
| data = load_digits() | ||
| X, y = data.data, data.target | ||
| X_train, X_test, y_train, y_test = train_test_split(X, y, test_size=0.2, | ||
| random_state=0) | ||
|
|
||
| scaler = StandardScaler() | ||
| X_train_s = scaler.fit_transform(X_train) | ||
| X_test_s = scaler.transform(X_test) | ||
|
|
||
| model = GFDLClassifier(hidden_layer_sizes=[100] * 4, | ||
| activation="sigmoid", | ||
| weight_scheme=weight_scheme, | ||
| seed=0, | ||
| gamma=gamma) | ||
| model.fit(X_train_s, y_train) | ||
|
|
||
| y_hat_cur = model.predict(X_test_s) | ||
| y_hat_cur_proba = model.predict_proba(X_test_s) | ||
|
|
||
| acc_cur = accuracy_score(y_test, y_hat_cur) | ||
| roc_cur = roc_auc_score(y_test, y_hat_cur_proba, multi_class="ovo") | ||
|
|
||
| np.testing.assert_allclose(acc_cur, expected_acc) | ||
| np.testing.assert_allclose(roc_cur, expected_roc) | ||
|
|
||
|
|
||
| def test_gamma_scaling_size_mismatch_classifier(): | ||
| # properly handle case where `gamma` parameter has size mismatch with | ||
| # hidden_layer_sizes | ||
| data = load_digits() | ||
| X, y = data.data, data.target | ||
| X_train, _, y_train, _ = train_test_split(X, y, test_size=0.2, | ||
| random_state=0) | ||
|
|
||
| X_train_s = StandardScaler().fit_transform(X_train) | ||
| model = GFDLClassifier(hidden_layer_sizes=[100] * 4, | ||
| activation="sigmoid", | ||
| weight_scheme="normal", | ||
| seed=0, | ||
| gamma=[0.5, 0.7]) | ||
| with pytest.raises(ValueError, match="Mismatch between number of gamma values"): | ||
| model.fit(X_train_s, y_train) | ||
|
|
||
|
|
||
| def test_gamma_scaling_invalid_input_classifier(): | ||
| # properly handle case where `gamma` parameter is out of range | ||
| data = load_digits() | ||
| X, y = data.data, data.target | ||
| X_train, _, y_train, _ = train_test_split(X, y, test_size=0.2, | ||
| random_state=0) | ||
|
|
||
| X_train_s = StandardScaler().fit_transform(X_train) | ||
| model = GFDLClassifier(hidden_layer_sizes=[100, 100], | ||
| activation="sigmoid", | ||
| weight_scheme="normal", | ||
| seed=0, | ||
| gamma=[-0.8, 100]) | ||
| with pytest.raises(ValueError, match="Out of range"): | ||
| model.fit(X_train_s, y_train) |
There was a problem hiding this comment.
GFDLRegressor gamma tests
GFDLRegressor receives the same gamma parameter in this PR but there are no corresponding tests exercising it. The validation logic in GFDL.fit() is shared, but any regressor-specific code path (e.g. the way Y is shaped) combined with gamma scaling is untested. Consider adding at least a smoke-test and an equivalent size-mismatch/out-of-range test for GFDLRegressor.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
* Related to gh-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 #70 (comment) is introduced into the source code. * Although I did not use AI in the preparatio of this branch, the original review comment above is from the greptile AI reviewer.
* Related to gh-111, but only aims to address a subset of it. This is the regression equivalent to the classifier-based testing added in gh-112. * Similar to that other PR, the regression test added here also causes a failure when introducing the API violation from #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 gh-70.
3 participants
This draft MR contains the changes needed for gamma scaling experiments.
Addresses issue: #71