Skip to content

Torch: reject unknown scaling values instead of silent NOPScaler#3293

Open
shaun0927 wants to merge 1 commit intoawslabs:devfrom
shaun0927:fix/scaling-literal
Open

Torch: reject unknown scaling values instead of silent NOPScaler#3293
shaun0927 wants to merge 1 commit intoawslabs:devfrom
shaun0927:fix/scaling-literal

Conversation

@shaun0927
Copy link
Copy Markdown

Issue #, if available: follow-up to #3251

Description of changes:

Since #3251, the DL estimator modules (patch_tst, lag_tst, d_linear, tide) accept scaling: Optional[str], with "mean" and "std" each picking a scaler and None intentionally falling back to NOPScaler. The current control flow is:

if scaling == "mean":
    self.scaler = MeanScaler(keepdim=True)
elif scaling == "std":
    self.scaler = StdScaler(keepdim=True)
else:
    self.scaler = NOPScaler(keepdim=True)

The final else matches both the intended None opt-out and any typo or unsupported value ("standard", "minmax", "mean ", …). Users who mistype silently get un-scaled inputs with no warning, and the combination of NOPScaler + StudentTOutput is exactly the NaN pathway that #3265 now has to recover from at training time.

This PR keeps the None opt-out, but raises ValueError for any other string. The fix is four identical blocks, one per module:

if scaling == "mean":
    self.scaler = MeanScaler(keepdim=True)
elif scaling == "std":
    self.scaler = StdScaler(keepdim=True)
elif scaling is None:
    self.scaler = NOPScaler(keepdim=True)
else:
    raise ValueError(
        f"Unknown scaling={scaling!r}; "
        "expected one of 'mean', 'std', or None."
    )

Trade-offs considered

  • Tightening the estimator field type to Literal["mean", "std"] (pydantic would reject at construction): rejected. It would change the serialized representation for existing users and any saved predictor JSON that stored scaling: "mean" as a plain string. The runtime ValueError path is lower blast-radius and catches the same garbage.
  • Warn-and-fallback: rejected. A silent fallback hides the typo exactly when it matters; by the time the user notices NaN loss they are several epochs in.

Verification

All four estimators were instantiated and create_lightning_module() called for each of "mean", "std", None, and "garbage_value":

PatchTSTEstimator scaling='mean':  MeanScaler OK
PatchTSTEstimator scaling='std':   StdScaler OK
PatchTSTEstimator scaling=None:    NOPScaler OK
PatchTSTEstimator scaling='garbage_value': REJECTED ValueError OK
... (same for LagTSTEstimator, DLinearEstimator, TiDEEstimator)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Please tag this pr with at least one of these labels to make our release process faster: BREAKING, new feature, bug fix, other change, dev setup

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant