chore: timeSeries deep forecasters: correct autodiff training (GradientTape + IEngine) + GPU-residency seam#1841
Merged
Merged
Conversation
…model actually learns
NHiTSModel.TrainCore looped over ForwardWithGradients/ApplyGradients, but
ForwardWithGradients built an EMPTY gradient dictionary ("// Backward removed")
so ApplyGradients updated nothing — training was a no-op that returned an
untrained model.
Fix (mirrors the working NBEATSModel path):
- Re-express the stack MLP forward under GradientTape<T> using IEngine tensor
ops (NHiTSStackTensor.ForwardTape: TensorPermute / TensorMatMul /
TensorBroadcastAdd / ReLU). Autodiff now produces gradients for every
weight/bias and AdamOptimizer.Step applies them. The forward is fully
GPU-dispatchable (the point of the residency campaign).
- Register every weight/bias via RegisterTrainableParameter so
TapeTrainingStep.CollectParameters picks them up.
- TrainCore: z-normalize the series, build [B,L]->[B,H] windowed batches,
run tape forward over all stacks (multi-rate avg-pooled inputs), MSE loss,
tape.ComputeGradients + optimizer.Step. Denormalize at inference.
- Fix a train/inference mismatch: inference ForwardInternal used a hand-rolled
scalar 2D-indexer matmul that disagreed with the tape path (~3500x off),
producing garbage predictions from correctly-trained weights. It now uses the
same Engine.Tensor* ops as ForwardTape, so a trained model's inference output
matches what training optimized.
- Add best-checkpoint / early-stopping restore: snapshot params at the best
epoch and restore at the end, so late-epoch mini-batch Adam noise cannot
degrade the returned model.
- Remove the dead hand-derived Backward/UpdateParameter/GetParameterNames and
the unused activation caches.
Public API and NHiTSOptions are unchanged.
Verification on a noisy sinusoid+trend series (default hyperparameters,
lookback 20 / horizon 8 / 3 stacks): windowed forecast MSE 287.08 -> 18.44
(ratio 0.064, well under 0.5), beats the repeat-last-value baseline (21.85),
and probe forecasts track actuals closely (e.g. 18.13 vs 18.16, 16.87 vs
17.07). Before this change the model returned untrained output.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-dispatchable) Re-express InformerModel<T> training with automatic differentiation and batched Engine.Tensor* ops, mirroring the NBEATSModel / NHiTSModel tape pattern, so autodiff produces the whole backward pass (no hand-derived gradients) and the model runs through the GPU engine. Model (src/TimeSeries/InformerModel.cs): - TrainCore: z-normalizes the series, stacks windows into a [B, L] batch, runs a batched tape forward (ForwardBatch), MeanSquaredErrorLoss.ComputeTapeLoss, tape.ComputeGradients, AdamOptimizer.Step. Supervises the full H-step horizon per sample; snapshots/restores best-epoch parameters. - ForwardBatch: batched [B,L,1] -> [B,H] forward built entirely from Engine.* ops (embedding, multi-head scaled-dot-product attention via ScaledDotProductAttention with the [B,H,S,headDim] layout, LayerNorm, ReLU FFN, batched distilling conv+maxpool, generative decoder, per-position output projection). Token-wise ops run on a flattened [B*S, d] matrix. - ATTENTION SIMPLIFICATION: replaced ProbSparse self-attention with full multi-head scaled-dot-product attention. ProbSparse's top-u query selection is a data-dependent host gather that neither batches nor differentiates cleanly, and for these sequence lengths it already reduces to full attention. Self-attention distilling is retained (now trained: its conv weights were added to CollectTrainableParameters). - Inference (ForwardEngine/PredictSingle/Predict/ForecastHorizon) uses the SAME batched Engine ops (B=1), normalizing the input and denormalizing the forecast — no scalar/tape divergence. Normalization stats are serialized for round-trip. - Removed the dead hand-derived gradient/scalar code: ComputeGradients/ForwardWithCache/ EmbedInput/CreateDecoderInput/ComputeOutput/AccumulateGradients/ApplyGradients and the scalar Forward/Backward/ProbSparse/FeedForward/attention in the encoder/decoder/distilling layer classes (~630 lines). Public API and InformerOptions unchanged. testconsole: informer-verify (double CPU correctness) and informer-gpu (float32 GPU util) harnesses + Program dispatch entries. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…fused plan Adds a reusable GPU-residency path for the tensorized TimeSeries forecasters and wires NBEATSModel<float> to it, following NeuralNetworkBase's compiled fused-optimizer template. TimeSeriesModelBase<T>: - CanTrainOnGpu guard (float + DirectGpuTensorEngine + compilation enabled), mirroring NeuralNetworkBase.CanTrainOnGpu. - TryFusedResidentStep(...): thin reusable wrapper over CompiledTapeTrainingStep<T>.TryStepWithFusedOptimizer that compiles forward+backward+Adam into a single on-device graph (weights, activations and Adam moments stay resident; no per-op host<->device round-trip). Applies gradient clipping (maxGradNorm=1.0) to match the eager Adam path. NBEATSModel<T>: - TryTrainGpuResident: runs the doubly-residual stack through the fused compiled plan with a constant batch shape (required for capture/replay). - Correctness-first validation gate: the resident run is kept only if it improves validation MSE over the untrained baseline; on divergence or no-improvement it re-initializes the blocks and returns false so TrainCore falls back to the eager tape path. Gated to epoch-bounded mode so a rejected attempt can't consume a wall-clock training budget. - LastRunEpochLosses / LastRunUsedGpuResidentPath instrumentation (both paths). Behavior: - The double/CPU training path is unchanged (CanTrainOnGpu is float+GPU only); verified NBEATS still trains cleanly on CPU (normalized loss 0.44 -> 0.0004). - Float+GPU: the eager tape already dispatches every op to the CUDA engine; the resident attempt is validated and safely falls back to eager when the compiled plan doesn't reproduce eager gradients. Known limitation (precise blocker): on the currently-linked Tensors build the compiled fused plan does not reliably reproduce eager gradients for NBEATS's per-layer TensorPermute + TensorBroadcastAdd op graph, so the resident attempt typically validation-rejects for NBEATS and hands off to eager. NBEATS is also host-bound (small per-op tensors), which caps GPU occupancy. The seam is retained for TimeSeries models whose op graphs the compiler handles faithfully. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
github-actions
Bot
changed the title
Contributor
|
🤖 PR Title Auto-Fixed Your PR title was automatically updated to follow Conventional Commits format. Original title: New title: Detected type: Valid types and their effects:
If the detected type is incorrect, you can manually edit the PR title. |
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (14)
WalkthroughInformer, Autoformer, DeepAR, N-BEATS, N-HiTS, and TFT now use tape-based tensor training, normalized forecasting, updated persistence, and optional GPU-resident fused execution. Console verification and profiling commands were added for Informer and Autoformer. ChangesTime-series training modernization
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant TimeSeriesModel
participant Engine
participant GradientTape
participant AdamOptimizer
TimeSeriesModel->>Engine: build normalized tensor forecast
Engine->>GradientTape: record tensor operations
TimeSeriesModel->>GradientTape: compute training loss
GradientTape->>AdamOptimizer: provide gradients
AdamOptimizer->>TimeSeriesModel: update trainable tensors
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/TimeSeries/NBEATSModel.cs`:
- Around line 295-313: Disable the GPU-resident training path until
compiled/eager gradient and parameter-update parity is established: remove the
TryTrainGpuResident invocation from the main training flow and prevent its
related helpers from being reachable in production. Preserve the eager training
path as the only active fallback, and retain or clearly isolate the resident
implementation for future compatibility without executing it.
- Around line 539-541: The acceptance gate in NBEATSModel’s resident
optimization validates on the same windows used for training. Reserve a
time-ordered holdout from the validation data, exclude those windows from
order/resident optimization, and use the holdout for preMse and all subsequent
ValidationStackMse comparisons in the fallback logic around the resident result.
Update the related validation paths at ValidationStackMse call sites near the
resident acceptance and eager fallback so both fused and eager models are
evaluated on the untouched holdout.
- Around line 76-95: Remove the public LastRunEpochLosses and
LastRunUsedGpuResidentPath members from NBEATSModel; keep these diagnostics
internal for verification or expose them through the supported AiModelResult
facade. Ensure any retained loss collection is exposed via an immutable or
read-only snapshot so callers cannot mutate internal state, while preserving the
existing training behavior.
- Around line 559-585: Remove the unreachable time-bounded logic from the
training loop in TrainCore: delete the timeBounded and stopwatch declarations,
use _options.Epochs directly as the loop bound, and remove both timeout-check
branches while retaining cancellation checks. Update related control flow so
epoch and batch iteration remain unchanged.
- Around line 602-615: Update the failure branch in the training loop around
TryFusedResidentStep: whenever ran is false, return false to trigger TrainCore’s
eager fallback, including after fusedEngaged has already been set. Remove the
continue path and any associated partial-run handling so a failed fused step is
never silently skipped.
In `@src/TimeSeries/NHiTSModel.cs`:
- Around line 175-197: Validate the input series at the start of the
training/prediction workflow before normalization: reject empty series and any
series shorter than LookbackWindow + ForecastHorizon with a clear argument
exception. Define and retain the required-length validation variables once, then
reuse them in the later training-window logic around the affected sections
instead of redeclaring them; ensure invalid inputs cannot proceed to division by
zero or a zero-update training loop.
- Around line 82-86: Persist _normMean and _normStd in NHiTSModel’s versioned
serialization payload and restore them during deserialization so reloaded models
produce identical forecasts; add appropriate version gating or
backward-compatible defaults for existing model files, and update the payload
version/schema handling consistently.
- Around line 132-136: Validate every value in PoolingKernelSizes before the
downsampledLength calculation in the NHiTSModel constructor or initialization
path, rejecting zero and negative pooling sizes with a clear argument-validation
exception; only perform the ceil-division after validation, including the
originating option/index in the error where practical.
- Around line 318-346: Checkpoint selection in the training loop compares
pre-update batch losses against an end-of-epoch parameter snapshot. Update the
logic around the epoch snapshot and `EvaluateFrozenEpochLoss` so that, after
training, it evaluates every valid window using the eager normalized forward
path with the current frozen parameters, computes the mean MSE, and uses that
value for finite best-loss comparison before saving `bestSnapshot`.
In `@src/TimeSeries/TimeSeriesModelBase.cs`:
- Around line 140-143: Change the GPU implementation hooks in
TimeSeriesModelBase, including CanTrainOnGpu and the related members around the
referenced section, from protected to private protected so only subclasses
within the defining assembly can access them while external subclasses cannot.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5dcc0e2c-66f4-4eb2-9ba7-e02b12c4ed89
📒 Files selected for processing (6)
src/TimeSeries/InformerModel.cssrc/TimeSeries/NBEATSModel.cssrc/TimeSeries/NHiTSModel.cssrc/TimeSeries/TimeSeriesModelBase.cstestconsole/InformerProfile.cstestconsole/Program.cs
…used plan Mirrors NBEATSModel.TryTrainGpuResident so N-HiTS routes forward + backward + Adam through a single on-device compiled plan (weights / activations / Adam moments resident across every step) when float + DirectGpuTensorEngine + compilation are all live. * PoolBatchedTape: batched, tape-recordable average pooling via Reshape + ReduceMean. [B, L] -> [B, L/k]. Requires L % k == 0; returns null (fused path unsupported) for configs where the pool size doesn't divide the lookback cleanly, so callers cleanly fall back to the eager scalar pool. * RunForwardBatched: full multi-rate stack on-tape via per-stack PoolBatchedTape + ForwardTape + sum. Single [B, L] input suits the fused step's constant-shape replay contract. * TryTrainGpuResident: NBEATS-parallel structure — pre-MSE baseline, batch loop through TryFusedResidentStep, divergence guard, validation gate. On reject the stacks are re-initialized so the eager fallback starts clean. * LastRunUsedGpuResidentPath: telemetry flag on the model instance. * TrainCore now attempts the resident path first (epoch-bounded only, per the same wall-clock hazard NBEATS docs) and falls through to the existing eager tape loop on failure. Builds green net471 / net8.0 / net10.0. Correctness gate rejects the run if it doesn't improve validation MSE by 2%, guaranteeing the resident path can never ship worse weights than eager. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…d fused plan Same seam NBEATSModel + NHiTSModel use. Informer's forward is SDPA + LayerNorm + FFN + distilling — a very different op graph from NBEATS's Permute + BroadcastAdd chain that trips the divergence guard on the linked Tensors build, so the compiled fused plan should engage cleanly here even while NBEATS keeps falling back. * CollectTrainableLayers: flat ITrainableLayer<T> list over encoder + distilling + decoder layers (all LayerBase-derived). Needed by TryFusedResidentStep to ZeroGrad per step. * ValidationMseGpu: 256-window baseline for the accept/reject gate. * TryTrainGpuResident: NBEATS-parallel structure — pre-MSE, batch loop through TryFusedResidentStep, divergence guard, validation gate. On reject calls InitializeModel() so the eager fallback starts clean. * LastRunUsedGpuResidentPath telemetry flag. * TrainCore attempts the resident path first (epoch-bounded only, same rationale as NBEATS/NHiTS) and falls through to the existing eager tape+optimizer loop on failure. Builds green net471 / net8.0 / net10.0. This completes wiring for the three tape-based deep forecasters currently on the PR branch (NBEATS + NHiTS + Informer). Autoformer / DeepAR / TFT branches referenced in the PR body haven't landed on the remote yet — will be wired the same way when they do. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/TimeSeries/NHiTSModel.cs (1)
784-796: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Clone()still aliases_stacks; this is a shallow copy.
clone._stacks.AddRange(_stacks)reuses the sameNHiTSStackTensor<T>instances, so mutating or training the clone also mutates the original, andDeepCopy()inherits the same bug. Recreate each stack independently instead of copying the list references, e.g. via per-stack serialization/deserialization or a dedicated copy path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/TimeSeries/NHiTSModel.cs` around lines 784 - 796, Clone() shallow-copies _stacks, causing the original and clone to share NHiTSStackTensor<T> instances; replace AddRange(_stacks) with independent deep copies using the stack serialization/deserialization mechanism or a dedicated copy method. Ensure DeepCopy() also receives fully independent stacks while preserving the existing training series, model parameters, and normalization values.src/TimeSeries/InformerModel.cs (1)
349-360: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftScore the checkpoint on frozen weights, not the running model.
epochLossis accumulated from batch losses taken before eachoptimizer.Step, butbestSnapshotstores the post-epoch parameters. Recompute the loss for those saved weights (or use a validation pass) before comparing againstbestLoss.src/TimeSeries/InformerModel.cs:349-360🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/TimeSeries/InformerModel.cs` around lines 349 - 360, Update the epoch checkpoint logic around the training loop and bestSnapshot assignment so the candidate score is computed using the post-epoch frozen parameters, not the pre-optimizer-step batch-loss average. After saving or cloning the current weights, run a loss/validation evaluation with those weights, then compare that recomputed score against bestLoss and retain the snapshot only when it improves.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/TimeSeries/InformerModel.cs`:
- Around line 507-517: InitializeModel currently appends layers when called
after a rejected GPU-resident run, creating duplicate model layers. Update
InitializeModel to clear _encoderLayers, _distillingLayers, and _decoderLayers
before rebuilding them; this is safe for initial construction and ensures eager
fallback has exactly one layer set.
---
Outside diff comments:
In `@src/TimeSeries/InformerModel.cs`:
- Around line 349-360: Update the epoch checkpoint logic around the training
loop and bestSnapshot assignment so the candidate score is computed using the
post-epoch frozen parameters, not the pre-optimizer-step batch-loss average.
After saving or cloning the current weights, run a loss/validation evaluation
with those weights, then compare that recomputed score against bestLoss and
retain the snapshot only when it improves.
In `@src/TimeSeries/NHiTSModel.cs`:
- Around line 784-796: Clone() shallow-copies _stacks, causing the original and
clone to share NHiTSStackTensor<T> instances; replace AddRange(_stacks) with
independent deep copies using the stack serialization/deserialization mechanism
or a dedicated copy method. Ensure DeepCopy() also receives fully independent
stacks while preserving the existing training series, model parameters, and
normalization values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4c6092c4-5607-45c3-bb10-741cabbadf69
📒 Files selected for processing (2)
src/TimeSeries/InformerModel.cssrc/TimeSeries/NHiTSModel.cs
4 tasks
…e ops Re-express the TFT forward pass entirely with GradientTape + batched Engine.Tensor* ops so autodiff produces the backward pass (no hand-derived gradients) and every op is GPU-dispatchable — matching the NHiTS/Informer tape conversions. - ForwardBatch: [B,L]->[B,H] point forecast via embedding + positional encoding, softmax-gated variable selection (GRN), static-enrichment GRN, interpretable multi-head ScaledDotProductAttention, post-attention gated skip (GRN), mean-pool + forecast head. - GatedResidualNetwork rewritten batched/tape-safe (Engine ELU/Sigmoid/ LayerNorm with learned gamma/beta); removes scalar NumOps activation loops. - TrainCore: batched tape forward -> MSE -> ComputeGradients -> Adam.Step, z-normalized windows, best-epoch snapshot/restore. Inference (ForwardEngine) uses the identical ops. - Simplifications (documented): quantile/pinball head -> single point (mean) MSE forecast; sequential LSTM -> positional encodings + attention; univariate variable selection -> learned per-channel softmax gate. GRN + variable selection + interpretable attention retained. - Remove dead scalar VariableSelectionNetwork.cs. Public API and TemporalFusionTransformerOptions unchanged. Verify (standalone harness, double, sinusoid+trend series, 1100 pts, 40 ep): MSE before 205.45 -> after 0.472 (0.23% of init, <0.5x); beats naive repeat-last baseline 39.09 by ~83x. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…and-derived gradients) Convert AutoformerModel<T> training to a GradientTape + IEngine tensor-op forward so autodiff produces the entire backward pass and every op is GPU-dispatchable (matching the NHiTS/Informer conversions). - ForwardCore: whole forward (embedding, series-decomposition moving-avg trend/seasonal split, encoder/decoder, output projection) is now built from Engine.Tensor* ops. The DEFINING series decomposition is kept, and a tape-differentiable time-delay auto-correlation (R(lag) spectrum -> top-k softmax -> rolled-value aggregation) replaces the old scalar FFT-style attention; only the top-k index SELECTION is non-differentiable (as in the official implementation). - TrainCore rewritten: z-normalize the series, build [L]->[H] windows, mini-batch gradient by accumulating each sample's gradient across separate (immediately-disposed) tapes, then one averaged Adam step per batch; best-epoch params snapshotted and restored. Removed the output-bias level seeding (normalization handles level). - Inference (Predict/PredictSingle/PredictMultiple) uses the SAME ForwardCore ops (normalize in, denormalize out) so there is no train/predict divergence. - Removed the dead scalar/hand-derived code: ComputeGradientsMultiStep, ComputeMovingAverageNode, TopologicalSort, Accumulate/ApplyGradients, ForwardWithCache, AutoformerCache, the gradient-accumulator infrastructure, the ComputeGradients override, and all per-layer scalar forward/gradient methods (~1400 net lines removed). Layers now hold parameters + (de)serialization only. - Normalization stats persisted in Serialize/DeserializeCore. Public API and AutoformerOptions unchanged. Adds a testconsole `autoformer-verify` verb (trains on a learnable noisy sinusoid+trend, reports before/after horizon MSE vs a repeat-last baseline). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Re-express DeepARModel<T> training with GradientTape<T> + Engine.Tensor* ops so autodiff produces the full backward pass (no hand-derived gradients) and the forward path is GPU-dispatchable, mirroring the NHiTS/Informer/NBEATS conversions. - Unroll the LSTM recurrence over the L-step lookback as tape-tracked engine ops (gate matmuls via TensorMatMul/TensorBroadcastAdd, gates split with TensorNarrow, Sigmoid/Tanh/TensorMultiply), carrying [H,B] column-major hidden/cell state across steps under the tape. Two internal LayerBase subclasses (DeepARLstmCellTape, DeepARGaussianHead) register every weight/bias via RegisterTrainableParameter so TapeTrainingStep.CollectParameters picks them up. - TrainCore: batched teacher-forced forward -> loss -> tape.ComputeGradients -> AdamOptimizer.Step, with per-epoch best-checkpoint snapshot/restore. Inference (PredictDistribution) runs the identical Step ops (B=1) so there is no scalar/tape divergence. Removed all hand-derived scalar gradient code (ComputeGradients/ApplyGradients/BackwardInternal). - Gaussian likelihood retained: mean trained on MSE (undivided gradient) and the scale trained on the Gaussian NLL term over a StopGradient-ed mean, so sigma cannot inflate to collapse the mean (verified failure mode). Mean is emitted as a residual on the current observation (mu_t = x_t + delta(h_t)), which keeps the model at the persistence prior at init and lets training learn the one-step change. - Public API + DeepAROptions unchanged; added read-only TrainingLossHistory. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nput validation, checkpoint Addresses CodeRabbit review comments on PR#1841 (the clear, non-design ones; the GPU-resident gradient root cause is handled separately): - API visibility (keep the public/external-subclass surface unchanged, per the facade objective): TimeSeriesModelBase.CanTrainOnGpu and TryFusedResidentStep are now `private protected` (still usable by in-assembly models, hidden from external subclasses); NBEATS/NHiTS/Informer LastRunUsedGpuResidentPath and NBEATS LastRunEpochLosses are now `internal` (visible to tests/serving via InternalsVisibleTo). LastRunEpochLosses is exposed as an immutable snapshot (AsReadOnly) so callers cannot mutate the backing list. - InformerModel.InitializeModel is now idempotent — it clears _encoderLayers, _distillingLayers and _decoderLayers before rebuilding. TryTrainGpuResident re-invokes it on a rejected resident run; previously it APPENDED a second full layer set, doubling the model and corrupting ParameterCount/ForwardBatch/the forecast. Safe on the constructor path (lists already empty). - NHiTSModel now validates external inputs: every PoolingKernelSize must be positive (a zero divided by zero in the downsampled-length calc; a negative gave an invalid length), and a training series shorter than LookbackWindow + ForecastHorizon is rejected up front (an empty series divided by zero in the mean pass; a short series trained silently on zero windows). - NHiTSModel.SerializeCore/DeserializeCore now persist _normMean/_normStd. A reloaded model previously kept the defaults (0/1) and denormalized forecasts incorrectly, so it no longer matched the original trained model. - NHiTS best-checkpoint selection now scores each epoch on its FROZEN end-of-epoch weights (via the existing ValidationMse forward) instead of the mean of pre-update batch losses. bestLoss now measures exactly the weights bestSnapshot captures, so a late-diverging epoch can no longer be selected as best. The added pass is forward-only (no backprop). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…de, failed-step fallback Addresses the remaining CodeRabbit comments on the N-BEATS / N-HiTS GPU-resident path (the compiled-plan gradient root cause is fixed separately in AiDotNet.Tensors): - [3] Accept/reject gate now validates on a time-ordered HOLDOUT. Both models reserved their windows for BOTH resident optimization AND the pre/post MSE gate, so a 2% training-loss improvement proved nothing about generalization. Reserve the latest ~20% of windows as a holdout the optimizer never trains on; train on the earlier windows; score preMse/postMse on the holdout alone. - [4] Removed the unreachable time-bounded logic from NBEATSModel.TryTrainGpuResident. TrainCore only calls it when MaxTrainingTimeSeconds <= 0, so timeBounded, the stopwatch, and both timeout branches were always dead. The loop now uses _options.Epochs directly and keeps the cancellation checks. (N-HiTS already had no such dead code.) - [5] A failed fused step after the plan engaged now diverges + breaks (→ the gate reinitializes and hands off to eager) instead of silently `continue`-ing the batch, which could leave a partially-executed resident run to be accepted. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Deployment failed with the following error: Learn More: https://vercel.com/franklins-projects-02a0b5a0?upgradeToPro=build-rate-limit |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/TimeSeries/InformerModel.cs (1)
446-463: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBLOCKING: The GPU-resident accept/reject gate validates on its own training set.
Unlike the sibling models,
TryTrainGpuResidenthere builds a singlevalidlist and uses it for both the resident optimization (order = valid.OrderBy(...)) and the pre/postValidationMseGpugate. Because the gate scores the very windows it trained on,postMse < preMse * 0.98is essentially guaranteed, so the gate cannot detect overfitting or an incorrect fused-gradient update — it will accept a resident run that generalizes worse than eager.NHiTSModel(Lines 510‑521) andNBEATSModel(Lines 534‑542) already reserve a time-ordered ~20% holdout for exactly this reason; Informer must do the same.As per path instructions, the correctness fallback must contain production-grade validation rather than a training-set check.
🐛 Proposed fix (mirror NHiTS/NBEATS holdout)
var valid = new List<int>(); for (int idx = L; idx + H <= yNorm.Length; idx++) valid.Add(idx); - if (valid.Count < batchSize) return false; + int holdoutCount = Math.Max(1, valid.Count / 5); + int trainCount = valid.Count - holdoutCount; + var trainWindows = valid.Take(trainCount).ToList(); + var holdoutWindows = valid.Skip(trainCount).ToList(); + if (trainWindows.Count < batchSize) return false;Then score
preMse/postMseonholdoutWindowsand shuffletrainWindows(notvalid) intoorder.Also applies to: 521-533
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/TimeSeries/InformerModel.cs` around lines 446 - 463, Replace the single `valid` window set in `TryTrainGpuResident` with separate time-ordered `trainWindows` and approximately 20% `holdoutWindows`, matching the holdout construction used by `NHiTSModel` and `NBEATSModel`. Use only `trainWindows` when building and shuffling `order` for resident optimization, and pass `holdoutWindows` to both `ValidationMseGpu` calls used for the pre/post acceptance gate; preserve minimum-size validation checks.Source: Path instructions
src/TimeSeries/NHiTSModel.cs (1)
399-409: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBLOCKING: end-of-epoch checkpointing skips non-divisible pooling configs
src/TimeSeries/NHiTSModel.cs:399-409
ValidationMse()goes throughRunForwardBatched()→PoolBatchedTape(), which returnsnullunlessLookbackWindow % PoolingSize == 0. ButInitializeStacks()andForecastHorizon()already use ceil pooling, so those configs train and infer fine in the eager path. For any non-divisible pooling size, the epoch score staysNaN,bestSnapshotis never populated, and the late-epoch restore silently never runs. Score checkpoints through the same ceil-based path as inference, or make the batched scorer handle the remainder.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/TimeSeries/NHiTSModel.cs` around lines 399 - 409, Update the end-of-epoch checkpoint scoring around ValidationMse so non-divisible pooling configurations produce a valid score. Align RunForwardBatched/PoolBatchedTape with the ceil-based pooling behavior already used by InitializeStacks and ForecastHorizon, including remainder windows, or route validation through the equivalent eager path; ensure valid scores populate bestSnapshot and enable the existing restore logic.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/TimeSeries/DeepARModel.cs`:
- Around line 682-688: Change DeepARModel.TrainingLossHistory from public to
internal and expose _epochLosses through a read-only wrapper such as
_epochLosses.AsReadOnly(), matching NBEATSModel.LastRunEpochLosses. Keep the
diagnostic plumbing internal so the public model facade remains unchanged and
callers cannot cast the returned collection back to List<double> or mutate
training state.
In `@src/TimeSeries/TemporalFusionTransformer.cs`:
- Around line 417-432: The public PredictQuantiles method must not return
fabricated identical quantile bands after the quantile head simplification.
Replace its point-forecast result construction with a NotSupportedException that
clearly states quantile forecasts are unavailable because this implementation
only supports point forecasts, and remove or update the misleading
degenerate-output logic and documentation.
In `@testconsole/AutoformerVerify.cs`:
- Around line 89-96: Make verification failures produce a non-zero process
result: in Program.Main, after computing decreased and beatsBaseline, throw an
InvalidOperationException or otherwise return a non-zero exit status when either
criterion is false, while preserving the existing success behavior when both
pass.
- Around line 45-56: The Autoformer verification in Run() reports failed checks
without returning a failure status. Update Run() so that when either assertion
fails, it throws an exception or sets the process exit code to a non-zero value
after printing the failure summary; preserve successful zero-exit behavior when
all checks pass.
---
Outside diff comments:
In `@src/TimeSeries/InformerModel.cs`:
- Around line 446-463: Replace the single `valid` window set in
`TryTrainGpuResident` with separate time-ordered `trainWindows` and
approximately 20% `holdoutWindows`, matching the holdout construction used by
`NHiTSModel` and `NBEATSModel`. Use only `trainWindows` when building and
shuffling `order` for resident optimization, and pass `holdoutWindows` to both
`ValidationMseGpu` calls used for the pre/post acceptance gate; preserve
minimum-size validation checks.
In `@src/TimeSeries/NHiTSModel.cs`:
- Around line 399-409: Update the end-of-epoch checkpoint scoring around
ValidationMse so non-divisible pooling configurations produce a valid score.
Align RunForwardBatched/PoolBatchedTape with the ceil-based pooling behavior
already used by InitializeStacks and ForecastHorizon, including remainder
windows, or route validation through the equivalent eager path; ensure valid
scores populate bestSnapshot and enable the existing restore logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bbf089a3-9039-45e0-826b-cba3ce7a1747
📒 Files selected for processing (11)
src/TimeSeries/AutoformerModel.cssrc/TimeSeries/DeepARModel.cssrc/TimeSeries/InformerModel.cssrc/TimeSeries/NBEATSModel.cssrc/TimeSeries/NHiTSModel.cssrc/TimeSeries/TFT/GatedResidualNetwork.cssrc/TimeSeries/TFT/VariableSelectionNetwork.cssrc/TimeSeries/TemporalFusionTransformer.cssrc/TimeSeries/TimeSeriesModelBase.cstestconsole/AutoformerVerify.cstestconsole/Program.cs
💤 Files with no reviewable changes (1)
- src/TimeSeries/TFT/VariableSelectionNetwork.cs
On noisy / fat-tailed real series the eager tape-Adam training loop diverges: the normalized training loss climbs instead of falling (measured 1.20 -> 1.55 over 15 epochs on SPY daily log-returns) and the final weights produce forecasts blown up to ~hundreds of times the target scale (a walk-forward eval measured N-BEATS prediction variance ~692x the target vs ~0.07-4x for Informer/DLinear). Two coupled guards, both on vectorized Tensor/IEngine ops (no jagged arrays, no per-element scalar loops): 1. Global-norm gradient clipping (Oreshkin et al. 2020). New NBEATSModelOptions.GradientClipNorm (default 1.0). The global L2 norm is computed with Engine.TensorSumOfSquares per gradient and the whole set is scaled in place with Engine.TensorMultiplyScalarInPlace when it exceeds the threshold. 2. Best-epoch-weights checkpointing. Adam's adaptive step makes clipping alone insufficient, so the parameters from the lowest-loss epoch are kept (as Tensor clones, refreshed via Engine.TensorMultiplyScalarInto) and restored after training — inference never runs on a diverged tail. After the fix a 6-fold walk-forward over SPY daily returns keeps every fold's prediction/target std ratio in 0.18x-0.74x (worst max|pred| 0.049 vs a target std of 0.011) — the ~692x blow-up is gone. Existing NBEATSModelTests: 15/15 green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…elper, facade, resident parity Addresses the blocking review comments on the tape-converted time-series models: - TemporalFusionTransformer: restore a REAL quantile head. The tape conversion had collapsed the quantile/pinball head to a single MSE point forecast, so PredictQuantiles returned identical (degenerate) bands. Now the head projects to [H*Q] (quantile-major) and trains with the summed pinball loss over every QuantileLevels level, so PredictQuantiles returns a genuine ordered spread (verified: q0.1 < q0.5 < q0.9 across the horizon); the point forecast is the median-level head. Also validates each quantile level is in (0,1) at construction. - new Random(42) -> RandomHelper.CreateSeededRandom(42) in Informer/NBEATS/NHiTS (deterministic + the project's crypto-secure helper), matching DLinear/Autoformer. - DeepARModel.TrainingLossHistory: public IReadOnlyList (leaked the mutable backing List) -> internal + .AsReadOnly(), matching NBEATSModel.LastRunEpochLosses and the facade-only rule. - NBEATS GPU-resident path: on AiDotNet.Tensors 0.112.0 (#759 fixed the TensorPermute/TensorBroadcastAdd multi-consumer grad-buffer blowup, #764 the resident-parameter mistrain) the resident run now trains faithfully — verified on GPU that it lowers the eager-forward training MSE and improves held-out MSE, so the correctness gate accepts it. Updated the stale "does not reliably reproduce eager gradients" docs; the gate is retained as a generalization safety net. - testconsole/AutoformerVerify: seeded RandomHelper + Environment.Exit(1) on verification failure so a regression fails the process. - Directory.Packages.props: AiDotNet.Tensors + Native 0.111.1 -> 0.112.0 (brings the resident-param + N-BEATS grad-buffer fixes). Real TFT tests (unit + TS model-family) + NBEATSModelTests: 46/46 green. The Generated.* generic-NN tests feed TS models non-time-series data (1-point series) and fail pre-existing regardless of these changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e-gpu # Conflicts: # Directory.Packages.props
Contributor
Commit messages auto-fixedOne or more commit messages did not follow Conventional Commits, so they were rewritten to comply (subject case, header length ≤ 100, valid type). Each commit and its diff were preserved — no squashing. The branch was force-pushed with the corrected messages. If you have local work on this branch, run |
ooples
force-pushed
the
feat/timeseries-tape-gpu
branch
from
2b230c9 to
19a5f63
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TimeSeries deep forecasters: correct autodiff training via GradientTape + IEngine (and a GPU-residency seam)
Several
AiDotNet.TimeSeriesdeep forecasters trained via hand-written scalarNumOpsCPU loops — some with broken/absent backward passes — so they either didn't learn or couldn't dispatch to the GPU engine. This PR re-expresses their forward/training with the automaticGradientTape<T>+Engine.Tensor*(IEngine) path (the same patternNBEATSModelalready uses), so autodiff produces the backward for free and the models become GPU-dispatchable.Included so far
ForwardWithGradientsbuilt an empty gradient dict, soTrainwas a no-op (never learned), and inference used a scalar matmul that disagreed with the tape math by ~3500×. Re-expressed the stack forward withEngine.TensorPermute/TensorMatMul/TensorBroadcastAdd/ReLUunder a tape; inference now uses the same ops. Verified: windowed MSE 287 → 18 (beats the naive baseline).[B,H,S,headDim], LayerNorm, FFN, distilling, generative decoder). ProbSparse simplified to full SDPA (reduces to it at these lengths). Verified: windowed MSE 208 → 0.057 (200× below the repeat-last baseline).CanTrainOnGpu+TryFusedResidentSteponTimeSeriesModelBase(compiled fwd+bwd+Adam plan, weights/activations/Adam moments resident), gated behind a correctness-validation fallback so it never ships worse than the eager path.Still incoming to this same PR (parallel work)
Autoformer, DeepAR, TFT — the identical scalar→tape conversion (branches
feat/{autoformer,deepar,tft}-tape), cherry-picked here as each is verified.Measured GPU reality (honest)
Making a model tape+IEngine'd makes it correct + GPU-dispatchable, but not automatically GPU-fast: NBEATS (big dense GEMMs) hits ~87% peak util in float32, while Informer (tiny
headDim=8attention GEMMs) is host-bound at ~15% peak. Sustained high utilization needs on-device tensor residency (per-op host↔device dispatch dominates otherwise) — that's the follow-on Tensors-engine work the residency seam is scaffolding toward.Public APIs +
*Optionsunchanged. Builds clean (src, net10.0). Each model has atestconsoleverify harness.Summary by CodeRabbit
LastRunUsedGpuResidentPath).