WIP, ENH: Updated the GFDL package to allow testing using Kyle's scaling parameters

[aganguly-lab]

Necessity
We found that the current implementation does not allow us to introduce the scaling parameters used by Kyle in his theoretical work. There is also no easy way to apply these scalings using pre/postprocesssing methods. Therefore there is a need for a version of the GFDL project that incorporates these scalings.

Incomplete Status
This branch is not intended to be merged immediately. The scaling has only been partially implemented in the non-ensemble fit and predict methods. The ensemble GFDL does not incorporate the scaling, nor do any partial_fit methods. Moreover, in the long term, I think it would be better to implement custom activation functions/weights. However, that would be a much larger change, and this will be useful in the short term for using our GFDL code to test Kyle's theoretical predictions.

New Parameters

• d_scaling: boolean Divides the input data X by a factor of sqrt(d), where d is the number of features
• p_scaling: boolean Divides the activation function by a factor of sqrt(p), where p is the number of hidden layers. Returns an error if set to true when there are multiple hidden layers.
• activation_scale: float Multiplies the activation function by a factor of sqrt(rho_h), where rho_h = activation_scale
• direct_links_scale: float Multiplies the direct link portion of the design function by a factor of sqrt(rho_x), where rho_x = direct_links_scale. Does nothing if direct_links is set to false.
• sign: str New activation function. Returns the sign of its input (-1,1) or 0 if the input is 0.
• cbrt: str New activation function. Returns the cube root of its input.

New Tests
All old test should pass with the new updates.

• test_improper_scaling:
  Tests error handling for cases in which scaling is applied inappropriately.
• test_scaling_classifier:
  Creates a baseline test establishing expected acc/roc values for scaled RVFLs.
  Expected acc/roc values were generated using this version of the GFDL project,
  so if there are any bugs I missed, they may affect this test.

TODO

• Add/incorporate tests for the new activation functions.
• Add some additional tests of expected scaling behavior.
  • The ELM with the sign activation function should return the same output with or without d_scaling
  • We should be able to test equivalence of the ELM with scaling and unscaled ELM with pre/postprocessing
  • Same for RVFL in the special case that rho_h/p = rho_x

AI Statement
AI was used for debugging, but the base code changes and tests were written without the help of AI.

[aganguly-lab]

I checked the failing test. It's exactly the test from Issue #117.

[tylerjereddy]

I checked the failing test. It's exactly the test from Issue #117.

Right, but that's still blocked because of the complexity discussed at #120 (comment).

It may be a bit before I can give this a proper review--I've made a note to circle back.

I've asked for a bit more info on practical uses for scaling in your issue at gh-124. Would be good to confirm that we can't accomplish this without changes to GFDL proper as well, but just setting _private or public_ attributes of estimators if this is really just for research purposes (you typically can access the weights for example). For selling the general practical interest, are there any published examples of internal scaling beyond our current research curiosities? Is this just a convenience matter for theoreticians to use in analysis? It hasn't been clearly explained to me why theoreticians are using scaling given the paucity of practical RVFL scaling estimator implementations that I'm aware of.

[tylerjereddy]

There is also no easy way to apply these scalings using pre/postprocesssing methods

I provided a first round of review here. The first scaling parameter I checked was trivial to replace with external scaling, without needing to scale inside the estimator, so my first impression here is that things probably haven't been checked carefully enough to make sure that they really belong in GFDL.

In addition to that, I have yet to see a plot/written argument or argument from in person meetings that suggests that the fancy/new scalings serve a clear practical purpose outside of mathematical convenience for i.e., proofing/analysis work. While that may not rule out adding them here eventually, my initial investigation suggests more care is needed before proposing that these scalings are not possible to achieve outside of the estimator.

[tylerjereddy]

Let's keep unrelated code changes out of the pull request to minimize the burden on the reviewer. If you think this would be helpful to add, go ahead and open a carefully crafted separate pull request to explain/add it.

[aganguly-lab]

This has been removed. It was an AI recommended fix for some issues I was having with ruff, but I should not have added an add-hoc fix like that to the MR. For some reason I had no issues this time, so I don't know what happened or how I could replicate those issues for a possible work item.

[tylerjereddy]

Please check your work carefully before presenting for review (even WIP)--this isn't the sign function and it would only take a few seconds of checking to catch that.

[aganguly-lab]

fixed

[tylerjereddy]

I wonder how much value there really is in writing docstrings that just duplicate (poorly) the content already available in the NumPy functions that they wrap -- we could also just put the NumPy functions here directly.

