Fix division by zero in math operations - #253
Conversation
joeloskarsson
left a comment
There was a problem hiding this comment.
Thanks, these are good changes to make the code more robust. Would it be possible to also add some test that checks that we don't run into numerical issues with the chosen constants?
|
@joeloskarsson Thanks for the feedback! I've just pushed a commit with the updates: Warning: Added a UserWarning in ar_model.py that alerts the user and lists the indices if any variables receive a feature weight of exactly 0.0 (excluding them from the loss). Tests: Added test_numerical_stability.py with 5 pure tensor unit tests. These verify that the softplus epsilon (1e-6) and the feature weight epsilon (1e-8) successfully prevent exact zeros and subnormals, keeping downstream loss metrics (wmse, wmae, per_var_std) perfectly finite even with worst-case or zero-weight inputs. Let me know if you need anything else! |
|
please link this issue in your PR description: #526 |
Re-applies @Ayushhgit's PR mllam#253 onto current main, scoped down to the clear-cut feature-weights fix and rebased onto the post-mllam#208 layout: - The original PR also added `+ 1e-6` to `softplus(pred_std_raw)` to guard against float32 underflow at very negative `pred_std_raw`. That overlapped with mllam#523's `* self.diff_std` scaling on the same line and is largely redundant once pred_std starts on the empirical scale, so dropped here to avoid coupling the two PRs. - Target file `models/ar_model.py` is gone since mllam#208; fix is applied at `models/module.py:120` next to the existing `per_var_std` registration. - Use `torch.finfo(torch.float32).eps` instead of a hard-coded `1e-8` for the sqrt epsilon, matching the eps choice used a few lines below for `state_std` / `forcing_std`. - Slimmed tests/test_numerical_stability.py to the 3 feature-weights assertions (finite output, eps does not perturb non-zero weights, warning fires with the zero index). Dropped the softplus / wmae / wmse integration tests since the softplus piece is no longer in scope. Refs mllam#526. Co-Authored-By: Ayush <Ayushhgit@users.noreply.github.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
c80a1b7 to
ae8550d
Compare
There was a problem hiding this comment.
Thanks @Ayushhgit, rebased onto current main and scoped down. Two changes:
- Dropped the
softplus(pred_std_raw) + 1e-6piece - that line overlaps with #523's* self.diff_stdscaling, and oncepred_stdstarts on the empirical scale via #523 the underflow concern is largely moot. Better to land the two ideas separately so they don't fight. - Switched to
torch.finfo(torch.float32).epsfor the sqrt eps (matches the existingstate_std/forcing_stdepsilon choice a few lines below).
tests requested by @joeloskarsson have been added before. please review
Refs #526. Merging after CI is green.
|
Thanks for adding the tests, but I am a bit confused here since they don't call any of the actual code. E.g. mirroring the |
Describe your changes
Summary of the changes:
1e-6epsilon to the output oftorch.nn.functional.softplusin [base_graph_model.py].1e-8epsilon to theself.feature_weightsinverse square root calculation insideARModel.__init__.Motivation and context:
These changes resolve two separate division-by-zero crashes:
pred_stdusing softplus, inputsfloat32will evaluate exactly to0.0. This zero denominator causes cascadingNaNoutputs instantly destroying training across metrics like wmse or wmae.0.0for specific state variables they want the model to ignore, creatingper_var_stdtriggers a scalar division by zero, crashing model initialization. The epsilons bring numerical stability against these mathematical singularities.Dependencies:
None
Issue Link
N/A
Type of change
Checklist before requesting a review
pullwith--rebaseoption if possible).Checklist for reviewers
Each PR comes with its own improvements and flaws. The reviewer should check the following:
Author checklist after completed review
reflecting type of change (add section where missing):
Checklist for assignee