fix: 6 critical time series bugs (#834-#839) + integration tests - #840
Merged
Merged
Conversation
Override Forecast in ARIMAModel to properly handle differencing: - Difference history before extracting lag features - Apply AR/MA prediction on differenced values - Undifference forecasts by cumulative integration - Store last d original values during training for undifferencing Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Override Forecast in SARIMAModel to: - Apply both regular and seasonal differencing before prediction - Extract enough lag values (Math.Max(p, P*m)) from differenced history - Apply both AR and seasonal AR components - Undifference with both regular and seasonal integration Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add trained state fields (level, trend, seasonal factors) - Save end-of-training state by running through training data - Override Forecast to start from trained state instead of resetting - Update state between forecast steps using forecast values Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use Math.Min(LookbackWindow, x.Columns) when extracting input vectors in ComputeBatchLoss, padding remaining values with zero when the input matrix has fewer columns than the lookback window. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…#838) Override TrainCore to construct proper lookback windows from the target vector y instead of using x.GetRow(i), which returns only 1 element for univariate time series. For each sample index i, extract y[i-lookback:i] as input and y[i:i+forecastHorizon] as target. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Override TrainCore to extract lookback windows from y vector instead of using x.GetRow(i) which returns 1 element for univariate data - Replace all Convert.ToDouble() with _numOps.ToDouble() for proper generic type conversion - Reduce default EmbeddingDim from 512 to 64 and Epochs from 100 to 10 - Add early termination when loss converges Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
SARIMA's Forecast should always use its own prediction logic even when d=0 and D=0, because PredictSingle requires Math.Max(p, P*m) lag values but base.Forecast only provides LagOrder (typically 2) values via PrepareForecastFeatures. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The ff1Weight tensor is stored as [ffDim, embDim] but MatrixMultiply expects inner dimensions to match. Transpose ff1W and ff2W before matmul in both encoder and decoder ProcessLayerAutodiff methods. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add integration tests that actually train and forecast (not just construct) to catch the bugs fixed in issues #834-#839: - ARIMA with d=1 differencing produces finite forecasts - SARIMA with seasonal P=1, m=12 does not crash - ExponentialSmoothing does not reset state between forecast steps - NBEATS with single-column input does not index out of range - Autoformer with small data does not crash with rank-1 tensors - Informer training completes in reasonable time Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds/changes forecasting and training flows: Informer and Autoformer training rebuilt to use lookback windows from targets with batched, shuffled sampling; ARIMA/SARIMA/ExponentialSmoothing gain Forecast implementations and undifferencing/state persistence; N-BEATS supports univariate inputs; Informer defaults reduced; integration tests added. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Trainer as Trainer
participant DataPrep as DataPrep
participant Batcher as Batcher
participant Model as Model
participant Optim as Optimizer
Trainer->>DataPrep: compute valid sampleIndices from y (lookback windows)
Trainer->>Trainer: shuffle sampleIndices each epoch
loop each epoch
Trainer->>Batcher: request next batch of indices
Batcher->>DataPrep: build batch inputs/targets from y windows
Batcher->>Model: forward pass -> predictions
Model->>Model: ComputeGradients(batch) -> (gradients, predictions)
Model->>Optim: AccumulateGradients(gradients)
Batcher->>Trainer: report batch loss (from predictions)
alt end of batch
Trainer->>Optim: ApplyGradients()
Trainer->>Optim: ResetGradientAccumulators()
end
end
Trainer->>Model: SaveTrainedState / finalize parameters
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR targets a set of correctness and stability fixes across multiple time-series models (ARIMA/SARIMA/Exponential Smoothing/N-BEATS/Autoformer/Informer), and adds integration tests that exercise train→forecast flows to prevent regressions.
Changes:
- Add/override forecasting/training logic in several models to fix crashes/hangs and handle univariate inputs more robustly.
- Improve Informer hot-loop numeric conversions and reduce default training sizes.
- Add 6 integration tests covering train-and-forecast for the fixed models.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/AiDotNet.Tests/IntegrationTests/TimeSeries/TimeSeriesIntegrationTests.cs | Adds train-and-forecast integration tests for multiple models. |
| src/TimeSeries/ARIMAModel.cs | Adds differencing-aware Forecast and stores tail values for undifferencing. |
| src/TimeSeries/SARIMAModel.cs | Adds seasonal-aware Forecast that applies/undoes differencing. |
| src/TimeSeries/ExponentialSmoothingModel.cs | Persists end-of-training state and overrides Forecast to avoid state resets. |
| src/TimeSeries/NBEATSModel.cs | Prevents IndexOutOfRange in batch-loss input extraction for single-column matrices. |
| src/TimeSeries/AutoformerModel.cs | Builds lookback windows from y and fixes FF matmul dimensions in autodiff path. |
| src/TimeSeries/InformerModel.cs | Extracts training windows from y, adds early stopping, and replaces Convert.ToDouble in hot paths. |
| src/Models/Options/InformerOptions.cs | Reduces default EmbeddingDim and Epochs to speed training. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/TimeSeries/ExponentialSmoothingModel.cs (1)
133-179:⚠️ Potential issue | 🔴 CriticalPersist trained forecast state in serialization/reset (blocking).
Forecast now depends on
_trainedLevel,_trainedTrend, and_trainedSeasonalFactors, but these are not serialized/deserialized or cleared inReset(). A reloaded or reset model will forecast from zeros/stale state. Persist them (or recompute on load) to keep forecasts correct.
As per coding guidelines, incomplete features are blocking issues.💾 Persist trained-state fields
protected override void SerializeCore(BinaryWriter writer) { writer.Write(Convert.ToDouble(_alpha)); writer.Write(Convert.ToDouble(_beta)); writer.Write(Convert.ToDouble(_gamma)); writer.Write(_initialValues.Length); @@ foreach (var value in _initialValues) { writer.Write(Convert.ToDouble(value)); } + + writer.Write(Convert.ToDouble(_trainedLevel)); + writer.Write(Convert.ToDouble(_trainedTrend)); + writer.Write(_trainedSeasonalFactors.Length); + foreach (var value in _trainedSeasonalFactors) + { + writer.Write(Convert.ToDouble(value)); + } } protected override void DeserializeCore(BinaryReader reader) { @@ for (int i = 0; i < initialValuesLength; i++) { _initialValues[i] = NumOps.FromDouble(reader.ReadDouble()); } + + _trainedLevel = NumOps.FromDouble(reader.ReadDouble()); + _trainedTrend = NumOps.FromDouble(reader.ReadDouble()); + int trainedSeasonalLength = reader.ReadInt32(); + _trainedSeasonalFactors = new Vector<T>(trainedSeasonalLength); + for (int i = 0; i < trainedSeasonalLength; i++) + { + _trainedSeasonalFactors[i] = NumOps.FromDouble(reader.ReadDouble()); + } } public override void Reset() { _alpha = NumOps.Zero; _beta = NumOps.Zero; _gamma = NumOps.Zero; _initialValues = Vector<T>.Empty(); + _trainedLevel = NumOps.Zero; + _trainedTrend = NumOps.Zero; + _trainedSeasonalFactors = Vector<T>.Empty(); }src/TimeSeries/ARIMAModel.cs (1)
94-125:⚠️ Potential issue | 🔴 CriticalUse provided history for undifferencing + validate length (blocking).
Forecastignores the suppliedhistoryfor undifferencing and relies on_lastOriginalValuescaptured at training time. This produces incorrect scale when history differs, and a deserialized model (empty_lastOriginalValues) will throw when building tail values. Derive tails fromhistoryand validate length; then remove or serialize_lastOriginalValues.
As per coding guidelines, missing validation of external inputs is a blocking issue.🧮 Use history tail for undifferencing
public override Vector<T> Forecast(Vector<T> history, int steps) { @@ int d = _arimaOptions.D; @@ + if (history.Length <= d) + { + throw new ArgumentException( + $"History length ({history.Length}) is too short for differencing order {d}.", + nameof(history)); + } @@ - var tempTail = new List<T>(); - for (int i = 0; i < _lastOriginalValues.Length; i++) - { - tempTail.Add(_lastOriginalValues[i]); - } + var tempTail = new List<T>(d); + for (int i = history.Length - d; i < history.Length; i++) + { + tempTail.Add(history[i]); + }Also applies to: 392-404, 510-639
🤖 Fix all issues with AI agents
In `@src/TimeSeries/AutoformerModel.cs`:
- Around line 355-364: The code currently returns early when there isn’t enough
data (endIdx <= startIdx); instead, in AutoformerModel replace the silent return
with a thrown exception (e.g., InvalidOperationException) that clearly states
the minimum required series length and the current length. Compute
requiredMinimum = lookback + forecastHorizon + 1 (since y.Length must be >
lookback + forecastHorizon), and throw with a message like “Insufficient data:
need at least {requiredMinimum} observations (lookback {lookback} +
forecastHorizon {forecastHorizon} + 1) but got {y.Length}” so callers can fail
fast and correct configuration/data; update the block using the existing
variables startIdx, endIdx, lookback, forecastHorizon, and y.Length.
In `@src/TimeSeries/ExponentialSmoothingModel.cs`:
- Around line 910-1001: Forecast currently starts from the stored trained state
but never applies the supplied history, and uses history.Length to guess the
seasonal index which misaligns seasons; fix Forecast by first replaying the
provided history values into the model state (level, trend, seasonalFactors)
using the same update equations used during training so the seasonal index and
state reflect the actual recent observations, then proceed with the existing
forecast loop; reference the Forecast method and fields _trainedLevel,
_trainedTrend, _trainedSeasonalFactors, Options.SeasonalPeriod, _alpha, _beta,
_gamma and reuse the same deseasonalization/level/trend/seasonal update logic to
update state for each element in history before computing forecasts.
In `@src/TimeSeries/InformerModel.cs`:
- Around line 218-230: In TrainCore remove the unused local declaration "int
forecastHorizon = _options.ForecastHorizon;" since it's never referenced; keep
the existing lookback variable and logic (validIndices loop) intact and simply
delete that dead variable to avoid confusion and compiler warnings.
In `@src/TimeSeries/SARIMAModel.cs`:
- Around line 699-852: The Forecast method calls ApplyDifferencing and
seasonal/regular undifferencing without validating that history is long enough,
which can cause negative/underflow allocations when history.Length < _m * _D or
< _d; add the same minimum-history guard used in TrainCore before calling
ApplyDifferencing: validate history != null and then check history.Length >=
Math.Max( _d + 1, _m * _D + 1 ) (or the exact condition used in TrainCore) and
throw ArgumentException/InvalidOperation if too short; update Forecast
(reference function name Forecast, call to ApplyDifferencing, and fields _m, _D,
_d) to perform this pre-check so subsequent differencing and vector allocations
are safe.
In
`@tests/AiDotNet.Tests/IntegrationTests/TimeSeries/TimeSeriesIntegrationTests.cs`:
- Around line 532-553: The test InformerModel_Train_CompletesInReasonableTime
currently only checks for exceptions and can hang; run the training call
model.Train(x, y) inside a Task (e.g., Task.Run) and assert it completes within
a fixed timeout (for example 5 seconds) by using Task.Wait(timeout) or
Task.WhenAny and then Assert.True that the task completed in time and
Assert.Null for any exception from the task; update the test method
InformerModel_Train_CompletesInReasonableTime to start the training task, await
or wait with a timeout, fail if the timeout elapses, and propagate/assert any
exception thrown by model.Train so the test both verifies it finishes and
doesn't throw.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+707
to
+852
| { | ||
| if (!IsTrained) | ||
| { | ||
| throw new InvalidOperationException("The model must be trained before forecasting."); | ||
| } | ||
|
|
||
| if (history == null) | ||
| { | ||
| throw new ArgumentNullException(nameof(history), "History cannot be null."); | ||
| } | ||
|
|
||
| if (steps <= 0) | ||
| { | ||
| throw new ArgumentException("Number of forecast steps must be positive.", nameof(steps)); | ||
| } | ||
|
|
||
| // Apply the same differencing as in TrainCore | ||
| Vector<T> diffHistory = ApplyDifferencing(history); | ||
|
|
||
| // Build a working list of differenced values | ||
| int maxLag = Math.Max(_p, _P * _m); | ||
| List<T> extendedDiff = new List<T>(diffHistory.Length + steps); | ||
| for (int i = 0; i < diffHistory.Length; i++) | ||
| { | ||
| extendedDiff.Add(diffHistory[i]); | ||
| } | ||
|
|
||
| // Generate forecasts on the differenced scale | ||
| Vector<T> diffForecasts = new Vector<T>(steps); | ||
| for (int step = 0; step < steps; step++) | ||
| { | ||
| T prediction = _constant; | ||
|
|
||
| // Non-seasonal AR component | ||
| for (int j = 0; j < _p && j < extendedDiff.Count; j++) | ||
| { | ||
| prediction = NumOps.Add(prediction, NumOps.Multiply( | ||
| _arCoefficients[j], extendedDiff[extendedDiff.Count - 1 - j])); | ||
| } | ||
|
|
||
| // Seasonal AR component | ||
| for (int j = 0; j < _P; j++) | ||
| { | ||
| int lagIdx = extendedDiff.Count - (j + 1) * _m; | ||
| if (lagIdx >= 0) | ||
| { | ||
| prediction = NumOps.Add(prediction, NumOps.Multiply( | ||
| _sarCoefficients[j], extendedDiff[lagIdx])); | ||
| } | ||
| } | ||
|
|
||
| // MA/SMA components are assumed zero for future predictions | ||
|
|
||
| diffForecasts[step] = prediction; | ||
| extendedDiff.Add(prediction); | ||
| } | ||
|
|
||
| // Undifference: first undo regular differencing, then seasonal differencing | ||
| // (reverse order of ApplyDifferencing which does seasonal first, then regular) | ||
|
|
||
| // Undo regular (non-seasonal) differencing d times | ||
| var currentForecasts = new List<T>(steps); | ||
| for (int i = 0; i < steps; i++) | ||
| { | ||
| currentForecasts.Add(diffForecasts[i]); | ||
| } | ||
|
|
||
| // We need the tail of history at each intermediate differencing level | ||
| // ApplyDifferencing does: seasonal D times -> then regular d times | ||
| // So to undo: first undo regular d times, then undo seasonal D times | ||
|
|
||
| // Compute the series after seasonal differencing but before regular differencing | ||
| Vector<T> afterSeasonalDiff = history; | ||
| for (int i = 0; i < _D; i++) | ||
| { | ||
| afterSeasonalDiff = SeasonalDifference(afterSeasonalDiff, _m); | ||
| } | ||
|
|
||
| // Undo regular differencing d times | ||
| // Each level needs the last value of the series at that level | ||
| Vector<T> tempSeries = afterSeasonalDiff; | ||
| var regularTailValues = new T[_d]; | ||
| for (int level = 0; level < _d; level++) | ||
| { | ||
| regularTailValues[level] = tempSeries[tempSeries.Length - 1]; | ||
| // Difference for next level | ||
| Vector<T> nextLevel = new Vector<T>(tempSeries.Length - 1); | ||
| for (int i = 1; i < tempSeries.Length; i++) | ||
| { | ||
| nextLevel[i - 1] = NumOps.Subtract(tempSeries[i], tempSeries[i - 1]); | ||
| } | ||
| tempSeries = nextLevel; | ||
| } | ||
|
|
||
| // Undo regular differencing in reverse | ||
| for (int level = _d - 1; level >= 0; level--) | ||
| { | ||
| T lastVal = regularTailValues[level]; | ||
| for (int i = 0; i < currentForecasts.Count; i++) | ||
| { | ||
| T undiff = NumOps.Add(currentForecasts[i], lastVal); | ||
| currentForecasts[i] = undiff; | ||
| lastVal = undiff; | ||
| } | ||
| } | ||
|
|
||
| // Undo seasonal differencing D times | ||
| // For seasonal undifferencing: forecast[i] = diffForecast[i] + value[i - m] | ||
| // We need the last m values from the previous level for each D | ||
| for (int level = 0; level < _D; level++) | ||
| { | ||
| // Compute the series at this seasonal differencing level | ||
| Vector<T> seriesAtLevel = history; | ||
| for (int d2 = 0; d2 < _D - 1 - level; d2++) | ||
| { | ||
| seriesAtLevel = SeasonalDifference(seriesAtLevel, _m); | ||
| } | ||
|
|
||
| // Get the last m values from this series | ||
| var seasonalTail = new List<T>(); | ||
| for (int i = Math.Max(0, seriesAtLevel.Length - _m); i < seriesAtLevel.Length; i++) | ||
| { | ||
| seasonalTail.Add(seriesAtLevel[i]); | ||
| } | ||
|
|
||
| // Undo seasonal differencing: forecast[i] = diffForecast[i] + value[i - m] | ||
| // where value[i-m] comes from either the tail or previous forecasts | ||
| var combined = new List<T>(seasonalTail); | ||
| for (int i = 0; i < currentForecasts.Count; i++) | ||
| { | ||
| int refIdx = combined.Count - _m; | ||
| T refVal = refIdx >= 0 ? combined[refIdx] : NumOps.Zero; | ||
| T undiff = NumOps.Add(currentForecasts[i], refVal); | ||
| currentForecasts[i] = undiff; | ||
| combined.Add(undiff); | ||
| } | ||
| } | ||
|
|
||
| Vector<T> result = new Vector<T>(steps); | ||
| for (int i = 0; i < steps; i++) | ||
| { | ||
| result[i] = currentForecasts[i]; | ||
| } | ||
|
|
||
| return result; | ||
| } |
Check notice
Code scanning / CodeQL
Block with too many statements Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
In general, to fix "block with too many statements" in a method, you factor coherent sub-tasks into separate private methods. The top-level method then orchestrates the flow by calling these helpers, reducing the number of complex statements (loops/conditionals) in its own body while preserving the behavior.
For this Forecast method in src/TimeSeries/SARIMAModel.cs, the best approach is to extract the two main internal phases into private helpers:
-
A helper that generates the forecasts on the differenced scale and returns
diffForecasts:- Inputs:
Vector<T> diffHistory,int steps. - Internal: build
extendedDiff, run the AR and seasonal AR contributions in loops, assume MA/SMA = 0, and fill aVector<T>of differenced forecasts. - Output:
Vector<T> diffForecasts.
- Inputs:
-
A helper that undoes differencing and builds the final result vector:
- Inputs:
Vector<T> diffForecasts,Vector<T> history,int steps. - Internal: copy
diffForecastsinto aList<T>, call existingUndoRegularDifferencingandUndoSeasonalDifferencing, then copy into aVector<T>and return it. - Output:
Vector<T> result.
- Inputs:
The Forecast method will keep its parameter checks and the call to ApplyDifferencing, then delegate to these two helpers. This reduces the number of loops and conditionals in Forecast’s block, satisfying the rule without any behavior change. No new imports or external dependencies are required; only new private methods within the same class are added.
Concretely:
- In
SARIMAModel<T>insrc/TimeSeries/SARIMAModel.cs, above the currentForecastmethod, insert two new private methods:GenerateDifferencedForecastsandBuildFinalForecastsFromDifferenced. - Replace the body of
Forecast(lines 707–790) so that:- It performs the same argument validation as now.
- Calls
ApplyDifferencing(history)as now. - Calls
GenerateDifferencedForecasts(diffHistory, steps)to getdiffForecasts. - Calls
BuildFinalForecastsFromDifferenced(diffForecasts, history, steps)and returns its result.
- Remove the now-duplicated logic (the loops that build
extendedDiff, computediffForecasts, managecurrentForecasts, and copy intoresult) fromForecast, since that logic lives in the helpers.
Suggested changeset
1
src/TimeSeries/SARIMAModel.cs
| @@ -732,6 +732,21 @@ | ||
| // Apply the same differencing as in TrainCore | ||
| Vector<T> diffHistory = ApplyDifferencing(history); | ||
|
|
||
| // Generate forecasts on the differenced scale | ||
| Vector<T> diffForecasts = GenerateDifferencedForecasts(diffHistory, steps); | ||
|
|
||
| // Undo differencing and build the final result | ||
| return BuildFinalForecastsFromDifferenced(diffForecasts, history, steps); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Generates future values on the differenced scale using the trained SARIMA model. | ||
| /// </summary> | ||
| /// <param name="diffHistory">The historical time series values after differencing.</param> | ||
| /// <param name="steps">The number of future steps to forecast.</param> | ||
| /// <returns>A vector of forecasted values on the differenced scale.</returns> | ||
| private Vector<T> GenerateDifferencedForecasts(Vector<T> diffHistory, int steps) | ||
| { | ||
| // Build a working list of differenced values | ||
| List<T> extendedDiff = new List<T>(diffHistory.Length + steps); | ||
| for (int i = 0; i < diffHistory.Length; i++) | ||
| @@ -769,6 +784,18 @@ | ||
| extendedDiff.Add(prediction); | ||
| } | ||
|
|
||
| return diffForecasts; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Undoes differencing and builds the final forecast vector on the original scale. | ||
| /// </summary> | ||
| /// <param name="diffForecasts">Forecasts on the differenced scale.</param> | ||
| /// <param name="history">The original historical time series values.</param> | ||
| /// <param name="steps">The number of future steps to forecast.</param> | ||
| /// <returns>A vector of forecasted values on the original scale.</returns> | ||
| private Vector<T> BuildFinalForecastsFromDifferenced(Vector<T> diffForecasts, Vector<T> history, int steps) | ||
| { | ||
| // Undifference: first undo regular differencing, then seasonal differencing | ||
| // (reverse order of ApplyDifferencing which does seasonal first, then regular) | ||
| var currentForecasts = new List<T>(steps); |
Copilot is powered by AI and may make mistakes. Always verify output.
|
- ARIMAModel: remove unused variable, extract UndifferenceForecasts helper, derive tail values from history parameter instead of _lastOriginalValues - InformerModel: remove unused variable, throw on insufficient data instead of silent return, return prediction from ComputeGradients to avoid redundant forward pass - AutoformerModel: throw on insufficient data, fix off-by-one in sample range calculation - SARIMAModel: remove unused variable, add minimum history length validation - ExponentialSmoothingModel: use EsOptions.UseTrend consistently instead of Options.IncludeTrend, store _trainingLength for correct seasonal index alignment, serialize/deserialize trained state fields - Fix test doc comment that mentioned noise term not present in code Co-Authored-By: Claude Opus 4.6 <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/AutoformerModel.cs (1)
344-406:⚠️ Potential issue | 🟡 MinorFix off‑by‑one in insufficient‑data error message.
The logic allows a single sample wheny.Length == lookback + forecastHorizon, but the message says+ 1. That can mislead users when diagnosing input size issues.🛠️ Suggested fix
- $"Require at least {lookback + forecastHorizon + 1} points, got {y.Length}.", + $"Require at least {lookback + forecastHorizon} points, got {y.Length}.",src/TimeSeries/ExponentialSmoothingModel.cs (1)
701-707:⚠️ Potential issue | 🟠 MajorReset should clear the new trained-state fields.
Leaving trained values after Reset can leak stale state into serialization or later forecasts.🧹 Suggested reset fix
public override void Reset() { _alpha = NumOps.Zero; _beta = NumOps.Zero; _gamma = NumOps.Zero; _initialValues = Vector<T>.Empty(); + _trainedLevel = NumOps.Zero; + _trainedTrend = NumOps.Zero; + _trainedSeasonalFactors = Vector<T>.Empty(); + _trainingLength = 0; }
🤖 Fix all issues with AI agents
In `@src/TimeSeries/ARIMAModel.cs`:
- Around line 510-633: The Forecast method must validate that the provided
history is long enough for differencing: before calling
TimeSeriesHelper<T>.DifferenceSeries and before using _arimaOptions.D, check
that history.Length >= d + 1 (where d = _arimaOptions.D) and throw a clear
ArgumentException/ArgumentNullException with a descriptive message if not; add
this precondition near the start of Forecast (after the null and steps checks)
so UndifferenceForecasts and its tail extraction logic cannot index empty or
too-short history.
Add validation that history.Length >= d + 1 before attempting to difference the series, preventing runtime failures when history is too short for the configured differencing order. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/TimeSeries/ARIMAModel.cs`:
- Around line 94-99: Remove the dead field _lastOriginalValues: delete its
private declaration, remove its initialization where the model is constructed,
and remove the population logic inside TrainCore that assigns values to
_lastOriginalValues; also remove any using references and ensure
UndifferenceForecasts and serialization/deserialization remain unchanged (no
reliance on _lastOriginalValues) and that the project compiles and tests pass
after removing those assignments and the field.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- ARIMAModel: remove dead _lastOriginalValues field (written but never read) - ExponentialSmoothingModel: add backward-compatible deserialization for trained state fields, replay smoothing updates over provided history in Forecast instead of ignoring it - NBEATSModel: construct lookback windows from y values for univariate input instead of silently padding with zeros - Tests: make InformerModel timeout test async, strengthen ExponentialSmoothing forecast assertion with fixed params and monotonicity check Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/TimeSeries/ExponentialSmoothingModel.cs`:
- Around line 670-690: When legacy serialized payloads lack the trained-state
block, do not zero-out forecasting state; instead seed it from the model's
stored initial values so seasonality and baseline are preserved. In the
deserialization branch that currently sets _trainedLevel, _trainedTrend,
_trainedSeasonalFactors, and _trainingLength to zeros (the else after the
reader.BaseStream.Position check), initialize _trainedLevel and _trainedTrend
from the appropriate entries in _initialValues (or computed from them), set
_trainedSeasonalFactors to a Vector<T> populated from the last (or appropriate)
seasonal slice of _initialValues with length matching the model's seasonal
period, and set _trainingLength to _initialValues.Length (or the correct count
used during training) so legacy models continue to forecast with seasonality and
a valid baseline.
- Seed trained state from initial values in ExponentialSmoothingModel legacy deserialization so backward-compat forecasts remain correct - Extract ComputeIntegrationTailValues helper in ARIMAModel to reduce UndifferenceForecasts complexity - Extract UndoRegularDifferencing and UndoSeasonalDifferencing helpers in SARIMAModel to reduce Forecast method complexity Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/TimeSeries/ExponentialSmoothingModel.cs`:
- Around line 669-702: The current deserialization uses
reader.BaseStream.Position < reader.BaseStream.Length to detect presence of
newer trained-state fields which is brittle; change serialization to write an
explicit version marker (e.g., an int version) at the start of the model blob
and update the deserialization to read that version and conditionally read
_trainedLevel, _trainedTrend, _trainedSeasonalFactors and _trainingLength only
when version >= the new version number. Update the corresponding serialize/write
routine to emit the version marker and bump the version when adding fields, and
update the deserialize/read logic (the code block that uses reader and sets
_trainedLevel/_trainedTrend/_trainedSeasonalFactors/_trainingLength and
references Options.SeasonalPeriod and _initialValues) to use a version check
instead of Position < Length.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…r test - Add serialization versioning with backward-compatible format detection - Fix forecast double-replay by initializing from initial values not trained state - Fix seasonal factor update formula to use level consistently with training - Replace informer hang test timeout attribute with explicit cancellation token Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Feb 11, 2026
Closed
Closed
3 tasks done
1 of 5 tasks
This branch was successfully deployed
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.
Summary
Bug Details
Forecastto difference/undifference properlyPrepareForecastFeaturescreates vector too short for seasonal lagsForecastwith seasonal-aware prediction logicPredict()resets level/trend/seasonal from initial valuesForecastoverrideComputeBatchLossindexesx[i,j]beyond column countMath.Min(LookbackWindow, x.Columns)x.GetRow(i)returns 1 element for univariate datayvector insteadConvert.ToDouble()in hot loops + large defaults_numOps.ToDouble(), reduce defaultsTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Configuration
Tests