I'd like to see evidence that these activation functions are actually useful in the literature/practice, and not just mathematical conveniences/curiosities. If we really need to add them for research purposes we can, but I've basically seen no practical evidence that any of the "weird scaling" stuff that's happening for the error project with Kyle is genuinely needed yet.

[aganguly-lab]

A common theme in this review is a skepticism that this MR should be added to the main branch which is certainly a fair criticism.

I mostly added this branch so we could incorporate it into @nray's testing (currently she manually calculates the RVFL and ELM test errors, and you had commented that she should incorporate the built-in GFDL code that we already have).

Is there some middle ground between adding this branch to the main branch and adding an untested hacked GFDL implementation to @nray's theory work?

Long term, once the hyperopt code is stable and we have new baseline estimates for our various gfdl runs, I plan to run a few quick tests to see if scaling or post/pre-processing can give us any improvements. If I see improvements, I will probably push to have this code cleaned up and merged with the main branch at that time. Otherwise, I would agree that this branch should not be added to main.

[aganguly-lab]

Also regarding the application of the sign and cbrt activation functions, I do not believe these are useful for improving the GFDL. Instead, I was trying to demonstrate that the sign activation function could be used to approximate GFDL with many activation functions (e.g. erf, tanh, sigmoid) in the absence of d-scaling, which may be useful for Kyle's theoretical work. If we find these scalings do not improve our error, then this could be used to argue for Kyle to do his analysis using a sign activation function.

The cbrt activation function is less justified. I was doing some experiments of my own, and I wanted to see if double scaling X would improve GFDL error rates when the derivative of the activation function is infinite at 0. I don't know of any standard activation functions with this property, so I added a cube root activation function. This was just based on me testing a hunch, but we could easily remove it and I can add it back to my local workspace if I need it for testing.

[tylerjereddy]

you had commented that she should incorporate the built-in GFDL code that we already have

Yes, exactly, the GFDL code we already have without any additional internal scaling. We've coded up an estimator over a few months, now we want to probe the error behavior of that model, not a mutant of it. You can just use this branch for messing around if you want, but results of the research should clearly indicate which estimators use ectopic internal scaling, and which are the "real"/native GFDL, since the latter is all I'd like to see initially. If the internal scalings are producing truly incredible results, +5% accuracy on challenging datasets across the board for example, then sure, push for it. I've not seen such a clear picture yet, but yeah feel free to push on that if you're enjoying it.

Is there some middle ground between adding this branch to the main branch and adding an untested hacked GFDL implementation to @nray's theory work?

Yes, removing the need for ectopic scaling inside the estimator and scaling on the data only, then you can just use the native, fully tested GFDL, and whatever scaling you want on the data you can use/test at your own peril, as usual for consuming an sklearn-compatible estimator in user code. As presented, this PR did not and does not convince me that we need internal scaling to compare with theory that is focused on a true single layer RVFL as used in practice. Maybe just use this branch as is, but be prepared to clearly separate research results between "real GFDL" and the ectoptic internal weights model. The former is likely of most immediate interest for the upcoming paper while completely new scaling schemes that are not used in practice likely require extremely careful vetting.

I'd also point out that I've generally had to push pretty hard to get even basic regression testing in the internal error model repo, and trying to push this past me definitely seems like it is trying to shift the burden of handling the regression testing to this project/me. If you do a really great job of demonstrating practical value/resilience of the internally scaled models in the internal regression tests, that may help move this forward socially, but at the moment there definitely seems to be some attempt to transfer the testing burden for the ectopic scaling schemes to this project, which probably doesn't make sense yet and is... a bit annoying. Prove things out in your rough code first, then try to propose a place for it here after very careful checking.

A common theme in this review is a skepticism that this MR should be added to the main branch

Sure, I was easily able to abstract out some of the internal scaling, and it may still be possible to abstract out even more with a bit of creativity, especially since there's confinement to a single layer. I wasn't convinced and remain unconvinced that thorough checking has been performed to ensure that these scaling effects can't be abstracted outside the estimator, and so far that skepticism has been justified.

If there's a strong desire to explore unusual scaling schemes inside the estimator that's fine to do somewhere else, or just off of this branch if you want, but until there's an actual indication of pragmatic value I'm mostly looking for prediction of error behavior of a "real" GFDL for obvious reasons. You could always propose to add the new hyperparameters later, but I'd say the bar for addition to main is high given the issues I've seen so far, the overwhelming presentation of plots that don't really tell me a clear story, and justifications for internal scaling that wander too close to backpropagated models and don't consider the guards/protections we already have against "explosion." Why can't we use those? Do we really need any change here at all? It remains unclear at best.

You can point Navamita's repo/project at this branch easily enough without needing to merge it, but again please clearly separate research results between ectopic scaling and "native" GFDL. You can try to push in more regression tests here if Navamita doesn't want them in her repo.

[aganguly-lab]

Fair. In that case, do you recommend I close this merge request for now? I believe the process would require me to convert this MR to a draft?

I can give @nray access to the branch for testing Kyle's results, and if we find anything significant, we can try to implement it using existing infrastructure or reopen the merge request if the scaling is absolutely crucial. Same if I find good indication that scaling can give us something the other hyperparameter choices cannot.

[tylerjereddy]

Leaving it as a WIP PR should be fine I think.

[tylerjereddy]

Also, there is value in leaving this open because I may take a stab at abstracting the internal scalings outside of the estimator using the provided regression test as a proving ground.

[tylerjereddy]

One more interesting idea if you really want to prove value of the new hyperparams--show that they outperform native GFDL in TabArena over the same number of hyperopt trials by a substantial amount. That should be challenging, since you'll have more hyperparameters with the same number of trials and I'd be impressed if there were any really substantial gains. IIRC, even direct links don't matter much in TabArena, we just perform terribly on accuracy and great on speed no matter what, so if you gained substantial ranks there that could be impressive (naturally, I'm quite skeptical, but happy to be impressed!).

[tylerjereddy]

The CI is correctly failing because there are no docstrings written for these new params.

[aganguly-lab]

Fixed

[aganguly-lab]

Quick question, is there a way for me to do lint testing locally? I have 3 commits to this MR for dumb mistakes I made while trying to update the API, and they were only caught when the pipeline caught them (though the last is my fault; I forgot to ruff check my changes).

[tylerjereddy]

Yes, you should be testing everything locally before you push changes up. You can check the CI testing configuration to mimic its commands locally.

Also, keep an eye on the commit message prefixes, some of them are mismatched to what they do.

[tylerjereddy]

I see no justification whatsoever for d_scaling -- you can easily scale X before you feed it to the estimator so why is this here? The burden of determining if a plain scaling of the design matrix is appropriate definitely falls back to the custom code that you and Navamita are writing.

You already have access to the number of features in the design matrix before you feed anything to the estimator. This is unnecessary complexity inside the estimator. As it stands, many of the weird scalings done on X and y at https://lisdi-git.lanl.gov/gfdl_research/gfdl_error_model/-/merge_requests/1 seem to perform in weird ways, but that's not a good excuse to try baking them in to the estimator--you can easily write tests over there for scalings that are directly applicable to X and y like this one.

[aganguly-lab]

d-scaling has been removed.

[tylerjereddy]

Why are you doing a second check for GFDLClassifier in the same test that a conditional check already exists for the type of classifier? You can just hoist this up above under the arm of the conditional where only GFDLClassifier is allowed rather than checking this yet again.

[aganguly-lab]

Fixed.

[tylerjereddy]

This test design does not look correct--the test would pass if partial_fit did NOT raise an error, but fit() did, which is definitely not what you want to enforce here.

Also, as usual, please use match=... to assert on the desired error message text.

[aganguly-lab]

Yes, thanks for catching that! I now run fit to see if the code returns an error when negative scaling parameters are called and partial_fit to catch the NotImplementedError.

[tylerjereddy]

What's the gamma scaling test? Reviewers have limited bandwidth and git grep -E -i "gamma" returns no other results in the repo at the moment.

[aganguly-lab]

I removed the comment. It was mostly written for myself and I forgot to remove it. I was referencing a test in the gamma scaling MR that I copied to create this test.

[tylerjereddy]

what future regression tests? this comment is confusing at the moment

[aganguly-lab]

Removed.

[tylerjereddy]

It was trivial for me to set d_scaling=False and still have the tests pass by scaling outside the estimator using the diff below the fold.

Details

--- a/src/gfdl/tests/test_model.py
+++ b/src/gfdl/tests/test_model.py
@@ -899,12 +899,15 @@ def test_scaling_classifier(d_scaling,
     scaler = StandardScaler()
     X_train_s = scaler.fit_transform(X_train)
     X_test_s = scaler.transform(X_test)
+    if d_scaling:
+        X_train_s = X_train_s / np.sqrt(X_train_s.shape[1])
+        X_test_s = X_test_s / np.sqrt(X_test_s.shape[1])
 
     model = GFDLClassifier(hidden_layer_sizes=[100],
                            activation="sigmoid",
                            weight_scheme="he_normal",
                            seed=0,
-                           d_scaling=d_scaling,
+                           d_scaling=False,
                            p_scaling=p_scaling,
                            activation_scale=activation_scale,
                            direct_links_scale=direct_links_scale)

I don't see a reason to add support for scaling operations that may be performed outside the estimators--the testing burden for those should fall back to https://lisdi-git.lanl.gov/gfdl_research/gfdl_error_model/-/merge_requests/1, just as it does for any consumer of any sklearn estimator that makes dubious scaling decisions before calling the estimator.

[aganguly-lab]

dscaling has been removed.

[tylerjereddy]

In addition to the obvious issue with d_scaling I've already described, there are also some concerns around p_scaling and "extra scaling" in general:

1. As noted recently in house at https://lisdi-git.lanl.gov/gfdl_research/gfdl_error_model/-/work_items/7, we can apparently achieve very similar behavior to p_scaling by using hyperparameters that we already have access to by default:

aganguly and I noted that lecun_normal or the like with relu is very similar to p-scaling

We'd generally want to avoid adding hyperparameters that are largely redundant.

2. The "defense" for the "extra scaling" added in the theoretical model at the broader team meeting on August 25/2026 mostly centered around exploding gradients in backpropagated models. It wasn't at all clear why that would apply to us in our "gradient free" estimators with mostly frozen weights, nor why our currently-existing scaling/activation approaches wouldn't be sufficient to protect us.

3. It isn't even clear which of the many closed-form and iterative solvers available to sklearn Ridge Kyle's theory actually applies to, as I detail internally at https://lisdi-git.lanl.gov/gfdl_research/gfdl_error_model/-/work_items/3#note_174877. Any of the seven options may be selected at runtime at the moment, and we do not control that ourselves as noted at ENH: expose option to switch Ridge solver? #69. If the solvers that do/do not apply are clear to the mathematicians, it certainly hasn't been clearly articulated here nor in the plans for gfdl adoption in the comparisons to the theory work.

[aganguly-lab]

1. This is true if we specifically stick to the relu activation function. This is because the relu function commutes with scaling, so we can pull the lecun_normal scaling outside the activation function. It is not as clear that this would hold for other activation functions, though there is definitely a strong case for testing this!

2: During the meeting, they provided defenses of scaling for backpropagated models. However, as I mentioned in the meeting, I think the role of scaling for us is for regularization. Intuitively, ridge regularization slightly increases the training error in exchange for reducing the magnitude of the coefficients. This prevents the model from assigning large coefficients (and therefore prediction influence) to largely irrelevant nodes which would result in large test errors. The problem is, when our model is over-parameterized, most coefficients will be small, including coefficients of important nodes. So the regularization parameter (reg_alpha) becomes largely useless. Scaling artificially increases these parameters to a point where they are affected by the regularization. If I'm right, this is important for our model to demonstrate double descent.

The plot below is a little contrived, but it illustrates the picture. The data is perfectly linear (no noise). Because of that, adding more hidden layers actually increases our test error because we add several unnecessary terms to our ridge regression. And as the size of the hidden layer increases, the model allocates a lot of predictive power to the non-linear terms.

However, when we add scaling, the regularization parameter is able to down-regulate the coefficients of these non-linear terms and we get good predictions.

Of course, as you said in point 1, it is possible that these problems can be avoided through the use of scaled weighting schemes such as lecun_normalization and proper preprocessing of the data X. And this phenomenon may not be as apparent when we use real data. However, Kyle's theoretical work uses unscaled normal distributions, so for now we need these scaling terms to coordinate with his results.

3: I'm a little confused by this. If I understand correctly, all seven methods should all be mathematically equivalent, only differing in their efficiency and numerical stability? I don't believe either is a factor in Kyle's theoretical work, at least for the moment. So his results should be ambivalent to the exact choice of solver except in some pathological cases in which we run into numerical instability.

[tylerjereddy]

I think the role of scaling for us is for regularization.

I suppose the challenge is convincing the reader that we can't accomplish the same thing with what we already have in most practical scenarios. Over a large enough number of weird datasets any weird scaling will eventually be the perfect choice of scaling, again back to "no free lunch."

[tylerjereddy]

I did a second round of review this morning--things look like they were a bit rushed on the last few commits without really checking the diff.

[tylerjereddy]

Why are you deleting a comma in a file that isn't related to the work at hand?

Have you been checking your git diff locally before pushing up? Inspecting your diff on GitHub after you push it up? Try to do that--I believe this is my second review comment on this file that should be unrelated to the theme of this PR, and it can distract from the core review process.

[aganguly-lab]

Ok, I apologize. I was a bit rushed for these last commits; they took longer than I had anticipated and I was a little impatient. In future I will try to remember to take a moment and run the relevant checks (git diff, ruff, lint, pytest) locally before committing.

[tylerjereddy]

Why are you changing the typing signatures for arguments that shouldn't be touched in this PR? Did you check that your version of ruff matches the one used in the CI? git grep -E -i "ruff" will show you locally.

Even if you don't do that--when you inspect your diff locally, or on GitHub, you should notice the pollution of unrelated changes that can be distracting to the reviewer.

[aganguly-lab]

Ok, should I revert these unrelated changes with a new commit?

[tylerjereddy]

You could, just check your commit(s)/diffs/commit messages carefully, look at the diff before/after you push it up, etc. You want to make the presentation as clean as possible for the reviewer.

And similarly, you should expect the same from me/Seth, so we can keep the code clean/sane.

[tylerjereddy]

You shouldn't be touching the lines related to rtol in your PR--you're not adjusting rtol

[aganguly-lab]

fixed

[tylerjereddy]

activation_scaling isn't a real variable or argument name--this seems rushed/not checked

Also, the commit adding this content doesn't have a prefix matching the standards we discussed during onboarding (DOC for documentation): https://numpy.org/devdocs/dev/development_workflow.html#writing-the-commit-message

[aganguly-lab]

Should I rebase to update the commit messages?

[tylerjereddy]

nah, just try to format/label properly moving forward

[tylerjereddy]

direct_links_scaling isn't a variable or argument name in this code base per git grep -E -i "direct_links_scaling", "square root" is two words, etc. Try to check work a bit more before presenting for review/pushing up.

[aganguly-lab]

The reference to direct_links_scaling instead of direct_links_scale was fixed in the next commit. I have now fixed the spelling of "square root."

[tylerjereddy]

why are these arguments unrelated to your code changes being modified here? adds line noise--please check diffs before pushing up..

[aganguly-lab]

fixed

[tylerjereddy]

these changes are all just adding noise for reviewer--please check your diff before pushing up and make sure you use the appropriate ruff version by grepping the repo to see where we are pinned

[aganguly-lab]

fixed

[tylerjereddy]

Why these completely unrelated changes? You should be able to see in the diff that they are not related to the work at hand. You're probably using an incorrect version of ruff, but regardless, you should be able to see the line noise if you spend a few seconds looking at your diff before pushing up.

[aganguly-lab]

fixed

[tylerjereddy]

Although unlikely, np.unique could still raise an error and mask the behavior you're trying to enforce--best to hoist it up..

[aganguly-lab]

fixed

[tylerjereddy]

I did another round of review here. Things are still looking a bit messy/rushed, which makes me not want to dive into the deeper details.

I'd suggest:

1. Try to triple check your diff before pushing up each time/presenting for review.
2. Try to get Seth to give you a detailed round of review before I give a round of review.
3. Try to make sure that everything that gets tested in the CI testing is passing locally before you push up.

[tylerjereddy]

The CI is correctly failing because you didn't check your ruff result locally before pushing up--see recent review comments about rushing/attention to detail.

[tylerjereddy]

This code is not currently hit by regression testing--try to make sure your lines are covered before you push code up for review.

Checking locally on the latest version of this branch: python -m pytest -n 8 --cov=. --cov-report=html I can see the red lines for lack of coverage:

[tylerjereddy]

self.pscaling isn't a variable I don't think

[tylerjereddy]

this comment contributes nothing that isn't already obvious from the code block below

[tylerjereddy]

unrelated whitespace changes?

[tylerjereddy]

Why does the reader need to know this? The default is already single layer handling anyway.

[tylerjereddy]

I already mention this kind of thing elsewhere, but git grep -E -i "activation_scaling" returns no results on this branch. Please read your diff at least 3 times before presenting for review/pushing up. Especially with all the AI and linting tools available, this kind of stuff should only show up occasionally these days.

[tylerjereddy]

I already mention this kind of thing elsewhere, but git grep -E -i "direct_links_scaling" returns no other results, so this need to be fixed up.

Try to triple check your diff before pushing up/presenting for review.

[tylerjereddy]

That's not true--this makes sure an error is correctly raised when partial_fit() is called with the fancy new scaling terms. You can't prevent it from being called really.

[tylerjereddy]

this comment contributes nothing, it says load_digits below, please remove it