refactor(1789): Finance + TimeSeries + CausalDiscovery + CausalInference + (17/18) - #1982
Merged
Merged
Conversation
…nce + A (17/18) Slice 17 of 18 splitting #1789 into review-sized PRs; CodeRabbit caps reviews at 100 changed files and rate-limits, so slices are packed close to the cap. Scope: Finance + TimeSeries + CausalDiscovery + CausalInference + AnomalyDetection + SurvivalAnalysis + ReinforcementLearning + MetaLearning Targets integration/1789; see #1966 for the split rationale. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Warning Review limit reached
Next review available in: 40 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 Plus Run ID: 📒 Files selected for processing (100)
WalkthroughThis pull request revises anomaly scoring, causal discovery, portfolio and trading models, reinforcement-learning agents, model persistence, meta-learning utilities, and time-series forecasting. It also corrects research metadata and updates tracing and optimizer paths. ChangesAnomaly detection and causal discovery
Finance and trading
Model state and agents
Time series and utilities
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
split/1789-17-finance-timeseries
branch
from
August 7, 2026 13:21
7c55bac to
8740b99
Compare
ooples
marked this pull request as ready for review
August 8, 2026 13:02
![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: 79
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/TimeSeries/NLinearModel.cs (2)
269-270: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
CreateInstancediscards the user-supplied optimizer.The constructor now accepts an
IGradientBasedOptimizer<T, Matrix<T>, Vector<T>>.CreateInstancedoes not forward_optimizer. Every clone therefore reverts to the default Adam optimizer. A user who injects a configured optimizer loses that configuration onClone()and on any base-class path that rebuilds the model. Forward the field.♻️ Proposed fix
protected override IFullModel<T, Matrix<T>, Vector<T>> CreateInstance() - => new NLinearModel<T>(new NLinearOptions<T>(_options)); + => new NLinearModel<T>(new NLinearOptions<T>(_options), _optimizer);🤖 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/NLinearModel.cs` around lines 269 - 270, Update NLinearModel<T>.CreateInstance to pass the existing _optimizer into the reconstructed NLinearOptions<T>, preserving the injected optimizer for Clone() and other model-rebuild paths.
230-251: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSerialization drops the injected optimizer, and the reader has no payload guard.
The scaler round-trip itself is correct. Two follow-ups:
DeserializeCorereads four extra doubles unconditionally.TiDEModel.DeserializeCoreguards the same addition with a stream-position check. Payloads written before this change fail here. Decide whether older payloads must load, and apply the same guard if they must.- Line 244 discards the persisted
_l. The subsequent loop uses the constructor-derived_l. If the two differ, the reader consumes the wrong number of doubles and the scaler values are garbage. Validate the persisted length instead of discarding it.🛡️ Proposed fix for the length validation
protected override void DeserializeCore(BinaryReader reader) { - reader.ReadInt32(); + int storedL = reader.ReadInt32(); + if (storedL != _l) + { + throw new InvalidOperationException( + $"Serialized lookback window ({storedL}) does not match the configured window ({_l})."); + } for (int j = 0; j < _l; j++) { _w[j] = reader.ReadDouble(); }🤖 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/NLinearModel.cs` around lines 230 - 251, Update NLinearModel.DeserializeCore to capture the persisted length from the stream and validate it matches the constructor-derived _l before reading weights; do not discard it. Preserve compatibility with payloads written before scaler statistics were serialized by guarding the four additional reads with the stream-position check used by TiDEModel.DeserializeCore, leaving existing defaults when the payload has no remaining data.src/TimeSeries/ProphetModel.cs (1)
984-1012: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRemove the dead
statesmatrix. It is allocated, written once, and discarded.Line 985 allocates an
n × GetStateSize()matrix. Line 1012 writes a single row into it. No code readsstatesafterwards, and the method does not return it. The allocation is wasted, and it scales with the training set size.The comment "Store final state for future reference" no longer matches the code.
SyncModelParametersFromStateat Line 987 already publishes the state to the base class. This looks like a leftover from the per-timestep fitting loop thatFitLeastSquaresreplaced.Note the ordering problem the removal also fixes: Line 987 syncs the state before
OptimizeParametersruns at Line 994, and Line 1012 captures the state after. Only the discarded matrix records the post-optimization snapshot. Confirm thatApplyParameters, whichOptimizeParameterscalls at Line 515, already propagates the optimized values to the base class through its ownbase.ApplyParameters(parameters)call at Line 1127.🛠️ Proposed fix
FitLeastSquares(x, y); - int n = y.Length; - Matrix<T> states = new Matrix<T>(n, GetStateSize()); - SyncModelParametersFromState(); IsOptimized = false;} - // Store final state for future reference - states.SetRow(n - 1, GetCurrentState()); - // Compute residual statistics if anomaly detection or prediction intervals are enabledBased on the path instruction that flags "dead code" and "unused variables/parameters that suggest incomplete refactoring" as blocking.
🤖 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/ProphetModel.cs` around lines 984 - 1012, Remove the unused states matrix allocation and its final SetRow call from the fitting method. Keep SyncModelParametersFromState before optimization and rely on OptimizeParameters, via ApplyParameters, to propagate optimized values through the base class; remove the now-inaccurate “Store final state” comment as well.Source: Path instructions
src/CausalDiscovery/Bayesian/DiBSAlgorithm.cs (1)
239-266: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftThe unbounded
nscale factor makes edge selection sample-size dependent.
dataScale = nmultiplies a per-edge term that is already bounded by the squared correlation, while the sparsity and acyclicity priors stay O(1). At n = 200 the balance works. At n = 50,000 a correlation of 0.02 producesdataGrad = 50000 * 0.0004 = 20, which dominates the priors (magnitude ~3) and pushes every logit positive. The result is a near-complete graph for large datasets, which is the opposite failure of the one this change fixes.Bound the effective scale, or scale the priors by the same factor so the likelihood-vs-prior ratio stays fixed.
🛡️ Proposed fix: scale the priors together with the data term
- T dataScale = NumOps.FromDouble(n); + // Scale BOTH the data term and the O(1) priors by n so the balance is + // independent of the sample count. + T dataScale = NumOps.FromDouble(n); + T priorScale = NumOps.FromDouble(Math.Sqrt(n));Then multiply
sparsityGradandacycGradbypriorScalebefore the sum on Line 280.🤖 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/CausalDiscovery/Bayesian/DiBSAlgorithm.cs` around lines 239 - 266, Bound the sample-size scaling in the gradient calculation so edge selection remains stable across dataset sizes. Update the dataScale/prior handling in the surrounding gradient loop, and apply the same priorScale to sparsityGrad and acycGrad before they are combined with dataGrad, preserving a fixed likelihood-to-prior ratio.
🤖 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/AnomalyDetection/DistanceBased/COFDetector.cs`:
- Around line 96-101: Update ScoreAnomalies XML documentation to state that
in-sample scoring requires passing the same training-matrix instance;
value-identical copies use the query path and may produce different results. In
ScoreAnomalies, resolve _trainingDistanceMatrix once before selecting the
distance matrix, then reuse that validated value for the trainingDistanceMatrix
reference instead of duplicating the null guard.
In `@src/AnomalyDetection/Linear/KernelPCADetector.cs`:
- Around line 378-399: Update KernelPCA_OutlierGetsHighestScore so the anomalous
point is not included in the fitted training data: use a held-out or externally
defined new anomaly for scoring, and retain the assertion that it receives the
highest score. Alternatively remove the test rather than asserting the
documented in-sample behavior. Also update the ResearchPaper attribute for KPCA
novelty scoring to cite Hoffmann 2007 in addition to Scholkopf 1998.
In `@src/CausalDiscovery/Bayesian/DiBSAlgorithm.cs`:
- Around line 203-233: Move StandardizeColumns from DiBSAlgorithm and
MMHCAlgorithm into the shared CausalDiscoveryBase<T>, preserving its current
behavior and signature as appropriate for base-class access. Remove both
per-algorithm copies and update callers to use the inherited implementation;
also consolidate the near-identical ContinuousOptimizationBase.StandardizeData
through the shared routine where compatible.
In `@src/CausalDiscovery/ContinuousOptimization/ContinuousOptimizationBase.cs`:
- Around line 279-303: Confirm the supported maximum variable count for
continuous-optimization discovery and assess whether HasDirectedPath, invoked by
EnforceAcyclic for each candidate edge, is acceptable at that scale. If the
supported graph size makes the O(d⁴) worst-case cost impractical, replace the
repeated DFS reachability checks with an incremental reachability matrix or
another approach that removes the quartic term; otherwise document or enforce
the intended size limit.
In `@src/CausalDiscovery/ContinuousOptimization/DAGMALinear.cs`:
- Around line 140-143: Replace the mutable configured-rate field with an
immutable _configuredLearningRate value, and initialize a local learningRate
from it at the start of DiscoverStructureCore. Update SolveInnerProblem and its
callers to accept and mutate that per-run value by ref, preserving decay across
outer steps while resetting it for each DiscoverStructure invocation.
- Around line 165-168: Replace the use of _configuredMaxIterations in the
DAGMALinear inner-step calculation with a dedicated DAGMA inner-iteration
configuration, and apply that setting when capping defaultInner. Preserve
MaxIterations exclusively for outer iterations, or explicitly document the
deviation if no separate configuration is introduced.
In `@src/CausalDiscovery/ContinuousOptimization/DAGMANonlinear.cs`:
- Around line 181-231: Remove the local ProjectToDag and ContainsDirectedCycle
implementations and update the nonlinear optimization flow to call the inherited
ContinuousOptimizationBase.EnforceAcyclic instead. Delete the now-unused
EDGE_TOLERANCE constant, preserving the existing thresholding and
DAG-enforcement behavior through the shared deterministic implementation.
In `@src/CausalDiscovery/ContinuousOptimization/MCSLAlgorithm.cs`:
- Around line 144-179: Update the gradient calculation in the MCSL optimization
loop around Math.Sign to use the same NaN-safe sign guard as NOTEARSLowRank,
preventing Math.Sign from receiving a NaN effective edge value. Preserve the
existing sparsity-gradient behavior for finite values while ensuring non-finite
weights or masks cannot terminate the optimization with ArithmeticException.
In `@src/CausalDiscovery/ContinuousOptimization/NOTEARSLinear.cs`:
- Around line 336-350: Track whether the Armijo line search in the inner
optimization loop succeeds, including the logic around the fallback after the 20
failed trials at the line-search block. Apply the maxStep convergence check near
the existing maxStep calculation only when an Armijo step was accepted; never
terminate based on the fallback step size, while preserving normal iteration
behavior otherwise.
In `@src/CausalDiscovery/ContinuousOptimization/NOTEARSLowRank.cs`:
- Around line 286-289: Replace the magic value in the convergence gate
surrounding outerIter and hVal with a named constant representing the minimum
dual-ascent updates, and attach the reference citation to that constant. Use the
constant in the outerIter comparison while preserving the existing tolerance
behavior, including allowing the check when MaxIterations is below the minimum.
In `@src/CausalDiscovery/DeepLearning/DECIAlgorithm.cs`:
- Around line 299-320: Update candidate construction around the correlation
calculation in the DECI projection to orient each reciprocal pair from the
higher-variance variable to the lower-variance variable, using cov[i, i] and
cov[j, j] with a deterministic tie rule. Then update candidates.Sort to use a
total ordering, retaining descending Strength and adding deterministic From/To
tie-breakers so equal-strength candidates always produce the same orientation
and greedy selection order.
- Around line 282-293: Update the fallback edge-selection logic in
DECIAlgorithm<T> to compare the absolute weight against the inherited
EdgeThreshold instead of the hardcoded 0.1 value. Ensure the posterior edge gate
and fallback path consistently use the configurable threshold from
CausalDiscoveryOptions.
In `@src/CausalDiscovery/Functional/ICALiNGAMAlgorithm.cs`:
- Around line 80-83: Add an early boundary check at the start of
DiscoverStructureCore, before CenterData and WhitenData, returning an empty
adjacency matrix when n == 0 or d < 2. Match the sibling algorithm behavior and
prevent degenerate dimensions from reaching the decomposition and FastICA steps.
- Around line 357-364: Remove the unreachable natural-order fallback branch from
the loop around TryFindCausalOrder in ICALiNGAMAlgorithm. Keep the existing
order-return behavior and pruning progression unchanged, since
TryFindCausalOrder succeeds after all entries are pruned.
In `@src/CausalDiscovery/Functional/PNLAlgorithm.cs`:
- Around line 103-109: Update the PNL edge-assignment logic in the nearZeroDeps
branch and the unresolved-direction fallback of the PNLAlgorithm loop so
unidentified pairs retain adjacency without asserting a direction: represent
them using the framework’s unoriented-edge convention, such as symmetric
entries, rather than assigning only W[i, j]. Preserve the directed assignment
only when residual asymmetry resolves the direction, and verify the convention
expected by FunctionalBase<T>/CausalGraph if necessary.
In `@src/CausalDiscovery/Functional/RCDAlgorithm.cs`:
- Around line 159-175: Extract the shared wrong-way and total evidence
calculation into a helper near ConfoundingRatio, returning both accumulators
while iterating remaining and calling DiffMutualInfo once per pair. Update the
candidate-selection loop to use the helper’s EffectEvidence, and update
ConfoundingRatio to compute its ratio from both returned values, preserving the
existing zero-denominator behavior.
- Around line 267-288: Move the first XML documentation block describing
DirectLiNGAM’s entropy-based mutual-information difference from above
ConfoundingRatio to directly above the DiffMutualInfo method declaration. Leave
the ConfoundingRatio summary as its sole documentation block and preserve both
descriptions unchanged.
- Around line 189-207: Add focused RCD calibration tests covering the
first-round stop when ConfoundingRatio is NaN or exceeds
_confoundingScoreCutoff, including verification that no directed coefficients
are assigned among remaining variables. Alternatively, revise the
ConfoundingEvidenceCutoff documentation so it only describes behavior covered by
existing tests, while preserving the current RCDAlgorithm stopping behavior.
In `@src/CausalDiscovery/Functional/VARLiNGAMAlgorithm.cs`:
- Around line 102-110: Use an unthresholded matrix of original signed
DirectLiNGAM coefficients when computing the structural-weight adjustment in
VARLiNGAMAlgorithm, rather than B0Graph.AdjacencyMatrix if it contains
thresholded values. Preserve those coefficients through DirectLiNGAMAlgorithm’s
result, apply the intermediary subtraction with them, and threshold only the
final lagged coefficient result.
In `@src/CausalDiscovery/Hybrid/MMHCAlgorithm.cs`:
- Around line 80-112: Move the shared column-standardization logic into a
protected StandardizeColumns(Matrix<T> data) method on CausalDiscoveryBase<T>,
preserving the mean, population standard deviation, 1e-12 guard, and
zero-centering behavior. In src/CausalDiscovery/Hybrid/MMHCAlgorithm.cs lines
80-112, delete the private duplicate and have DiscoverStructureCore use the
inherited helper. In src/CausalDiscovery/TimeSeries/NeuralGrangerAlgorithm.cs
lines 203-232, delete StandardizeSeries, replace its call with the base helper,
and remove the redundant sampleCount and variableCount arguments.
In `@src/CausalDiscovery/ScoreBased/BOSSAlgorithm.cs`:
- Line 47: Update the Year value in the ResearchPaper attribute applied to
BOSSAlgorithm from 2022 to 2023, leaving the URL, title, and authors unchanged.
In `@src/CausalDiscovery/TimeSeries/CCMAlgorithm.cs`:
- Around line 58-72: Add the public DirectionalityAsymmetryThreshold property to
CausalDiscoveryOptions, documenting its purpose and 0.2 default value. Ensure
CCMAlgorithm can read this property through
options?.DirectionalityAsymmetryThreshold without changing its existing
validation or fallback behavior.
In `@src/CausalDiscovery/TimeSeries/LPCMCIAlgorithm.cs`:
- Line 298: In LPCMCIAlgorithm.OLSResiduals, change the
MatrixSolutionHelper.SolveLinearSystem<T> decomposition selection for the
ridge-stabilized ZtZ/Zty solve from MatrixDecompositionType.Lu to
MatrixDecompositionType.Cholesky, preserving the existing operands and result
handling.
- Around line 272-296: Update OLSResiduals to center every column of Z and the
target vector y before accumulating ZtZ and Zty, using centered values
consistently throughout the normal-equation calculations and residual
prediction. Preserve the existing ridge regularization and residual output
behavior while ensuring the regression includes the equivalent intercept
handling.
- Around line 103-122: Update the conditioning-set construction in the
parent-removal loop to order `others` deterministically by descending absolute
unconditional association strength before selecting `condSize` candidates. Reuse
the existing association/correlation computation for each candidate where
available, then build `condSet` from that ordered sequence instead of relying on
`HashSet` enumeration, while preserving the existing PC1 removal and termination
behavior.
In `@src/CausalDiscovery/TimeSeries/NeuralGrangerAlgorithm.cs`:
- Around line 135-145: Move the hidden buffer allocation from the per-sample
path containing the forward-pass loop to a single allocation before the epoch
loop. Reuse that buffer by overwriting every hidden-unit value for each sample,
preserving the existing computation in the forward pass.
In `@src/CausalDiscovery/TimeSeries/TSFCIAlgorithm.cs`:
- Around line 109-119: Update the Phase 1 skeleton computation to store the
maximizing lag alongside each skeleton result, rather than discarding the
argmax. Reuse that stored lag for both the conditional-independence test
currently using bestLag and the edge-weight calculation currently using
bestLagItoJ, removing their duplicate forward-direction loops; retain the
separate bestJtoI loop for the reverse direction.
In `@src/CausalInference/DoublyRobustEstimator.cs`:
- Around line 811-822: Update DoublyRobustEstimator<T>.DeepCopy() to clone
FeatureNames into the copied estimator as an independent array, alongside the
existing coefficient and fitted-state assignments. Preserve null behavior and
avoid sharing the original metadata array.
- Line 794: Validate the restored parameter vector in the parameter-loading
method before setting IsFitted: reject empty lengths and lengths not divisible
by three, then derive each outcome-vector size using parameters.Length / 3. Only
set IsFitted after successful validation and population of all three vectors.
In `@src/CausalInference/TLearner.cs`:
- Around line 417-443: Update LoadAdditionalModelData in TLearner to validate
fitted model data when IsFitted is true: require both WeightsTreated and
WeightsControl arrays to be present and ensure each length equals NumFeatures
before assigning the vectors. Reject missing or mismatched arrays with an
appropriate validation exception instead of leaving zero-length vectors;
preserve the existing loading behavior for valid data and non-fitted state.
In `@src/Finance/Portfolio/AssetGraphBuilder.cs`:
- Around line 289-292: In the ordering logic around the local order array,
replace the nullable Clone cast and fallback with an explicit double[] clone of
strength before calling Array.Sort. Keep sorting the cloned keys and reversing
order unchanged, ensuring strength itself is never used as the sort-key array.
- Around line 206-237: Update DistanceCorrelationMatrix to precompute and cache
each column’s DoubleCentredDistances result once, then use a shared
DistanceCorrelationFromCentred helper for every pair. Refactor
DistanceCorrelation to delegate to that helper so the accumulation and
normalization remain implemented in one place, while preserving the existing
matrix values and symmetry.
- Around line 321-355: Update FilterTmfg’s input validation to reject non-finite
dependency values with a clear argument error, and add a defensive check before
indexing faces when no best face is selected. Rework the greedy selection to
maintain each remaining node’s best-face and gain, refreshing only records
affected by the split; replace remaining.Remove with index-tracked removal to
avoid linear scans while preserving the existing graph construction.
In `@src/Finance/Portfolio/CVaRPortfolioObjective.cs`:
- Around line 255-262: Update the validation in TransactionCost to reject every
non-finite basisPoints value, including positive infinity, while retaining the
negative-value check. Use double.IsFinite where supported, or add a private
IsFinite helper using NaN and infinity checks if required by the target
framework.
- Around line 132-150: Update ConditionalValueAtRisk to use the dual-consistent
empirical CVaR: average the fully included worst losses and weight the boundary
loss by the fractional tail share when (1 - _alpha) * losses.Length is
non-integer, rather than rounding tailCount up. Preserve null and empty-input
validation, then revise the ConditionalValueAtRisk remarks to describe the
interpolated tail and its agreement with DualObjective.
In `@src/Finance/Portfolio/GraphAttentionLayerCore.cs`:
- Around line 202-217: Update ConcatenateHeads to validate headOutputs[0] has
rank 2 before reading Shape[0] or Shape[1], throwing the same ArgumentException
used for invalid subsequent heads. Preserve the existing node/feature
consistency checks for heads starting at index 1.
- Around line 122-151: Update the neighborhood processing in
GraphAttentionLayerCore to compute each pair’s adjacency value and LeakyReLU
score once during the first pass, caching the score for connected neighbors.
Reuse the cached scores in the exponential pass while preserving isolated-node
handling and the existing normalization behavior.
In `@src/Finance/Portfolio/GraphAttentionPortfolio.cs`:
- Around line 198-210: The OptimizePortfolio overrides accept prediction vectors
shorter than the asset universe, causing misaligned allocations. In
src/Finance/Portfolio/GraphAttentionPortfolio.cs:198-210, after the existing
truncation in GraphAttentionPortfolio.OptimizePortfolio, throw
InvalidOperationException when scores.Length is less than _options.NumAssets,
including both counts in the message; apply the same validation in
src/Finance/Portfolio/SignatureInformedTransformer.cs:173-186 before
Objective.Weights(scores, _options.Temperature).
- Around line 180-186: Update BuildAdjacency to require a rank-2 returnPanel
before calling BuildGraph, reject unsupported ranks consistently, and derive
assets directly from returnPanel.Shape[1] instead of using the
_options.NumAssets fallback.
- Around line 62-67: Define the missing GraphAttentionPortfolioOptions<T> type
used by GraphAttentionPortfolio, including the expected namespace and
configuration required by its constructor, field, and GetOptions override. Add
LayerHelper<T>.CreateDefaultGraphAttentionPortfolioLayers(...) with the layer
construction expected by the options, or remove and replace those references
consistently so GraphAttentionPortfolio compiles.
- Around line 249-267: Preserve the resolved architecture and loss function when
cloning in GraphAttentionPortfolio<T>.CreateNewInstance and
SignatureInformedTransformer<T>.CreateNewInstance. Pass the inherited
Architecture and LossFunction, or stored resolved equivalents, to each model’s
multi-argument constructor so clones retain custom layers and loss
configuration; update both listed sites:
src/Finance/Portfolio/GraphAttentionPortfolio.cs:249-267 and
src/Finance/Portfolio/SignatureInformedTransformer.cs:252-276.
- Around line 235-246: Update UpdateParameters in both
src/Finance/Portfolio/GraphAttentionPortfolio.cs (lines 235-246) and
src/Finance/Portfolio/SignatureInformedTransformer.cs (lines 238-249) to compute
the total expected parameter count from each layer’s ILayer.ParameterCount and
validate it equals parameters.Length before entering the mutation loop. Reject
mismatched vectors before any layer receives SetParameters, while preserving the
existing sequential slicing for valid input.
In `@src/Finance/Portfolio/PathSignatureTransform.cs`:
- Around line 121-142: In the step-processing loop of the path transform,
precompute each coordinate’s increment once per step into a reusable vector,
then use those values for both total/running updates and the level-2
accumulation instead of recalculating dj inside the nested i/j loops. Preserve
the existing second-order formula and accumulation order while reducing
NumOps.ToDouble calls to one pair per coordinate per step.
In `@src/Finance/Portfolio/SharpeRatioPortfolioObjective.cs`:
- Around line 95-106: Update SharpeRatioPortfolioObjective.Loss so non-positive
mean returns receive a large finite penalty before applying the volatility term,
ensuring every losing portfolio ranks worse than profitable portfolios. Keep the
existing positive-mean logarithmic calculation and volatility flooring for valid
cases, and choose or validate the penalty against the tests covering Loss and
SharpeRatio ordering.
In `@src/Finance/Portfolio/SignatureAugmentedAttention.cs`:
- Around line 194-207: Update ScaledDotProductLogits to reject non-positive key
dimensions after validating the query/key dimension match and before computing
scale. Add an explicit dk <= 0 guard that throws an appropriate argument
exception, while preserving valid positive-dimension behavior.
- Around line 52-61: Update the GammaLogit setter to reject all non-finite
values, including positive and negative infinity, while preserving the existing
NaN validation behavior and assignment for finite inputs. Use the existing
double.IsNaN validation in GammaLogit as the change point.
In `@src/Finance/Portfolio/SignatureInformedTransformer.cs`:
- Around line 116-139: Remove the redundant options.Validate() call from
CreateDefaultArchitecture, while retaining validation in ResolveArchitecture
before architecture resolution. Keep the existing Guard.NotNull checks and
default architecture construction unchanged so both portfolio models validate
options exactly once.
- Around line 62-67: Add the public SignatureInformedTransformerOptions<T> class
in the finance portfolio options namespace, deriving from the appropriate
model-options base and defining the members consumed by
SignatureInformedTransformer<T>, including SignatureLevel, CVaRAlpha,
LookbackWindow, NumAssets, NumHeads, ModelDimension, and DropoutRate. Ensure its
shape supports the transformer’s constructors, GetOptions(), validation, copy
construction, and layer setup.
In `@src/Finance/Trading/Factors/FactorVAE.cs`:
- Around line 775-778: Update SerializeNetworkSpecificData and
DeserializeNetworkSpecificData to persist and restore KlWeight, Seed, and
UseAMSGrad alongside the existing scalar fields, matching the values copied by
CreateNewInstance. Preserve the serialization order and ensure deserialization
reconstructs equivalent options for reloaded models.
- Around line 628-665: Update DecodeReturns and AlignReturns to reject
unsupported feature ranks rather than collapsing 3D-or-higher tensors into a
batch dimension. Validate that features/alpha use only the supported unbatched
or rank-2 batched shapes, and throw a clear argument or invalid-operation
exception for other ranks before reshaping; preserve existing rank-1 and rank-2
behavior.
- Around line 386-398: Update PredictCore’s custom native span path to switch
the model into inference mode before calling RunSpan, and preserve/restore the
prior training mode afterward. Ensure this applies to the
FeatureSpanStart/PriorSpanStart prediction flow without changing the ONNX or
PredictNative branches.
In `@src/Finance/Trading/Factors/Stockformer.cs`:
- Around line 246-265: Connect StockformerMultiTaskLoss to the differentiable
training path used by PredictCore instead of only calling it from ComputeLoss,
whose double-valued result severs gradients. Ensure return, low-frequency,
direction, and classification terms all contribute to the training objective;
otherwise update the class XML documentation to explicitly state that only the
return head is trained.
- Around line 525-532: Remove the stale duplicate XML summary above Lift and
retain a single summary describing projection of the D input factors to model
width through the lift layer.
- Around line 666-690: Validate that the input time dimension equals
_options.SequenceLength in ForwardCore before invoking PredictBands/Restore, and
reject mismatched windows consistently. This preserves Restore’s indexing
against the fixed output width of _lowUpsample and _highUpsample without
changing its tensor layout logic.
- Around line 451-459: Update the Encoder property to ensure _encoder is
initialized independently of Layers.Count, so restored or deserialized layers
cannot leave it null. Either make InitializeLayers build the encoder whenever
_encoder is missing, or have Encoder throw a clear InvalidOperationException if
initialization cannot produce one; preserve the existing non-null return
contract.
In `@src/Finance/Trading/Factors/StockformerBands.cs`:
- Around line 78-113: Update Split so its returned High band matches the
documented finest-detail behavior: preserve the detail produced by the first
_wavelet.Decompose call and do not overwrite it on later levels, while
continuing to use each approximation for subsequent decomposition. Keep the
existing single-level behavior and validation unchanged.
In `@src/Finance/Trading/Factors/StockformerDualEncoder.cs`:
- Around line 257-297: Cache the [time, time] causal-window operator used by
CausalWindow, keyed by time, and reuse it across calls instead of rebuilding and
filling it each time. In src/Finance/Trading/Factors/StockformerDualEncoder.cs
lines 257-297, update CausalWindow while preserving its existing matmul
behavior; in src/Finance/Trading/Factors/StockformerAttention.cs lines 122-131,
likewise cache and reuse the [positions, positions] causal mask created by
CausalBias, keyed by positions.
- Around line 257-297: Replace the one-hot matmul implementations in AssetSlice
and TimeSlice with the existing tape-connected slice operation used by the
encoder’s asset/time split, preserving their current output shapes and indexing
behavior. Update OverTime and OverAssets to use that operation without
traversing full rows or columns for each slice. Modify CausalWindow to cache its
causal [time, time] operator per time value and reuse the cached tensor on
subsequent calls instead of rebuilding it.
- Around line 179-185: In the shape-validation block, validate that high has
rank 3 before the loop indexes high.Shape[d], alongside the existing low rank
check. Throw the intended ArgumentException with high as the parameter and
retain the existing shape-equality checks for valid ranks.
In `@src/Finance/Trading/Factors/StockformerMultiTaskLoss.cs`:
- Around line 6-35: The StockformerMultiTaskLoss documentation contradicts the
taskLossWeight parameter and omits its parameter documentation. Update the
summary and remarks to describe the weighted task-loss behavior supported by the
implementation, remove claims that weighting was rejected or invented, and add a
<param> entry for taskLossWeight alongside the other constructor parameters,
matching the Eq. 12 weighting described by the existing comment.
- Around line 63-79: Update the loss calculation loop in
StockformerMultiTaskLoss to stop skipping NaN errors: only missing labels should
be excluded by the existing missingSentinel check. Count every non-missing label
as valid and let NaN prediction errors propagate into the accumulated loss
instead of returning zero when predictions diverge; preserve the valid == 0
fallback for entries masked out solely because their labels are missing.
In `@src/MetaLearning/Algorithms/FeatureWiseTransformation.cs`:
- Around line 62-77: Validate initialScale and initialBias in
FeatureWiseTransformation’s constructor, rejecting NaN and infinite values
before initializing hyperparameters. In the replacement-vector logic around
Apply or its setter, require both scale and bias vectors to match the configured
feature dimension before mutating state, and reject mismatches explicitly.
- Line 39: Change the visibility of the FeatureWiseTransformation<T> class from
public to internal so this implementation helper is not exposed through the
public API. Keep its existing behavior and generic shape unchanged.
In `@src/ReinforcementLearning/Agents/DoubleQLearningAgent.cs`:
- Around line 313-315: Add a ValidatePairedQTables method and invoke it
immediately after Deserialize restores QTable1 and QTable2. Validate identical
state keys and ensure both tables contain every configured action key for each
state; throw InvalidOperationException with descriptive messages when validation
fails, before GetParameters can access the tables.
In `@src/ReinforcementLearning/Agents/DynaQAgent.cs`:
- Around line 234-250: Use one validated, deterministic ordered Q-table entry
contract for parameter export and restore. In
src/ReinforcementLearning/Agents/DynaQAgent.cs#L234-L250, make SetParameters
consume the same ordered entries GetParameters exports and require an exact
vector length; in
src/ReinforcementLearning/Agents/PrioritizedSweepingAgent.cs#L304-L308, avoid
writing every configured action unless restored states are validated to contain
exactly actions 0..ActionSize - 1; in
src/ReinforcementLearning/Agents/QLambdaAgent.cs#L308-L308, reject non-empty
vectors for empty tables and restore only the validated ordered entries; in
src/ReinforcementLearning/Agents/WatkinsQLambdaAgent.cs#L228-L232, add exact
parameter-length validation and use the identical export/restore order.
In `@src/ReinforcementLearning/Agents/DynaQPlusAgent.cs`:
- Around line 209-219: Use one deterministic enumeration of existing (stateKey,
actionKey) entries for the sparse Q-table parameter contract: update
DynaQPlusAgent’s ParameterCount and GetParameters, plus restoration in
src/ReinforcementLearning/Agents/MonteCarloExploringStartsAgent.cs lines
279-294, src/ReinforcementLearning/Agents/OffPolicyMonteCarloAgent.cs lines
303-318, src/ReinforcementLearning/Agents/OnPolicyMonteCarloAgent.cs lines
289-304, and src/ReinforcementLearning/Agents/SARSALambdaAgent.cs lines 197-218,
to iterate only those entries in the same order. Validate that parameters.Length
matches the enumerated count before restoring values, without iterating
ActionSize or creating missing entries.
In `@src/ReinforcementLearning/Agents/TabularActorCriticAgent.cs`:
- Around line 183-192: Update GetParameters so it constructs the vector solely
from the actual value-table and policy-table entries, without initializing or
forcing a synthetic count of one. Ensure both empty tables produce an empty
parameter vector, while preserving all existing entries for non-empty tables;
keep ParameterCount deriving from GetParameters().Length.
In `@src/ReinforcementLearning/Policies/BetaPolicy.cs`:
- Around line 38-44: Update the Authors metadata in the ResearchPaper attribute
for BetaPolicy to use the full names Pei-Wen Chou, Daniel Maturana, and
Sebastian Scherer instead of abbreviated names; leave the title, URL, and year
unchanged.
In `@src/SurvivalAnalysis/NelsonAalenEstimator.cs`:
- Around line 308-311: Update the clone assignment block in NelsonAalenEstimator
so TrainedEventTimes, _cumulativeHazard, _variance, and BaselineSurvivalFunction
receive independent deep-copied vectors rather than references to the source
estimator’s vectors. Preserve the existing values while ensuring subsequent
mutations of either estimator cannot affect the other.
In `@src/TimeSeries/AnomalyDetection/DeepANT.cs`:
- Line 646: The listed ForwardTraced overrides do not match the LayerBase<T>
contract and prevent compilation. In
src/TimeSeries/AnomalyDetection/DeepANT.cs:646-646,
src/TimeSeries/AnomalyDetection/LSTMVAE.cs:446-446 and 692-692,
src/TimeSeries/AutoformerModel.cs:1144-1144 and 1325-1325,
src/TimeSeries/ChronosFoundationModel.cs:1171-1171,
src/TimeSeries/DeepARDistributionHeads.cs:211-211, 303-303, and 387-387,
src/TimeSeries/DeepARModel.cs:934-934,
src/TimeSeries/InformerModel.cs:1144-1144, 1299-1299, and 1440-1440,
src/TimeSeries/NBEATSBlock.cs:350-350, and
src/TimeSeries/NHiTSModel.cs:1039-1039, change each implementation to public
override Tensor<T> Forward(Tensor<T> input), or first add the exact matching
protected ForwardTraced hook to LayerBase<T> and ensure every override targets
that contract.
In `@src/TimeSeries/NLinearModel.cs`:
- Around line 94-101: Align non-finite-value imputation between training and
prediction by updating the training window construction near TrainCore to clamp
raw inputs to 0.0 before applying z-score normalization, matching Forecast’s
LastWindow behavior. Preserve Forecast’s existing normalization flow so both
paths produce the same normalized value for corrupt inputs.
In `@src/TimeSeries/ProphetModel.cs`:
- Around line 917-920: Update SerializeCore and DeserializeCore to persist and
restore every remaining serializable ProphetOptions member, plus
_anomalyThreshold, _residualStdDev, and _residualMean, so loaded models preserve
transformation, anomaly detection, and prediction-interval behavior. Keep
Optimizer and TransformPrediction excluded from serialization and document that
callers must re-supply them after loading; restore the values alongside the
existing option fields in DeserializeCore.
- Around line 348-357: In src/TimeSeries/ProphetModel.cs lines 348-357, narrow
the catch around CholeskyDecomposition<T>.Solve to the specific
non-positive-definite exception types it raises and call
System.Diagnostics.Trace.TraceWarning before the SvdDecomposition<T> fallback,
matching the existing pattern near line 999. In src/TimeSeries/ProphetModel.cs
lines 430-441, narrow the DateTime.FromOADate handling to catch only
ArgumentException so invalid holidayIndex values propagate instead of being
treated as non-holidays.
- Around line 322-328: Update the design-matrix construction around the
holiday-indicator loop to build a HashSet<DateTime> of holiday dates once before
iterating rows, using each configured holiday’s Date. Convert each row’s time
value to DateTime once, then test that value against the set when populating
holiday columns instead of calling IsHoliday for every holiday.
- Line 392: Replace the embedded 25 in the changepoint count calculation with a
named constant or configurable property on ProphetOptions<T, TInput, TOutput>,
preserving the existing Prophet default of 25 while allowing the value to be
identified and, if exposed as an option, tuned by users.
- Around line 599-612: Extract the shared harmonic-count calculation into a
private ProphetModel helper, such as HarmonicsForPeriod(double period),
preserving the existing FourierOrder normalization and Nyquist cap. Replace the
duplicated formula in the design-matrix layout code and the seasonal-value loop
with calls to this helper so serialization/deserialization always reproduces the
same _seasonalComponents mapping.
- Around line 409-427: Update ComputeEffectiveSeasonalPeriods to accept the
observed time span (tMax - tMin) rather than row count, and update its callers
to pass that span from the time values used by FitLeastSquares. Compare explicit
periods against the span, preserving the intended two-cycle identifiability
threshold consistently, and replace each default 2 * period <= n check with the
corresponding comparison against the span.
In `@src/TimeSeries/TiDEModel.cs`:
- Around line 269-277: Wire the existing ToFiniteT helper into PredictSingle by
replacing the direct NumOps.FromDouble conversion with ToFiniteT(pred) before
GuardPrediction, preserving the post-conversion finiteness safeguard and
correcting the adjacent comment if GuardPrediction already handles it. At
src/TimeSeries/TiDEModel.cs lines 269-277, make this call-site change; at lines
75-86, retain ToFiniteT because it is then used, otherwise delete the helper if
the call site is not changed.
- Around line 149-178: Update the target-statistics logic in the TiDEModel
scaler-fitting method to mirror NLinearModel.FitScalers: include only finite y
values when calculating _targetMean and _targetStd, with a valid-count
denominator. Also update the training batch path around target normalization and
gradient accumulation so rows with non-finite targets are skipped or otherwise
cannot produce non-finite errors or gradients; preserve normal processing for
finite targets.
---
Outside diff comments:
In `@src/CausalDiscovery/Bayesian/DiBSAlgorithm.cs`:
- Around line 239-266: Bound the sample-size scaling in the gradient calculation
so edge selection remains stable across dataset sizes. Update the
dataScale/prior handling in the surrounding gradient loop, and apply the same
priorScale to sparsityGrad and acycGrad before they are combined with dataGrad,
preserving a fixed likelihood-to-prior ratio.
In `@src/TimeSeries/NLinearModel.cs`:
- Around line 269-270: Update NLinearModel<T>.CreateInstance to pass the
existing _optimizer into the reconstructed NLinearOptions<T>, preserving the
injected optimizer for Clone() and other model-rebuild paths.
- Around line 230-251: Update NLinearModel.DeserializeCore to capture the
persisted length from the stream and validate it matches the constructor-derived
_l before reading weights; do not discard it. Preserve compatibility with
payloads written before scaler statistics were serialized by guarding the four
additional reads with the stream-position check used by
TiDEModel.DeserializeCore, leaving existing defaults when the payload has no
remaining data.
In `@src/TimeSeries/ProphetModel.cs`:
- Around line 984-1012: Remove the unused states matrix allocation and its final
SetRow call from the fitting method. Keep SyncModelParametersFromState before
optimization and rely on OptimizeParameters, via ApplyParameters, to propagate
optimized values through the base class; remove the now-inaccurate “Store final
state” comment as well.
🪄 Autofix
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 Plus
Run ID: f66e7053-9d82-406c-a534-4549e8e0bf6b
📒 Files selected for processing (95)
src/AnomalyDetection/DistanceBased/COFDetector.cssrc/AnomalyDetection/Linear/KernelPCADetector.cssrc/AnomalyDetection/NeuralNetwork/VAEDetector.cssrc/CausalDiscovery/Bayesian/DiBSAlgorithm.cssrc/CausalDiscovery/ContinuousOptimization/ContinuousOptimizationBase.cssrc/CausalDiscovery/ContinuousOptimization/DAGMALinear.cssrc/CausalDiscovery/ContinuousOptimization/DAGMANonlinear.cssrc/CausalDiscovery/ContinuousOptimization/GOLEMAlgorithm.cssrc/CausalDiscovery/ContinuousOptimization/MCSLAlgorithm.cssrc/CausalDiscovery/ContinuousOptimization/NOTEARSLinear.cssrc/CausalDiscovery/ContinuousOptimization/NOTEARSLowRank.cssrc/CausalDiscovery/ContinuousOptimization/NOTEARSNonlinear.cssrc/CausalDiscovery/ContinuousOptimization/NOTEARSSobolev.cssrc/CausalDiscovery/DeepLearning/CASTLEAlgorithm.cssrc/CausalDiscovery/DeepLearning/CausalVAEAlgorithm.cssrc/CausalDiscovery/DeepLearning/DECIAlgorithm.cssrc/CausalDiscovery/Functional/CAMUVAlgorithm.cssrc/CausalDiscovery/Functional/ICALiNGAMAlgorithm.cssrc/CausalDiscovery/Functional/PNLAlgorithm.cssrc/CausalDiscovery/Functional/RCDAlgorithm.cssrc/CausalDiscovery/Functional/VARLiNGAMAlgorithm.cssrc/CausalDiscovery/Hybrid/MMHCAlgorithm.cssrc/CausalDiscovery/ScoreBased/BOSSAlgorithm.cssrc/CausalDiscovery/ScoreBased/GRaSPAlgorithm.cssrc/CausalDiscovery/TimeSeries/CCMAlgorithm.cssrc/CausalDiscovery/TimeSeries/LPCMCIAlgorithm.cssrc/CausalDiscovery/TimeSeries/NeuralGrangerAlgorithm.cssrc/CausalDiscovery/TimeSeries/TSFCIAlgorithm.cssrc/CausalInference/CausalForest.cssrc/CausalInference/DoublyRobustEstimator.cssrc/CausalInference/TLearner.cssrc/Finance/Portfolio/AssetGraphBuilder.cssrc/Finance/Portfolio/AttentionAllocation.cssrc/Finance/Portfolio/CVaRPortfolioObjective.cssrc/Finance/Portfolio/GraphAttentionLayerCore.cssrc/Finance/Portfolio/GraphAttentionPortfolio.cssrc/Finance/Portfolio/PathSignatureTransform.cssrc/Finance/Portfolio/SharpeRatioPortfolioObjective.cssrc/Finance/Portfolio/SignatureAugmentedAttention.cssrc/Finance/Portfolio/SignatureInformedTransformer.cssrc/Finance/Probabilistic/DiffusionTS.cssrc/Finance/Trading/Agents/FinRLAgent.cssrc/Finance/Trading/Environments/StockTradingEnvironment.cssrc/Finance/Trading/Environments/TradingEnvironment.cssrc/Finance/Trading/Factors/FactorTransformer.cssrc/Finance/Trading/Factors/FactorVAE.cssrc/Finance/Trading/Factors/Stockformer.cssrc/Finance/Trading/Factors/StockformerAttention.cssrc/Finance/Trading/Factors/StockformerBands.cssrc/Finance/Trading/Factors/StockformerDualEncoder.cssrc/Finance/Trading/Factors/StockformerMultiTaskLoss.cssrc/Finance/Volatility/NeuralGARCH.cssrc/Finance/Volatility/RealizedVolatilityTransformer.cssrc/MetaLearning/Algorithms/ATAMLAlgorithm.cssrc/MetaLearning/Algorithms/ConstellationNetAlgorithm.cssrc/MetaLearning/Algorithms/EPNetAlgorithm.cssrc/MetaLearning/Algorithms/ETPNAlgorithm.cssrc/MetaLearning/Algorithms/FRNAlgorithm.cssrc/MetaLearning/Algorithms/FeatureWiseTransformation.cssrc/MetaLearning/Algorithms/FlexPACBayesAlgorithm.cssrc/MetaLearning/Algorithms/GCDPLNetAlgorithm.cssrc/MetaLearning/Algorithms/LBANPAlgorithm.cssrc/ReinforcementLearning/Agents/DeepReinforcementLearningAgentBase.cssrc/ReinforcementLearning/Agents/DoubleQLearningAgent.cssrc/ReinforcementLearning/Agents/DreamerAgent.cssrc/ReinforcementLearning/Agents/DuelingDQNAgent.cssrc/ReinforcementLearning/Agents/DynaQAgent.cssrc/ReinforcementLearning/Agents/DynaQPlusAgent.cssrc/ReinforcementLearning/Agents/ModifiedPolicyIterationAgent.cssrc/ReinforcementLearning/Agents/MonteCarloExploringStartsAgent.cssrc/ReinforcementLearning/Agents/MuZeroAgent.cssrc/ReinforcementLearning/Agents/OffPolicyMonteCarloAgent.cssrc/ReinforcementLearning/Agents/OnPolicyMonteCarloAgent.cssrc/ReinforcementLearning/Agents/PolicyIterationAgent.cssrc/ReinforcementLearning/Agents/PrioritizedSweepingAgent.cssrc/ReinforcementLearning/Agents/QLambdaAgent.cssrc/ReinforcementLearning/Agents/SARSALambdaAgent.cssrc/ReinforcementLearning/Agents/TabularActorCriticAgent.cssrc/ReinforcementLearning/Agents/ValueIterationAgent.cssrc/ReinforcementLearning/Agents/WatkinsQLambdaAgent.cssrc/ReinforcementLearning/Policies/BetaPolicy.cssrc/SurvivalAnalysis/NelsonAalenEstimator.cssrc/SurvivalAnalysis/SurvivalModelBase.cssrc/TimeSeries/AnomalyDetection/DeepANT.cssrc/TimeSeries/AnomalyDetection/LSTMVAE.cssrc/TimeSeries/AutoformerModel.cssrc/TimeSeries/ChronosFoundationModel.cssrc/TimeSeries/DeepARDistributionHeads.cssrc/TimeSeries/DeepARModel.cssrc/TimeSeries/InformerModel.cssrc/TimeSeries/NBEATSBlock.cssrc/TimeSeries/NHiTSModel.cssrc/TimeSeries/NLinearModel.cssrc/TimeSeries/ProphetModel.cssrc/TimeSeries/TiDEModel.cs
💤 Files with no reviewable changes (4)
- src/MetaLearning/Algorithms/GCDPLNetAlgorithm.cs
- src/MetaLearning/Algorithms/FlexPACBayesAlgorithm.cs
- src/Finance/Trading/Factors/FactorTransformer.cs
- src/Finance/Portfolio/AttentionAllocation.cs
DiBSAlgorithm, MMHCAlgorithm, NeuralGrangerAlgorithm and DeepCausalBase each carried a private copy of the same routine: column mean, population standard deviation, guard near zero, divide. Four copies of one numeric routine drift, and they already had: the guard sat at 1e-10 in two and 1e-12 in a third, a constant column was zeroed in two but mean-centred with a unit divisor in another, and DeepCausalBase used the sample standard deviation where every other copy used the population one. CausalDiscoveryBase now exposes StandardizeColumns and the four copies are gone. The shared version zeroes a column whose standard deviation falls below a named ConstantColumnStdDevTolerance: such a column carries no information, and rescaling its residual noise to unit variance manufactures correlations that are not in the data. Unifying on the population standard deviation rescales DeepCausalBase's output by a per-column constant, which is exactly what standardization exists to remove -- correlation and the discovered structure are unchanged. ContinuousOptimizationBase.StandardizeData and FunctionalBase.StandardizeData are left alone deliberately: the first applies a documented per-column perturbation that breaks exact collinearity for NOTEARS, and folding it into the shared routine would change solver behaviour that no review comment asked to change. Verified: build error count unchanged at the branch baseline of 92 (all pre-existing slice dependencies), with no new diagnostics. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… hiding BOSSAlgorithm cited Year = 2022 against arXiv:2310.17679, which was submitted in October 2023 and appeared at NeurIPS 2023. The year belonged to the previous citation. BetaPolicy carried initials where the ResearchPaper metadata takes full author names. RCDAlgorithm had two <summary> blocks on ConfoundingRatio: DiffMutualInfo's documentation was stranded above it when ConfoundingRatio was inserted between the block and its method, which leaves DiffMutualInfo undocumented and generates a description of a different method for ConfoundingRatio. Duplicate summary tags are also invalid documentation XML and raise CS1571 wherever a DocumentationFile is configured. The block moves down to its method. Stockformer.Lift had the same duplicate-summary defect, and the stale first line described lifting a scalar when the method projects featureCount factors. While there, the null-forgiving operator on the lift layer becomes a real guard: a forward pass before the layers are built is a wiring bug, and it should say so rather than surface as a NullReferenceException from inside Engine.Reshape. ICALiNGAMAlgorithm.FindCausalOrder had an unreachable natural-order fallback. The loop tests TryFindCausalOrder before the exhaustion check, and once every entry is zeroed the matrix is edgeless, so TryFindCausalOrder always succeeds and the fallback never ran. The loop is now bounded by the entry count and the terminal state is real; if it were ever reached without an order, that is a broken invariant, and returning the natural order there would hide it behind a plausible-looking graph. NOTEARSLowRank's `outerIter >= 3` convergence gate becomes MinimumDualAscentUpdates, with the citation attached (Fang et al. 2023, Algorithm 1) and the consequence documented: a caller who sets MaxIterations below it gets a solver whose tolerance check never runs. Verified: build error count unchanged at the branch baseline of 92, no new diagnostics. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Deployment failed for project aidotnet_website with the following error: Learn More: https://vercel.com/franklins-projects-02a0b5a0?upgradeToPro=build-rate-limit |
…p sharing fitted vectors DoublyRobustEstimator.SetParameters sized its three coefficient vectors with (Length + 2) / 3, which rounds UP. GetParameters emits three equal-length segments, so any other length produced vectors longer than the data available to fill them: the trailing coefficients stayed at zero, IsFitted was set regardless, and the estimator answered predictions from a silently truncated model. It now requires a non-empty multiple of three and rejects anything else at the boundary. The per-loop idx guards go with it -- they were masking the same invariant rather than enforcing it. DoublyRobustEstimator.DeepCopy dropped FeatureNames, so a clone lost the feature metadata its own summary output reads. Cloned as an independent array, matching CausalForest. TLearner accepted persisted state marked fitted with a missing or truncated weight array. Either vector then stayed at length zero and PredictTreated, PredictControl and EstimateTreatmentEffect indexed it once per feature and threw from inside prediction. Both vectors must now match NumFeatures, and the failure names what is wrong with the file. NelsonAalenEstimator.DeepCopy shared its four fitted vectors with the clone. The comment argued this was safe because Fit reassigns rather than mutates -- true of Fit, but EventTimes, BaselineSurvival, CumulativeHazard and Variance are all public and mutable, so a caller writing through any of them changed both estimators. Cloned instead. A stray duplicate <inheritdoc /> above Serialize is removed while there. Verified: build error count unchanged at the branch baseline of 92, no new diagnostics. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Deployment failed for project aidotnet-playground-api with the following error: Learn More: https://vercel.com/franklins-projects-02a0b5a0?upgradeToPro=build-rate-limit |
TabularActorCriticAgent exported and restored its tables through two different walks. GetParameters sized the vector as valueTable.Count + policy.Count * ActionSize but filled it by iterating each state's actual action entries, and SetParameters wrote back by looping 0..ActionSize-1 for every state regardless of which actions that state held. A tabular agent that has not visited every action has a ragged policy table, which is a normal state, not a corrupt one -- and for any such table the two walks disagree. Values then landed on the wrong (state, action) pair, and the idx < parameters.Length guards hid the mismatch instead of reporting it. Both paths now walk the same ordered (state, action) entries the policy actually holds, and restore requires an exact length. Ordering is ordinal by key rather than dictionary order: Dictionary guarantees nothing about enumeration order across insertions and removals, so a vector written in one order and read back in another is silently wrong rather than loudly broken. GetParameters also padded an empty agent's vector to length 1. ParameterCount is defined as GetParameters().Length, so a new agent reported one parameter that does not exist and that SetParameters had nowhere to put back. An untrained agent now reports zero. Verified: build error count unchanged at the branch baseline of 92, no new diagnostics. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
AssetGraphBuilder.FilterTmfg left bestFace at -1 whenever no gain comparison succeeded, which is exactly what a NaN weight produces: NaN > bestGain is false for every face. The loop then indexed faces[-1] and threw ArgumentOutOfRangeException from inside the greedy insertion with nothing for the caller to act on. FilterTmfg is public and takes a caller-supplied matrix, so this is reachable input rather than an internal invariant, and it now names the cause. The same method sorted through `strength.Clone() as double[] ?? strength`. The cast cannot fail for a double[], so the fallback was dead code -- but it documented the opposite of the intent, reading as permission to sort the caller's computed array in place. Made explicit. CVaRPortfolioObjective.TransactionCost rejected negative and NaN basis points but accepted PositiveInfinity, which propagated infinity into the returned cost for any non-zero turnover and produced NaN when turnover was zero. Non-finite values are now rejected, spelt out longhand because net471 has no double.IsFinite. GraphAttentionLayerCore.ConcatenateHeads read headOutputs[0].Shape[1] before any rank check, and its validation loop starts at h = 1. A rank-1 first head therefore threw IndexOutOfRangeException from the shape access instead of the ArgumentException every other malformed head produces. Head 0 is now checked by the same rule as the rest. Verified: build error count unchanged at the branch baseline of 92, no new diagnostics. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…t could not tell failure from success DAGMALinear stored the caller's learning rate in a mutable field that SolveInnerProblem halves on every M-matrix domain violation and never restores. Decay across one run is intended; keeping it in the field was not. A second DiscoverStructure call on the same instance started from whatever the first decayed it to, possibly 1e-16, so one model object returned a different graph for identical data. The configured value is now readonly and the decaying rate is a per-run local threaded through the outer loop. DAGMALinear also applies CausalDiscoveryOptions.MaxIterations, documented as an outer iteration budget, to its inner Adam loop, because its outer loop is fixed at the five central-path steps in _sValues. That deviation is now stated where it happens, including the consequence: MaxIterations = 100 permits up to 500 Adam steps in total. MCSLAlgorithm passed a possibly-NaN value to Math.Sign, which throws rather than returning zero. The loop reaches NaN by design: rho multiplies by 10 per failed constraint gate up to 1e16, augCoeff * hGrad overflows to Infinity, and the Adam update writes NaN into W. The throw then tore the run down where the rho ceiling would have clamped it. Guarded the same way NOTEARSLowRank already guards the identical failure. DAGMANonlinear carried its own ProjectToDag and ContainsDirectedCycle. The local copy sorted edges by magnitude with no tiebreak, and List<T>.Sort is introsort, so equal-magnitude edges were ordered arbitrarily and the projected DAG could differ between runs on identical input. It also rescanned the whole graph after every insertion and recursed to depth d. Both are deleted in favour of the shared EnforceAcyclic that NOTEARSNonlinear and NOTEARSSobolev already use, which breaks ties by (From, To), asks the cheaper reachability question, and traverses iteratively. NOTEARSLinear's step-size stop fired whenever the iterate stopped moving, which a stalled line search produces just as reliably as convergence does. With all 20 Armijo trials failing, the fallback step of norm 1e-4 spread over many coordinates leaves the largest per-coordinate move below INNER_TOLERANCE; the subproblem exited on its first iteration with W unchanged, h(0) = 0 satisfied the outer HTolerance, the solver terminated at zeros, and FallbackCorrelationGraph silently substituted a correlation graph for the NOTEARS result. The stop now requires an accepted Armijo step. Verified: build error count unchanged at the branch baseline of 92, no new diagnostics. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… add two missing guards DECIAlgorithm's covariance fallback ranked candidates by correlation, which is symmetric: cov[i,j] equals cov[j,i] and the denominator uses both variances, so (i, j) and (j, i) always scored identically. List<T>.Sort is introsort and not stable, so which of the two tied entries survived the greedy projection depended on internal partitioning that varies with list length and runtime version. The reverse was then rejected as a cycle. DECI was choosing edge direction by an implementation detail rather than by evidence. Each unordered pair is now considered once and oriented from the higher-variance variable to the lower-variance one -- the additive-noise attenuation direction, the same rule CausalVAEAlgorithm's fallback already applies -- and the sort carries a (From, To) tiebreak so equal-strength candidates have a total order. The same method hardcoded 0.1 in three places while inheriting a configurable EdgeThreshold that defaults to the same value, so a caller who changed it still got the built-in cutoff. All three now read the option. ICALiNGAMAlgorithm divided by n in CenterData, WhitenData and FastICA with no guard, so n = 0 returned an adjacency matrix of NaN rather than an empty graph, and d < 2 ran the eigen decomposition and row assignment on degenerate matrices. PNLAlgorithm already returns an empty matrix for this condition and RCDAlgorithm throws; ICALiNGAM now matches its siblings. CCMAlgorithm read options.DirectionalityAsymmetryThreshold, which CausalDiscoveryOptions never defined. Added, with the 0.2 default the algorithm assumes and the reason a margin is needed at all documented: without one, every pair yields an edge in whichever direction scored marginally higher. Verified: build error count unchanged at the branch baseline of 92, no new diagnostics. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…model TiDEModel protected its inputs and not its targets. Window clamps non-finite entries, so _inputMeans and _inputStds were safe, but the target path called Convert.ToDouble(y[row]) with no filter. A single NaN or Infinity label made _targetMean NaN; _targetStd is rescued back to 1.0 but nothing rescued the mean, so every normalized target, every error and every accumulated gradient turned NaN. Training completed without an exception and every later prediction collapsed to 0 through the output guard -- a total failure with no signal. Non-finite labels are now excluded from the mean and variance, counted separately so the divisor matches, the same way NLinearModel.FitScalers already does it. Filtering the statistics is not sufficient on its own, so the batch loop also skips a row whose label is non-finite rather than letting its NaN error reach every weight it touches. TiDEModel.PredictSingle kept the pre-conversion finiteness check that ToFiniteT was added to replace, leaving the helper dead and the comment beside it contradicting the code. Every double from about 3.4e38 up is finite yet overflows to Infinity once narrowed to float, which is exactly the case the helper exists for. The call site now uses it; GuardPrediction still runs behind it for the recursive-forecast path. NLinearModel imputed in two different spaces. TrainCore normalizes inside the window accessor, so a non-finite entry is clamped to 0.0 in normalized space, which is the mean in raw terms. PredictSingle clamped the raw value to 0.0 and normalized afterwards, giving (0 - _xMean) / _xStd. The same corrupt input meant two different things, and the gap grows with _xMean. Forecast now takes the raw accessor and normalizes before the clamp, so both paths impute in the normalized space. Verified: build error count unchanged at the branch baseline of 92, no new diagnostics. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… share the harmonic formula Both new catch blocks caught every exception type where one specific type applies, and neither recorded that the degraded path ran. The Cholesky solve now catches only what a non-positive-definite normal matrix raises and traces a warning before falling back to SVD, matching the pattern the optimizer catch below it already uses; a NullReferenceException or an out-of-range index there previously became a silent fallback to a wrong fit. The FromOADate conversion now catches only ArgumentException, so an out-of-range holiday index propagates instead of being reported as "not a holiday" and silently emptying that column of the design matrix. That conversion also moves out of the holiday loop. IsHoliday converted the row time once per holiday, so building the design matrix performed n x holidayCount conversions, each inside its own try/catch. The row's date is now computed once and compared against each holiday. The Fourier harmonic count -- min(order, max(1, floor(period / 2))) -- existed in two places, and the mapping from (period, order) to positions in _seasonalComponents depends on both producing the same answer. Serialization stores the periods and the order rather than the counts, so deserialization reproduces the layout through the same formula. Had the copies diverged, the bounds check in the seasonal evaluation would not have reported it: it would have returned a truncated seasonal term and forecast wrongly with no diagnostic. Extracted to one HarmonicCount helper. The changepoint count 25 becomes DefaultChangepointCount with the reference attached and its effect on trend flexibility documented. Verified: build error count unchanged at the branch baseline of 92, no new diagnostics. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…s saved SerializeCore wrote the fit and four options. DeserializeCore then replaced _prophetOptions with a fresh default instance and restored only FourierOrder, SeasonalPeriods, Holidays and RegressorCount, so every other setting silently reverted. The residual statistics were never written at all. The fit survived the round trip. The behaviour around it did not: - PredictSingle reads ApplyTransformation, so a model trained with a transformation returned untransformed predictions after loading. - DetectAnomalies and GetAnomalyThreshold threw InvalidOperationException because EnableAnomalyDetection reverted to false, and the message told the user to retrain a model that had been trained correctly. - PredictWithIntervals threw because _residualStdDev reverted to zero. All fifteen remaining scalar options are now persisted, along with _residualMean, _residualStdDev and _anomalyThreshold, and read back in the order they were written. Optimizer and TransformPrediction cannot follow: one is an interface reference and the other a delegate. Both revert to their defaults, and that is now documented at the restore site along with its consequence -- ApplyTransformation is restored faithfully, so a model saved with a custom transform applies the identity transform until TransformPrediction is set again. Verified: build error count unchanged at the branch baseline of 92, no new diagnostics. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… architecture SharpeRatioPortfolioObjective.Loss floored the mean and the volatility with the same constant, which destroyed the ordering its own documentation promises. A degenerate losing series with mean -0.01 and volatility 1e-15 collapsed to -ln(1e-12) + ln(1e-12) = 0, while an honest profitable series with mean 0.01 and volatility 0.02 scored about 0.69. Minimizing the objective preferred the losing portfolio. A non-positive mean is now its own branch returning a large finite penalty that no volatility term can offset, and subtracting the mean keeps losing candidates ordered among themselves so an optimizer still has a direction to follow. GraphAttentionPortfolio and SignatureInformedTransformer both truncated a long score vector and passed a short one through unchanged, producing an allocation with fewer entries than the asset universe. Callers index weights by asset, so every entry from the first missing one onward referred to the wrong asset. Both now reject a short vector. Both CreateNewInstance overrides rebuilt options and called the single-argument constructor, so a model built with a custom architecture or loss function cloned into one with default layers and the implicit default loss. Both now pass Architecture and LossFunction through. GraphAttentionPortfolio.UpdateParameters sliced sequentially with no length check, so a short vector threw partway through and left the model in a state that was neither the old one nor the new one, while a long vector left its tail silently unused. The total is now checked before any layer is mutated. BuildAdjacency fell back to _options.NumAssets for a non-rank-2 panel, but BuildGraph reaches Graph.VolatilitySeries, which throws for exactly that input, so the fallback was unreachable and documented a contract the method does not support. Rank is validated once instead. Verified: build error count unchanged at the branch baseline of 92, no new diagnostics. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…mplementation helper FeatureWiseTransformation was public but no public facade method or options type exposes it, so it added consumer-visible surface for an implementation helper. Made internal, keeping the public API to AiModelBuilder and AiModelResult. Its constructor accepted NaN and infinity for both initial values, which propagate into every sampled scale and from there into every transformed output, where they read as a modelling result rather than as bad input. Both are now required to be finite. SetHyperparameters accepted vectors of any length. A short bias vector made Apply throw on an index, and a matching-but-short PAIR was worse: it simply stopped transforming the trailing channels with no error at all. Both lengths must now match the configured feature dimension, and neither field is written unless both are usable, so a rejected call leaves the transformation on its previous state. COFDetector repeated the same _trainingDistanceMatrix null guard in both arms of one ternary and reached the training data through a null-forgiving operator, which would have surfaced as a NullReferenceException from inside ComputeDistanceMatrix rather than as a "not fitted" message. Both fitted fields are resolved once, before either branch. ScoreAnomalies now documents that in-sample scoring requires the same INSTANCE, not an equal one: the training path is chosen by reference identity so each point can exclude itself from its own neighbourhood. A value-identical copy takes the query path, keeps every point as its own zero-distance neighbour, collapses the chain cost, and produces scores that are not comparable with the contamination threshold Fit calibrated. Verified: build error count unchanged at the branch baseline of 92, no new diagnostics. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ing number StockformerMultiTaskLoss skipped any entry whose absolute error was NaN and did not count it as valid. The mask exists to drop missing LABELS, and those entries have labels -- a NaN there means the PREDICTION diverged. Skipping it removed the only evidence: with every prediction NaN, valid stayed 0 and the method returned 0.0, reporting a perfect regression term for a completely broken model. The NaN now propagates, which is what a diverged model should say. SignatureAugmentedAttention's gate setter rejected NaN but accepted infinity, and Softplus(PositiveInfinity) returns infinity, so ApplyBias produced infinity for a non-zero bias and NaN for a zero one -- against a documented contract that the gate stays strictly positive and finite. Its attention also computed 1 / sqrt(dk) without checking dk: a zero key dimension passes the rank and match checks, and every logit then became 0 * infinity = NaN. StockformerDualEncoder checked the rank of `low` and then read `high.Shape[d]` up to d = 2, so a rank-1 or rank-2 `high` threw IndexOutOfRangeException from the shape access instead of the ArgumentException that names the problem. Stockformer.Encoder called InitializeLayers and then returned `_encoder!`. InitializeLayers returns immediately when Layers.Count > 0, so that call cannot repair a null encoder -- exactly the state deserialization and clone flows produce when they restore Layers through the base class. It now throws with a message that names the cause. SignatureInformedTransformer validated its options twice on one construction path; GraphAttentionPortfolio validates once. Now both do. Verified: build error count unchanged at the branch baseline of 92, no new diagnostics. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…erence mode CreateNewInstance copies KlWeight, Seed and UseAMSGrad, but SerializeNetworkSpecificData and DeserializeNetworkSpecificData still handled only the older scalars. A model saved after training and reloaded into an instance built from defaults therefore got a different KL weight and a different sampling seed, so the reloaded model did not behave like the saved one. All three are now written and read, with a presence flag for the nullable seed. PredictCore's factor-span path walked Layers through RunSpan without touching training mode. The default native stack contains BatchNormalizationLayer and DropoutLayer, and PredictNative switches to inference mode for exactly that reason, so a prediction taken after a training step applied batch statistics and dropout. The span path now does the same, and restores the previous mode afterwards so a caller who was mid-training is left as it was found. Verified: build error count unchanged at the branch baseline of 92, no new diagnostics. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ed other code Restore read `restored[(row * time) + t]` using the time length of the INPUT, but the upsample filters are DenseLayer(_options.SequenceLength), so their output width is always SequenceLength regardless of the window. A longer window read past the end of the restored tensor; a shorter one silently picked values out of the wrong row. ReadShape accepts any time length, so PredictBands reached both. The two widths are now compared where both are in hand, and the mismatch names what to change. StockformerBands.Split documented the returned high band as the FINEST detail while the loop returns the coarsest. The code is right: only the coarsest detail has the same length as the low band, since each level halves both, and pairing the level-1 detail with a low band split Levels times would hand the encoder's two branches different sequence lengths. The documentation now says that, and notes the distinction only appears above the paper's Levels = 1, where the two coincide. StockformerMultiTaskLoss contradicted itself three ways: the summary said EQUAL weight, the remarks said weights were "considered and rejected" and that adding one "would be an invented deviation", and the signature exposed an undocumented taskLossWeight whose comment cited the paper's Eq. 12. Resolved in favour of the parameter, which is the right shape for this codebase: the paper defines lambda, so it is exposed, with the reference's 1.0 as the default, so calling Compute without it reproduces the reference exactly. The invented deviation would be a different default, not the knob. taskLossWeight now has its <param> entry. Verified: build error count unchanged at the branch baseline of 92, no new diagnostics. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
AssetGraphBuilder.DistanceCorrelationMatrix called DistanceCorrelation once per pair, and each of those calls double-centred BOTH of its arguments. Centring is the expensive half -- O(n^2) per call plus an n x n allocation -- so every column paid for it (assets - 1) times over. The computation splits into DistanceCorrelationFromCentred, and the sweep centres each column once up front. The public DistanceCorrelation is unchanged for callers scoring a single pair. GraphAttentionLayerCore's softmax read adjacency through NumOps.ToDouble and re-evaluated LeakyReLU(sourceScore[u] + targetScore[v]) once for the max pass and again for the exponential pass, doubling the conversion cost of the hot inner loop on a dense adjacency. Both are now computed once per row and reused. The third pass also keys off the same neighbour flags rather than testing exps[v] == 0.0, which previously conflated "not a neighbour" with "a neighbour whose exponential underflowed". PathSignatureTransform recomputed the increment dj inside the inner j loop, so each step made 2 * dim^2 NumOps.ToDouble calls where 2 * dim are enough. The transform runs per window over an asset panel, so this is a hot path. The step's increments are computed once and reused for both the level-1 and level-2 accumulation. Verified: build error count unchanged at the branch baseline of 92, no new diagnostics. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lation LPCMCIAlgorithm.OLSResiduals accumulated raw, uncentred Z and y, forcing the regression through the origin. With non-zero means the omitted constant stays in the residuals -- in residTarget and residSource alike -- and PearsonCorrelation's own centring cannot remove it, because the two residual vectors remain correlated THROUGH that shared constant. Nothing else in the file centres the input, so every PC1 and MCI test on data with non-zero means was biased. Both the normal equations and the residual reconstruction now work on mean-centred columns, which is equivalent to fitting an intercept and leaves already-centred data unchanged. That solve also moves from LU to Cholesky. The ridge-adjusted normal matrix is symmetric positive-definite by construction, which is the case Cholesky exists for. TSFCIAlgorithm computed the same argmax three times. Phase 1 scanned every lag and threw the winning lag away; Phase 2 rebuilt it as bestLag, and the orientation step rebuilt it again as bestLagItoJ. The two were necessarily equal, and the orientation step's two maxima were already sitting in skeleton[i, j] and skeleton[j, i]. ComputeLaggedCorrelation is O(n) per call, so this tripled the dominant cost -- and left three loops that had to stay in step, where editing one would silently desynchronize the lag used for the independence test from the lag used for the edge weight. Phase 1 now records the argmax and both later uses read it. NeuralGrangerAlgorithm allocated its hidden buffer inside the sample loop, next to a comment claiming temporary allocations had been removed: d * maxEpochs * effectiveN allocations. Now allocated once per target and overwritten. Verified: build error count unchanged at the branch baseline of 92, no new diagnostics. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ConditionalValueAtRisk sized its tail as ceil((1 - alpha) * n) whole observations. The
documentation claims this agrees with the Rockafellar-Uryasev dual, and DualObjective and
OptimalNu are public precisely so the two can be cross-checked -- but they only agreed when
(1 - alpha) * n happened to be an integer. Otherwise rounding up counted the boundary
observation as a whole one and diluted the average.
The reviewer's counterexample, alpha = 0.75 over losses {0, 1, 2, 3, 4}: ceil(1.25) = 2 gives
(3 + 4) / 2 = 3.5, while the dual at its optimal nu = 3 gives 3 + 1 / (5 * 0.25) = 3.8. Any
test asserting the two agree would fail for every fractional tail size.
The tail is now interpolated: whole observations beyond the boundary count fully and the
boundary observation contributes its fractional share. On the same example that is
(4 + 0.25 * 3) / 1.25 = 3.8, matching the dual exactly.
Verified: build error count unchanged at the branch baseline of 92, no new diagnostics.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…d ones VARLiNGAM forms (I - B_0) M_tau to recover the lagged structural coefficients, and read B_0 from the graph DirectLiNGAM returns. That graph is thresholded: DirectLiNGAM writes a coefficient only when its magnitude clears the edge threshold, so every smaller value is an exact zero there. Doing matrix algebra with it therefore biases B_tau by dropping the real contribution of every sub-threshold instantaneous effect. Thresholding belongs to the reported graph, not to the arithmetic. DirectLiNGAM now records the unthresholded coefficients as it goes and exposes them internally; VARLiNGAM uses those for the product while its own final result stays thresholded as before. The returned DirectLiNGAM graph is unchanged. Verified: build error count unchanged at the branch baseline of 92, no new diagnostics. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (19)
src/CausalInference/DoublyRobustEstimator.cs (1)
441-448: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBLOCKING: Reject feature matrices with an incompatible column count.
PredictOutcomenow limits the loop toavailableFeatures. A model restored withNumFeatures = 2can receive a matrix with more columns and silently ignore the extra features. This returns a treatment-effect estimate for a different feature schema.Require
x.Columns == NumFeaturesbefore prediction. Also requirecoefficients.Length == NumFeatures + 1for the intercept-plus-feature layout.Proposed fix
private Vector<T> PredictOutcome(Matrix<T> x, Vector<T> coefficients) { + if (x.Columns != NumFeatures) + throw new ArgumentException( + $"Expected {NumFeatures} feature columns, got {x.Columns}.", + nameof(x)); + if (coefficients.Length != NumFeatures + 1) + throw new InvalidOperationException("Outcome coefficient layout is invalid."); + var predictions = new Vector<T>(x.Rows); - int p = x.Columns; - int availableFeatures = Math.Max(0, coefficients.Length - 1); + int p = x.Columns;🤖 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/CausalInference/DoublyRobustEstimator.cs` around lines 441 - 448, Update PredictOutcome to validate the feature schema before iterating rows: require x.Columns to equal NumFeatures and coefficients.Length to equal NumFeatures + 1, rejecting incompatible inputs rather than truncating or ignoring columns. Preserve the existing intercept-plus-feature prediction logic only after both validations pass.Source: Path instructions
src/Finance/Trading/Factors/StockformerBands.cs (1)
136-140: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBLOCKING: Make
BandLengthvalidate the same input domain asSplit.
BandLength(1)returns0, althoughSplitrejects a one-sample series. For a highLevelsvalue, it can also return zero beforeSplitAllallocates zero-width matrices and later fails inSplit.Reject lengths that cannot support every configured decomposition level.
Proposed fix
public int BandLength(int inputLength) { int length = inputLength; - for (int level = 0; level < Levels; level++) length = length / 2; + for (int level = 0; level < Levels; level++) + { + if (length < 2) + throw new ArgumentException( + $"Input length {inputLength} cannot support {Levels} decomposition levels.", + nameof(inputLength)); + length /= 2; + } return length; }🤖 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/Finance/Trading/Factors/StockformerBands.cs` around lines 136 - 140, Update BandLength to reject input lengths that cannot support all configured decomposition levels, matching Split’s validation domain and preventing zero-width results for values such as 1 or insufficient lengths for high Levels. Preserve the existing halving calculation for valid inputs and ensure SplitAll receives only supported lengths.Source: Path instructions
src/Finance/Trading/Factors/StockformerMultiTaskLoss.cs (2)
155-175: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBLOCKING: Validate
taskLossWeight.A
NaNor infinitetaskLossWeightmakesTotalinvalid. A negative value reverses the classification loss and rewards worse classification results.Require a finite, non-negative weight before calculating the objective.
public static (double Regression, double Classification, double Total) Compute( @@ double missingSentinel = 0.0, double taskLossWeight = 1.0) { + if (double.IsNaN(taskLossWeight) || double.IsInfinity(taskLossWeight) || taskLossWeight < 0) + throw new ArgumentOutOfRangeException( + nameof(taskLossWeight), + "Task loss weight must be a finite non-negative value."); +🤖 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/Finance/Trading/Factors/StockformerMultiTaskLoss.cs` around lines 155 - 175, Validate taskLossWeight at the start of StockformerMultiTaskLoss.Compute, rejecting NaN, positive or negative infinity, and negative values before calculating regression, classification, or Total. Preserve the existing default and weighted objective for finite non-negative weights.Source: Path instructions
99-135: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBLOCKING: Reject an empty classification batch.
If
targets.Lengthis zero,CrossEntropyreturnstotal / 0.0. The loss becomesNaNwithout an input error.Reject empty
targetsbefore the division.public static double CrossEntropy(Vector<T> logits, Vector<T> targets, int numClasses) { if (logits is null) throw new ArgumentNullException(nameof(logits)); if (targets is null) throw new ArgumentNullException(nameof(targets)); + if (targets.Length == 0) + throw new ArgumentException("At least one target is required.", nameof(targets));🤖 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/Finance/Trading/Factors/StockformerMultiTaskLoss.cs` around lines 99 - 135, Update CrossEntropy to validate that targets.Length is greater than zero before processing or dividing the accumulated loss. Throw an appropriate argument exception for an empty classification batch, while preserving the existing validation and averaging behavior for non-empty targets.Source: Path instructions
src/MetaLearning/Algorithms/FeatureWiseTransformation.cs (1)
103-106: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBLOCKING: Validate
featuresbefore dereference.
Apply(null)throws aNullReferenceExceptionatfeatures.Length. Reject the invalid argument explicitly.public Vector<T> Apply(Vector<T> features) { + if (features is null) throw new ArgumentNullException(nameof(features)); + var result = new Vector<T>(features.Length);🤖 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/MetaLearning/Algorithms/FeatureWiseTransformation.cs` around lines 103 - 106, Update FeatureWiseTransformation.Apply to validate features before accessing features.Length, explicitly rejecting null with the established argument-validation behavior. Preserve the existing result allocation and channel-processing logic for non-null vectors.Source: Path instructions
src/CausalDiscovery/ContinuousOptimization/MCSLAlgorithm.cs (1)
64-74: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winValidate
MaxPenaltylike the sibling solver does.
_rhoMaxaccepts any caller value without a check. Two failure modes follow. IfMaxPenaltyis0or negative,rho >= _rhoMaxis true on the first outer iteration and the loop exits before any dual ascent. IfMaxPenaltyisNaN, bothMath.Min(rho * 10.0, _rhoMax)andrho >= _rhoMaxbehave as no-ops, so the escalation ceiling silently disappears.
NOTEARSLowRankin this same PR rejects both cases at the constructor (NOTEARSLowRank.cs:105-106). Apply the same guard here.🛡️ Proposed guard
- if (options?.MaxPenalty is { } maxPenalty) _rhoMax = maxPenalty; + if (options?.MaxPenalty is { } maxPenalty) + { + if (maxPenalty <= 0 || double.IsNaN(maxPenalty) || double.IsInfinity(maxPenalty)) + throw new ArgumentException("MaxPenalty must be positive and finite.", nameof(options)); + _rhoMax = maxPenalty; + }As per coding guidelines for
src/**: "missing validation of external inputs" is a production-readiness defect.🤖 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/CausalDiscovery/ContinuousOptimization/MCSLAlgorithm.cs` around lines 64 - 74, Validate options.MaxPenalty in the MCSLAlgorithm constructor before assigning _rhoMax, rejecting zero, negative, and NaN values with the same guard and behavior used by NOTEARSLowRank. Preserve the existing assignment for valid values and the default ceiling when MaxPenalty is unset.Source: Path instructions
src/CausalDiscovery/DeepLearning/DECIAlgorithm.cs (1)
262-272: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe posterior path still uses a hardcoded weight cutoff.
Line 291 in the fallback now compares against
EdgeThreshold. Line 268 in the posterior path still compares against a literal0.1. The two edge-selection gates therefore use different cutoffs, and a user who configuresEdgeThresholdchanges only one of them.Use
EdgeThresholdin both places.🐛 Proposed fix
- if (NumOps.GreaterThan(NumOps.Abs(weight), NumOps.FromDouble(0.1))) + if (NumOps.GreaterThan(NumOps.Abs(weight), NumOps.FromDouble(EdgeThreshold)))As per coding guidelines for
src/**: "hardcoded values instead of proper logic" and "magic numbers without constants or configuration" are blocking.🤖 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/CausalDiscovery/DeepLearning/DECIAlgorithm.cs` around lines 262 - 272, Replace the hardcoded 0.1 cutoff in the posterior edge-selection logic within DECIAlgorithm with the configured EdgeThreshold, matching the fallback path’s comparison while preserving the existing weight calculation and edge assignment behavior.Source: Path instructions
src/CausalDiscovery/ContinuousOptimization/NOTEARSLowRank.cs (1)
268-290: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecord the rejected optimizer step.
When
IsFiniteVectororIsFiniteMatrixfails, the solver silently discards the L-BFGS result and keeps the previous factors. WhenhValis non-finite, the outer loop silently breaks. Neither path leaves any diagnostic, so a run that produced a degenerate structure looks identical to a clean run.
ProphetModel.FitLeastSquaresin this same PR emitsSystem.Diagnostics.Trace.TraceWarningbefore its Cholesky-to-SVD fallback. Apply the same treatment to both branches here.♻️ Proposed change
- if (IsFiniteVector(optimized)) + bool accepted = false; + if (IsFiniteVector(optimized)) { var candidateA = new Matrix<T>(d, rank); var candidateB = new Matrix<T>(d, rank); UnflattenVectorToAB(optimized, candidateA, candidateB, d, rank); var candidateW = ReconstructW(candidateA, candidateB, d, rank); if (IsFiniteMatrix(candidateW)) { A = candidateA; B = candidateB; + accepted = true; } } + if (!accepted) + { + System.Diagnostics.Trace.TraceWarning( + $"[NOTEARSLowRank] Outer iteration {outerIter} produced non-finite factors; " + + "retaining the previous accepted factors."); + } // Outer: evaluate and update augmented Lagrangian var outerW = ReconstructW(A, B, d, rank); var (hVal, _) = ComputeNOTEARSConstraint(outerW); if (double.IsNaN(hVal) || double.IsInfinity(hVal)) + { + System.Diagnostics.Trace.TraceWarning( + $"[NOTEARSLowRank] Acyclicity residual is non-finite at outer iteration {outerIter}; stopping."); break; + }As per coding guidelines for
src/**: incomplete features include "half-implemented patterns where some code paths work but others silently do nothing".🤖 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/CausalDiscovery/ContinuousOptimization/NOTEARSLowRank.cs` around lines 268 - 290, Update the optimization flow around IsFiniteVector, IsFiniteMatrix, and the non-finite hVal check to emit System.Diagnostics.Trace.TraceWarning diagnostics whenever a candidate step is rejected or the outer loop terminates due to a non-finite constraint. Include enough context to distinguish the rejected optimizer result from the invalid outer constraint, while preserving the existing factor-retention and loop-break behavior.Source: Path instructions
src/TimeSeries/NLinearModel.cs (3)
247-256: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winBLOCKING:
DeserializeCorediscards the persisted window length and then reads with its own.Line 249 reads the
_lthatSerializeCorewrote at Line 237 and throws the value away. Line 250 then loops_ltimes using the field, which the constructor set fromNLinearOptions<T>.LookbackWindow, not from the payload._lisreadonly, so it cannot adopt the stored value.Two failures follow when the payload was written by a model with a different
LookbackWindow. If the current_lis smaller, the loop stops early, the stream stays positioned inside the weight block, and the four scaler reads at Lines 252-255 consume weight bytes as_xMean,_xStd,_yMeanand_yStd. The model loads without error and forecasts nonsense. If the current_lis larger, the reads run past the end and throwEndOfStreamExceptionfrom deep inside deserialization.Validate the stored length against the field and reject a mismatch.
🐛 Proposed fix
protected override void DeserializeCore(BinaryReader reader) { - reader.ReadInt32(); + int storedLookback = reader.ReadInt32(); + if (storedLookback != _l) + { + throw new InvalidOperationException( + $"Serialized {nameof(NLinearModel<T>)} was written with LookbackWindow {storedLookback}, " + + $"but this instance was constructed with {_l}. Construct the model with the matching " + + $"{nameof(NLinearOptions<T>.LookbackWindow)} before deserializing."); + } for (int j = 0; j < _l; j++) { _w[j] = reader.ReadDouble(); }As per coding guidelines for
src/**: "missing validation of external inputs" and "missing error handling at system boundaries" are blocking production-readiness defects.🤖 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/NLinearModel.cs` around lines 247 - 256, Update DeserializeCore to store the persisted window length read from the payload, validate it against the instance’s _l before reading weights, and reject mismatches with a clear deserialization error. Only proceed with the existing weight and scaler reads when the lengths match.Source: Path instructions
274-275: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
CreateInstancedrops the injected optimizer.The constructor accepts an
IGradientBasedOptimizer<T, Matrix<T>, Vector<T>>and stores it in the readonly_optimizer.CreateInstancepasses only the options, so the new instance falls back to the default Adam. A caller who supplied a configured optimizer loses it on everyClone()and on every framework path that rebuilds the model throughCreateInstance.The remark at Lines 42-48 states that "nothing here is hardcoded beyond that swappable default". This path contradicts that statement.
🐛 Proposed fix
protected override IFullModel<T, Matrix<T>, Vector<T>> CreateInstance() - => new NLinearModel<T>(new NLinearOptions<T>(_options)); + => new NLinearModel<T>(new NLinearOptions<T>(_options), _optimizer);Note that the optimizer instance is then shared between the source and the copy. If
AdamOptimizercarries per-model moment state, pass a fresh instance instead and document the behavior.🤖 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/NLinearModel.cs` around lines 274 - 275, Update NLinearModel<T>.CreateInstance() to pass the injected _optimizer into the new NLinearModel<T> so clones and rebuilds preserve the configured optimizer instead of selecting the default; if optimizer state must not be shared, create a fresh equivalent optimizer and document that behavior.
59-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
thisescapes from the constructor.
AdamOptimizer<T, Matrix<T>, Vector<T>>receives a reference to aNLinearModel<T>that is still under construction. The fields it might read are assigned above this line, so the current code is safe. The safety depends onAdamOptimizerstoring the reference without calling back into the model during its own construction.Nothing needs to change today. Note the coupling, because a later
AdamOptimizerchange that reads model state at construction time would break this silently.🤖 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/NLinearModel.cs` around lines 59 - 60, Document the constructor coupling at the optimizer initialization in NLinearModel<T>: AdamOptimizer receives this before construction completes, so preserve the current initialization order and avoid changes that invoke model state during optimizer construction. No code change is required unless AdamOptimizer’s construction behavior changes.src/CausalDiscovery/Functional/ICALiNGAMAlgorithm.cs (2)
204-212: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe identity-plus-perturbation initialization of
Wis dead.The deflation loop at Lines 215-270 assigns
W[p, j] = w[j]for everypfrom0tod - 1, so every entry written here is overwritten. The orthogonalization at Lines 250-257 reads only rowsk < p, and those rows already hold converged deflation results by the time they are read. No value written at Lines 209-211 is ever consumed.Note one side effect before deleting: the loop draws
d * dvalues fromrng, so removing it shifts the seeded random stream and changes the per-component starting vectors at Line 218. Re-check the ICA-LiNGAM integration expectations after the change.♻️ Proposed cleanup
var W = new Matrix<T>(d, d); - // Initialize W as identity + small random perturbation - for (int i = 0; i < d; i++) - { - W[i, i] = NumOps.One; - for (int j = 0; j < d; j++) - W[i, j] = NumOps.Add(W[i, j], NumOps.FromDouble(0.01 * (rng.NextDouble() - 0.5))); - } - // Deflation-based FastICAAs per coding guidelines for
src/**: "Dead code: ... unused variables/parameters that suggest incomplete refactoring".🤖 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/CausalDiscovery/Functional/ICALiNGAMAlgorithm.cs` around lines 204 - 212, Remove the unused identity-plus-perturbation initialization loop for W in the ICA algorithm, leaving W allocation intact for later deflation writes. Re-check the seeded RNG usage and ICA-LiNGAM integration expectations after removal, since eliminating the d*d draws changes the random starting vectors used by the deflation loop.Source: Path instructions
275-343: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a focused test for `FindRowPermutation
FindRowPermutationduplicates the Jonker-Volgenant solver inFedMaAggregationStrategy.HungarianAlgorithm. That existing helper is private and has a square-metric signature, so it does not prevent duplication. Since this is still a numerical solver, add a small test that asserts the recovered permutation for a known unmixing matrix; otherwise silent assignment/bookkeeping errors can produce a plausible but incorrect causal order.🤖 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/CausalDiscovery/Functional/ICALiNGAMAlgorithm.cs` around lines 275 - 343, Add a focused test covering FindRowPermutation with a known square unmixing matrix whose optimal assignment is unambiguous, and assert the exact recovered rowForColumn permutation. Exercise the solver through the owning ICALiNGAMAlgorithm API or an appropriate test-access mechanism, keeping the test targeted to assignment/bookkeeping correctness rather than duplicating the algorithm.src/ReinforcementLearning/Agents/TabularActorCriticAgent.cs (1)
192-192: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueCompute the count without materializing the vector.
GetParameters()copies two key lists, sorts both, and allocates aVector<T>.ParameterCountdiscards all of it and keeps only the length. Any caller that readsParameterCountin a loop pays that cost repeatedly.The single-source-of-truth goal is right. Keep it by deriving both the count and the vector from the same entry enumeration, rather than by building the vector to measure it.
♻️ Proposed refactor
- public override long ParameterCount => GetParameters().Length; + public override long ParameterCount => _valueTable.Count + PolicyEntryCount();Add the helper next to
OrderedPolicyEntries:/// <summary> /// The number of (state, action) preferences the policy holds. Counted the same way /// <see cref="OrderedPolicyEntries"/> enumerates them, so the count and the vector agree. /// </summary> private int PolicyEntryCount() { int count = 0; foreach (var state in _policy) count += state.Value.Count; return count; }🤖 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/ReinforcementLearning/Agents/TabularActorCriticAgent.cs` at line 192, Update ParameterCount to use a dedicated PolicyEntryCount helper instead of materializing GetParameters(). Implement PolicyEntryCount alongside OrderedPolicyEntries by summing each state’s action-count, preserving the same entry enumeration source so the reported count and parameter vector remain consistent.src/TimeSeries/ProphetModel.cs (1)
1096-1097: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
statesis allocated, written once, and discarded.Line 1097 allocates an
nbyGetStateSize()matrix. Line 1124 sets its last row. Nothing reads it, and it does not escapeTrainCore. The comment at Line 1123 says "Store final state for future reference", but there is no reference.The cost scales with the training set: for a series of 100000 points and a state size of 40, this allocates roughly 32 MB per
Train()call and then drops it.If the final state must be retained, assign it to a field. If not, delete both lines.
♻️ Proposed cleanup
- int n = y.Length; - Matrix<T> states = new Matrix<T>(n, GetStateSize()); - SyncModelParametersFromState();and remove the paired write:
- // Store final state for future reference - states.SetRow(n - 1, GetCurrentState()); -As per coding guidelines for
src/**: "Dead code: ... unused variables/parameters that suggest incomplete refactoring" is blocking.🤖 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/ProphetModel.cs` around lines 1096 - 1097, Remove the unused states allocation in TrainCore and the corresponding final-row assignment near the “Store final state for future reference” comment, since the matrix is never read or retained. Do not introduce a replacement field unless final-state retention is explicitly required.Source: Path instructions
src/TimeSeries/TiDEModel.cs (2)
222-249: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe batch divisor still uses the nominal batch size after rows are skipped.
Line 231 skips a row whose target is non-finite. Line 249 divides the accumulated gradient by
bs, which isbatchEnd - batchStart, the number of rows the batch would have contained. The gradient sum now runs over fewer rows than the divisor assumes.The result is a silently reduced step for every batch that contains a non-finite target. A batch of 32 with one valid row applies one thirty-second of the correct update. The skip is correct; the averaging was not updated to match it.
Count the rows that contribute and divide by that count.
🐛 Proposed fix
var gWr = new double[_l]; double gBr = 0; + int contributing = 0; for (int bi = batchStart; bi < batchEnd; bi++) { @@ double rawTarget = Convert.ToDouble(y[idx]); if (!IsFiniteValue(rawTarget)) continue; + contributing++; double normalizedTarget = (rawTarget - _targetMean) / _targetStd; @@ - double inv = bs > 0 ? lr / bs : 0.0; + double inv = contributing > 0 ? lr / contributing : 0.0;
bsthen becomes unused and can be removed.🤖 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/TiDEModel.cs` around lines 222 - 249, Update the gradient accumulation loop in the training method around Forward and the non-finite target check to count each row that contributes gradients, incrementing the count only after a finite target is accepted. Use this contributing-row count instead of the nominal bs when computing inv, and remove bs if it becomes unused.
339-352: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse an explicit version flag for TiDE normalization state.
BaseStream.Position < BaseStream.Lengthonly proves at least one byte remains. With a single byte past_br, the unconditional block reads_targetMean/_targetStd/per-lag scalers part-way and leaves_inputMeanshalf restored. Write aboolbefore TiDE normalization data, and branch on it inDeserializeCorebefore reading the doubles.🤖 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/TiDEModel.cs` around lines 339 - 352, Replace the BaseStream.Position < BaseStream.Length check in DeserializeCore with an explicit serialized boolean indicating whether TiDE normalization state follows _br. Write this flag before the normalization doubles during serialization, then read it and only restore _targetMean, _targetStd, _inputMeans, and _inputStds when true; preserve legacy payload handling as required by the existing format.src/Finance/Portfolio/SharpeRatioPortfolioObjective.cs (1)
108-133: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEnforce the ordering the constant documents, do not assume it.
Lines 55-58 state that the profitable branch "stays well inside +/-100 for every input a return series can produce". The method does not enforce that.
Momentsaccepts any finitedouble[]. A profitable series with a tiny mean and a very large volatility makes-Math.Log(mean) + Math.Log(safeVolatility)exceedLosingPortfolioPenalty, and the losing portfolio then scores better than the profitable one — the exact inversion this change set out to remove.The bound is one line. Clamp the profitable branch so the invariant holds by construction.
🛡️ Proposed fix
- return -Math.Log(mean) + Math.Log(safeVolatility); + // Clamped so the documented ordering is an INVARIANT, not an assumption about input scale: + // no profitable portfolio can reach the penalty a losing one starts at. + return Math.Min( + LosingPortfolioPenalty - double.Epsilon, + -Math.Log(mean) + Math.Log(safeVolatility));Then update the remarks on lines 55-58 to describe the clamp rather than an assumed input range.
🤖 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/Finance/Portfolio/SharpeRatioPortfolioObjective.cs` around lines 108 - 133, Clamp the profitable result returned by Loss so it cannot reach or exceed LosingPortfolioPenalty, preserving the invariant that every profitable portfolio scores better than any losing portfolio. Update the remarks describing the profitable branch to state that this ordering is guaranteed by the result clamp rather than by an assumed input range.src/Finance/Portfolio/SignatureInformedTransformer.cs (1)
250-262: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBLOCKING: the parameter-count guard was applied to only one of the two portfolio models.
src/Finance/Portfolio/GraphAttentionPortfolio.cslines 261-273 now validate the total parameter length before any layer is mutated. This method still slices sequentially with no check. A short vector throws partway through and leaves the earlier layers updated while the rest keep their old values. A long vector leaves its tail silently unused. That is the identical defect, in the identical method, in the sibling model.The prior consolidated review covered both sites. Only the anchor was fixed. Half-applied fixes are worse than none, because the next reader assumes the class is covered.
🛡️ Proposed fix, matching the sibling
Guard.NotNull(parameters); + // The whole length is checked BEFORE any layer is mutated, matching + // GraphAttentionPortfolio.UpdateParameters. Slicing sequentially left the model in a state + // that was neither the old one nor the new one when the vector was short. + int expected = 0; + foreach (var layer in Layers) expected += checked((int)layer.ParameterCount); + + if (parameters.Length != expected) + { + throw new ArgumentException( + $"Expected {expected} parameters for {Layers.Count} layers; got {parameters.Length}.", + nameof(parameters)); + } + int offset = 0; foreach (var layer in Layers) { - var layerParams = layer.GetParameters(); - layer.SetParameters(parameters.Slice(offset, layerParams.Length)); - offset += layerParams.Length; + int count = checked((int)layer.ParameterCount); + layer.SetParameters(parameters.Slice(offset, count)); + offset += count; }As per path instructions: "Incomplete features: Half-implemented patterns where some code paths work but others silently do nothing".
🤖 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/Finance/Portfolio/SignatureInformedTransformer.cs` around lines 250 - 262, Update SignatureInformedTransformer.UpdateParameters to validate that parameters.Length exactly matches the total parameter count across all Layers before mutating any layer. Reuse the sibling GraphAttentionPortfolio validation pattern, ensuring short vectors fail before partial updates and long vectors are not silently ignored.Source: Path instructions
♻️ Duplicate comments (2)
src/CausalDiscovery/ContinuousOptimization/DAGMALinear.cs (1)
179-192: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftUse
MaxIterationsas the outer-iteration cap.
CausalDiscoveryOptions.MaxIterationsdocuments a maximum number of outer iterations. Both DAGMA implementations keep the outer schedule fixed at five steps and instead use this option to cap Adam iterations. A value of100can therefore run up to 500 inner steps.Use
MaxIterationsto cap the central-path loop. UseInnerIterationsfor the per-step Adam budget.
src/CausalDiscovery/ContinuousOptimization/DAGMALinear.cs#L179-L192: capTwithMaxIterationsand derivemaxInnerfromInnerIterations.src/CausalDiscovery/ContinuousOptimization/DAGMANonlinear.cs#L143-L149: cap the outertloop withMaxIterationsand derivemaxInnerfromInnerIterations.🤖 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/CausalDiscovery/ContinuousOptimization/DAGMALinear.cs` around lines 179 - 192, Use MaxIterations as the cap for the central-path outer loop in DAGMALinear.cs lines 179-192 and DAGMANonlinear.cs lines 143-149, limiting T or the loop bound accordingly. Use InnerIterations, rather than MaxIterations, to derive each step’s maxInner Adam budget while preserving the existing per-step defaults and schedule.src/Finance/Trading/Factors/Stockformer.cs (1)
456-467: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftBLOCKING: this replaces a crash with a permanent dead end, it does not fix the state.
The guard is an improvement over the
NullReferenceException. It is still a symptom patch. Trace it:
_encoderis null because deserialization restoredLayersthrough the base class.- Line 461 calls
InitializeLayers().InitializeLayersreturns immediately at line 373 becauseLayers.Count > 0._encoderis still null, and line 463 throws.So a deserialized
Stockformer<T>now throwsInvalidOperationExceptionfrom every forward path instead ofNullReferenceExceptionfrom every forward path, and the message tells the user to "rebuild the model from its options" — which means save and load do not work for this model at all. That is a broken feature documented as a diagnostic.The root cause is that
InitializeLayerskeys idempotency offLayers.Countwhile the encoder is the thing that must exist. Key it off_encoder.🐛 Proposed fix at the root cause, `InitializeLayers` line 373
- // Idempotent: NeuralNetworkBase may also drive this, and appending twice would double every - // parameter and silently break the flat parameter contract. - if (Layers.Count > 0) return; + // Idempotent, keyed off the ENCODER rather than off Layers.Count. The dual-band routing lives + // in _encoder, and a restored Layers collection carries the weights but not the routing, so + // keying off Layers.Count left a deserialized model permanently unable to run a forward pass. + if (_encoder is not null) return; + + // Rebuild the routing, then restore the weights the base class already loaded. + var restored = Layers.Count > 0 ? new Vector<T>(GetParameters()) : null; + Layers.Clear();Then reapply
restoredthroughUpdateParametersat the end of the method when it is not null. That gives a deserialized model working routing AND its trained weights.The
Encoderproperty guard should stay as a last-resort assertion after that change.As per path instructions: "Incomplete features: Half-implemented patterns where some code paths work but others silently do nothing".
🤖 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/Finance/Trading/Factors/Stockformer.cs` around lines 456 - 467, Update InitializeLayers so its early-return/idempotency check is based on _encoder being initialized, not Layers.Count. Ensure deserialized or cloned instances rebuild the encoder even when Layers already contains restored entries, then reapply any non-null restored parameters through UpdateParameters at the end of initialization so routing and trained weights are preserved. Keep the Encoder property’s InvalidOperationException guard as a last-resort assertion.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/CausalDiscovery/ContinuousOptimization/DAGMALinear.cs`:
- Around line 293-308: Update the optimization retry logic around the Adam step
and the surrounding solve loop: after undoing the rejected step, repeatedly
halve learningRate and apply a replacement step, validating each attempt with
InvertMatrix and HasNegativeEntry. Continue retrying from the last valid weights
until validation succeeds; if learningRate reaches the minimum threshold, stop
the solve as unconverged rather than proceeding with invalid weights.
In `@src/CausalDiscovery/DeepLearning/DECIAlgorithm.cs`:
- Around line 332-341: Remove the leftover strength-only candidates.Sort call
after the total-order comparator in the candidate ordering logic. Keep the
preceding comparator that orders by Strength descending, then From, then To, so
the greedy projection loop receives deterministic tie ordering.
In `@src/CausalDiscovery/Functional/ICALiNGAMAlgorithm.cs`:
- Around line 82-88: Update the input guard in ICALiNGAMAlgorithm to reject n ==
1 as well as n == 0, returning the existing empty d-by-d matrix for insufficient
samples while preserving the d < 2 behavior.
In `@src/Finance/Portfolio/AssetGraphBuilder.cs`:
- Around line 235-248: Update the centred-distance caching in the outer asset
loop to process columns in bounded blocks instead of retaining all assets’
matrices. Add the private helper MaxResidentCentredMatrices(int steps), using a
documented memory-budget constant to derive the block size, and compute pair
correlations while each block’s centred matrices is resident. Preserve the
existing diagonal values and DistanceCorrelationFromCentred calculations while
ensuring memory scales with the block size rather than assets.
In `@src/Finance/Portfolio/GraphAttentionPortfolio.cs`:
- Around line 302-305: The clone paths in GraphAttentionPortfolio.cs lines
302-305 and SignatureInformedTransformer.cs lines 288-291 must deep-copy
Architecture, including each NeuralNetworkArchitecture<T>.Layers ILater<T>
instance, before constructing the clone. Preserve the existing architecture and
LossFunction values while ensuring cloned models never share live layers with
their source; both sites require the same deep-copy change.
In `@src/Finance/Trading/Factors/FactorVAE.cs`:
- Around line 851-855: Update FactorVAE deserialization so restored KlWeight,
UseAMSGrad, and Seed affect live behavior: rebuild the internally owned
_optimizer and _random after restoring values, while preserving caller-supplied
optimizers via an ownership flag, or make dependents read dynamically from
_options. Add a format-version byte before the new serialized block and
conditionally read the added fields so older payloads are not over-read.
In `@src/Finance/Trading/Factors/Stockformer.cs`:
- Around line 691-705: Update the validation around restored in the PredictBands
flow to handle non-rank-2 output separately from a restoredTime versus time
mismatch. Report the unexpected restored shape directly instead of treating it
as length 0, and include the input parameter name via the appropriate
nameof-equivalent in the argument error so callers know which window to correct.
Keep the existing length-mismatch guidance only for valid rank-2 output.
In `@src/Models/Options/CausalDiscoveryOptions.cs`:
- Around line 270-286: Complete the XML documentation for
DirectionalityAsymmetryThreshold by adding a <value> element describing its null
default and valid 0–1 range, plus a <para><b>For Beginners:</b> explanation of
how increasing or decreasing the threshold changes directional evidence
requirements.
In `@src/TimeSeries/ProphetModel.cs`:
- Around line 932-935: Remove the trailing _prophetOptions.RegressorCount write
from SerializeCore, leaving only the existing earlier write so the serialized
fields remain aligned with DeserializeCore.
---
Outside diff comments:
In `@src/CausalDiscovery/ContinuousOptimization/MCSLAlgorithm.cs`:
- Around line 64-74: Validate options.MaxPenalty in the MCSLAlgorithm
constructor before assigning _rhoMax, rejecting zero, negative, and NaN values
with the same guard and behavior used by NOTEARSLowRank. Preserve the existing
assignment for valid values and the default ceiling when MaxPenalty is unset.
In `@src/CausalDiscovery/ContinuousOptimization/NOTEARSLowRank.cs`:
- Around line 268-290: Update the optimization flow around IsFiniteVector,
IsFiniteMatrix, and the non-finite hVal check to emit
System.Diagnostics.Trace.TraceWarning diagnostics whenever a candidate step is
rejected or the outer loop terminates due to a non-finite constraint. Include
enough context to distinguish the rejected optimizer result from the invalid
outer constraint, while preserving the existing factor-retention and loop-break
behavior.
In `@src/CausalDiscovery/DeepLearning/DECIAlgorithm.cs`:
- Around line 262-272: Replace the hardcoded 0.1 cutoff in the posterior
edge-selection logic within DECIAlgorithm with the configured EdgeThreshold,
matching the fallback path’s comparison while preserving the existing weight
calculation and edge assignment behavior.
In `@src/CausalDiscovery/Functional/ICALiNGAMAlgorithm.cs`:
- Around line 204-212: Remove the unused identity-plus-perturbation
initialization loop for W in the ICA algorithm, leaving W allocation intact for
later deflation writes. Re-check the seeded RNG usage and ICA-LiNGAM integration
expectations after removal, since eliminating the d*d draws changes the random
starting vectors used by the deflation loop.
- Around line 275-343: Add a focused test covering FindRowPermutation with a
known square unmixing matrix whose optimal assignment is unambiguous, and assert
the exact recovered rowForColumn permutation. Exercise the solver through the
owning ICALiNGAMAlgorithm API or an appropriate test-access mechanism, keeping
the test targeted to assignment/bookkeeping correctness rather than duplicating
the algorithm.
In `@src/CausalInference/DoublyRobustEstimator.cs`:
- Around line 441-448: Update PredictOutcome to validate the feature schema
before iterating rows: require x.Columns to equal NumFeatures and
coefficients.Length to equal NumFeatures + 1, rejecting incompatible inputs
rather than truncating or ignoring columns. Preserve the existing
intercept-plus-feature prediction logic only after both validations pass.
In `@src/Finance/Portfolio/SharpeRatioPortfolioObjective.cs`:
- Around line 108-133: Clamp the profitable result returned by Loss so it cannot
reach or exceed LosingPortfolioPenalty, preserving the invariant that every
profitable portfolio scores better than any losing portfolio. Update the remarks
describing the profitable branch to state that this ordering is guaranteed by
the result clamp rather than by an assumed input range.
In `@src/Finance/Portfolio/SignatureInformedTransformer.cs`:
- Around line 250-262: Update SignatureInformedTransformer.UpdateParameters to
validate that parameters.Length exactly matches the total parameter count across
all Layers before mutating any layer. Reuse the sibling GraphAttentionPortfolio
validation pattern, ensuring short vectors fail before partial updates and long
vectors are not silently ignored.
In `@src/Finance/Trading/Factors/StockformerBands.cs`:
- Around line 136-140: Update BandLength to reject input lengths that cannot
support all configured decomposition levels, matching Split’s validation domain
and preventing zero-width results for values such as 1 or insufficient lengths
for high Levels. Preserve the existing halving calculation for valid inputs and
ensure SplitAll receives only supported lengths.
In `@src/Finance/Trading/Factors/StockformerMultiTaskLoss.cs`:
- Around line 155-175: Validate taskLossWeight at the start of
StockformerMultiTaskLoss.Compute, rejecting NaN, positive or negative infinity,
and negative values before calculating regression, classification, or Total.
Preserve the existing default and weighted objective for finite non-negative
weights.
- Around line 99-135: Update CrossEntropy to validate that targets.Length is
greater than zero before processing or dividing the accumulated loss. Throw an
appropriate argument exception for an empty classification batch, while
preserving the existing validation and averaging behavior for non-empty targets.
In `@src/MetaLearning/Algorithms/FeatureWiseTransformation.cs`:
- Around line 103-106: Update FeatureWiseTransformation.Apply to validate
features before accessing features.Length, explicitly rejecting null with the
established argument-validation behavior. Preserve the existing result
allocation and channel-processing logic for non-null vectors.
In `@src/ReinforcementLearning/Agents/TabularActorCriticAgent.cs`:
- Line 192: Update ParameterCount to use a dedicated PolicyEntryCount helper
instead of materializing GetParameters(). Implement PolicyEntryCount alongside
OrderedPolicyEntries by summing each state’s action-count, preserving the same
entry enumeration source so the reported count and parameter vector remain
consistent.
In `@src/TimeSeries/NLinearModel.cs`:
- Around line 247-256: Update DeserializeCore to store the persisted window
length read from the payload, validate it against the instance’s _l before
reading weights, and reject mismatches with a clear deserialization error. Only
proceed with the existing weight and scaler reads when the lengths match.
- Around line 274-275: Update NLinearModel<T>.CreateInstance() to pass the
injected _optimizer into the new NLinearModel<T> so clones and rebuilds preserve
the configured optimizer instead of selecting the default; if optimizer state
must not be shared, create a fresh equivalent optimizer and document that
behavior.
- Around line 59-60: Document the constructor coupling at the optimizer
initialization in NLinearModel<T>: AdamOptimizer receives this before
construction completes, so preserve the current initialization order and avoid
changes that invoke model state during optimizer construction. No code change is
required unless AdamOptimizer’s construction behavior changes.
In `@src/TimeSeries/ProphetModel.cs`:
- Around line 1096-1097: Remove the unused states allocation in TrainCore and
the corresponding final-row assignment near the “Store final state for future
reference” comment, since the matrix is never read or retained. Do not introduce
a replacement field unless final-state retention is explicitly required.
In `@src/TimeSeries/TiDEModel.cs`:
- Around line 222-249: Update the gradient accumulation loop in the training
method around Forward and the non-finite target check to count each row that
contributes gradients, incrementing the count only after a finite target is
accepted. Use this contributing-row count instead of the nominal bs when
computing inv, and remove bs if it becomes unused.
- Around line 339-352: Replace the BaseStream.Position < BaseStream.Length check
in DeserializeCore with an explicit serialized boolean indicating whether TiDE
normalization state follows _br. Write this flag before the normalization
doubles during serialization, then read it and only restore _targetMean,
_targetStd, _inputMeans, and _inputStds when true; preserve legacy payload
handling as required by the existing format.
---
Duplicate comments:
In `@src/CausalDiscovery/ContinuousOptimization/DAGMALinear.cs`:
- Around line 179-192: Use MaxIterations as the cap for the central-path outer
loop in DAGMALinear.cs lines 179-192 and DAGMANonlinear.cs lines 143-149,
limiting T or the loop bound accordingly. Use InnerIterations, rather than
MaxIterations, to derive each step’s maxInner Adam budget while preserving the
existing per-step defaults and schedule.
In `@src/Finance/Trading/Factors/Stockformer.cs`:
- Around line 456-467: Update InitializeLayers so its early-return/idempotency
check is based on _encoder being initialized, not Layers.Count. Ensure
deserialized or cloned instances rebuild the encoder even when Layers already
contains restored entries, then reapply any non-null restored parameters through
UpdateParameters at the end of initialization so routing and trained weights are
preserved. Keep the Encoder property’s InvalidOperationException guard as a
last-resort assertion.
🪄 Autofix
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 Plus
Run ID: cfdc9724-2a0c-491c-809b-cdec1e1593f9
📒 Files selected for processing (38)
src/AnomalyDetection/DistanceBased/COFDetector.cssrc/CausalDiscovery/Bayesian/DiBSAlgorithm.cssrc/CausalDiscovery/CausalDiscoveryBase.cssrc/CausalDiscovery/ContinuousOptimization/DAGMALinear.cssrc/CausalDiscovery/ContinuousOptimization/DAGMANonlinear.cssrc/CausalDiscovery/ContinuousOptimization/MCSLAlgorithm.cssrc/CausalDiscovery/ContinuousOptimization/NOTEARSLinear.cssrc/CausalDiscovery/ContinuousOptimization/NOTEARSLowRank.cssrc/CausalDiscovery/DeepLearning/DECIAlgorithm.cssrc/CausalDiscovery/DeepLearning/DeepCausalBase.cssrc/CausalDiscovery/Functional/ICALiNGAMAlgorithm.cssrc/CausalDiscovery/Functional/RCDAlgorithm.cssrc/CausalDiscovery/Hybrid/MMHCAlgorithm.cssrc/CausalDiscovery/ScoreBased/BOSSAlgorithm.cssrc/CausalDiscovery/TimeSeries/NeuralGrangerAlgorithm.cssrc/CausalInference/DoublyRobustEstimator.cssrc/CausalInference/TLearner.cssrc/Finance/Portfolio/AssetGraphBuilder.cssrc/Finance/Portfolio/CVaRPortfolioObjective.cssrc/Finance/Portfolio/GraphAttentionLayerCore.cssrc/Finance/Portfolio/GraphAttentionPortfolio.cssrc/Finance/Portfolio/PathSignatureTransform.cssrc/Finance/Portfolio/SharpeRatioPortfolioObjective.cssrc/Finance/Portfolio/SignatureAugmentedAttention.cssrc/Finance/Portfolio/SignatureInformedTransformer.cssrc/Finance/Trading/Factors/FactorVAE.cssrc/Finance/Trading/Factors/Stockformer.cssrc/Finance/Trading/Factors/StockformerBands.cssrc/Finance/Trading/Factors/StockformerDualEncoder.cssrc/Finance/Trading/Factors/StockformerMultiTaskLoss.cssrc/MetaLearning/Algorithms/FeatureWiseTransformation.cssrc/Models/Options/CausalDiscoveryOptions.cssrc/ReinforcementLearning/Agents/TabularActorCriticAgent.cssrc/ReinforcementLearning/Policies/BetaPolicy.cssrc/SurvivalAnalysis/NelsonAalenEstimator.cssrc/TimeSeries/NLinearModel.cssrc/TimeSeries/ProphetModel.cssrc/TimeSeries/TiDEModel.cs
💤 Files with no reviewable changes (3)
- src/CausalDiscovery/DeepLearning/DeepCausalBase.cs
- src/CausalDiscovery/Hybrid/MMHCAlgorithm.cs
- src/CausalDiscovery/Bayesian/DiBSAlgorithm.cs
…ks rejected RCDAlgorithm computed the same direction evidence twice per round. Candidate selection walked `remaining` accumulating sum of min(0, DiffMI)^2, and ConfoundingRatio then walked the same list for the selected candidate and recomputed every one of those DiffMutualInfo calls. The wrong-way-evidence formula also lived in two places that had to be edited together to stay correct. Both now come from one DirectionEvidence helper returning both accumulators, and the selection loop keeps the winner's values for the stop test. FactorVAE.DecodeReturns checked only for rank 1 and treated alpha.Shape[0] as the whole batch otherwise. A rank-3 feature tensor such as [batch, sequence, assets] therefore collapsed its sequence axis into the batch: the beta reshape asked for batch * assets * factors elements out of a buffer holding batch * sequence * assets * factors, and reshaped the wrong count silently. AlignReturns had the matching hole, forcing mismatched returns to rank 2 while features could be rank 3, so Engine.Concat could join tensors of different ranks. Both now reject ranks above 2 and say how to fix the input. ContinuousOptimizationBase.EnforceAcyclic documents its cost and the variable count it is sized for, in answer to the review question: the projection is quartic, invisible at the tens of variables NOTEARS, DAGMA and MCSL are used at (their own inner solves are already O(d^3) per iteration), and dominant by a few hundred -- where a union-find over a topological order or an incremental reachability matrix is the fix. Verified: build error count unchanged at the branch baseline of 92, no new diagnostics. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ta does not support PNLAlgorithm wrote a single directed edge W[i, j] in both of its unresolved-direction branches. The comment called index order an "acyclic tie-break", and the acyclicity claim is right -- the loop runs over i < j -- but the OUTPUT was the problem: a caller could not tell an edge PNL resolved from the residual asymmetry from one where PNL gave up and used the column layout. Because that fallback follows column order, reordering the caller's columns silently reversed the reported cause and effect. Both branches now write the pair symmetrically, which is what CAMUVAlgorithm already does for a pair it cannot orient (RCDAlgorithm declines the edge entirely). The adjacency is kept; the direction claim is not made. ProphetModel.ComputeEffectiveSeasonalPeriods compared DURATIONS against a row COUNT. SeasonalPeriods are documented in days, the standard seasonalities are durations in days, and FitLeastSquares reads column 0 as the time value -- so the guard only coincided with the right answer for daily-sampled data. Hourly OADate input admitted yearly seasonality after 3000 rows, which is 125 days, and any coarser sampling dropped periods the window did cover. It now takes tMax - tMin and compares like with like. Explicit periods are held to the same two-cycle threshold as the defaults while there. One cycle cannot separate a seasonal term from the trend, so admitting a period the window covers once reports seasonality the data cannot identify. Verified: build error count unchanged at the branch baseline of 92, no new diagnostics. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…-table validation DynaQPlusAgent.GetParameters allocated the real Q-table entry count and then wrote ActionSize values per state. For a ragged table -- the normal state of a tabular agent that has not tried every action -- that wrote PAST the end of the vector. SetParameters had the mirror defect: it looped 0..ActionSize-1, inserting zero-valued entries for actions the agent had never visited and shifting every later value onto the wrong pair. DynaQAgent had the same split: export walked the actual dictionary entries, restore looped ActionSize per state and hid the mismatch behind an idx < parameters.Length guard. Both now walk one ordered (state, action) enumeration for ParameterCount, GetParameters and SetParameters, and restore requires an exact length. Ordering is ordinal by key, because Dictionary guarantees nothing about enumeration order across insertions and a vector written in one order and read back in another is silently wrong rather than loudly broken. DoubleQLearningAgent keeps its full-table contract, so the fix there is at the boundary: Deserialize accepted the two Q-tables independently, and GetParameters sizes its vector from QTable1 while filling from both and indexing every action in 0..ActionSize-1. A state present in one table only overran that vector; a state missing an action threw KeyNotFoundException from inside the flatten. Both tables must now describe the same states with complete action sets, checked as soon as they are read, with a message that names the offending state. Verified: build error count unchanged at the branch baseline of 92, no new diagnostics. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…n hash order LPCMCIAlgorithm's PC1 phase built its conditioning set with others.Take(condSize) off a list derived from HashSet enumeration, whose order is an implementation detail. The discovered graph therefore depended on hash ordering -- identical input could produce different output across runtime versions -- and only one arbitrary subset of each size was ever tested. The comment above it claimed to implement PC1; PC1 in Runge et al. 2019 draws subsets from the STRONGEST remaining parents, which is what makes the removal rule meaningful: conditioning on the strongest competitors is the test a spurious link should fail. Candidates are now ranked by their own unconditional association with the target, strongest first, with a (var, lag) tiebreak so equal strengths still give a total order. `others` inherits that ordering, so Take(condSize) draws the strongest remaining parents. KernelPCADetector cited only Scholkopf 1998, which gives kernel PCA itself. The novelty SCORE this detector reports -- reconstruction error in feature space -- is Hoffmann 2007, and that is the rule its scoring semantics have to be judged against. Added alongside. Verified: build error count unchanged at the branch baseline of 92, no new diagnostics. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ming what is not there KernelPCA_OutlierGetsHighestScore fitted and scored the same 30 rows, including the extreme one, then asserted that row scored highest. KPCA's novelty score is Hoffmann's feature-space reconstruction error, and an in-sample fitted point is reconstructed with near-zero residual whenever its own direction is retained -- so the test expected the documented failure mode to behave as the success path. It now fits on the inliers alone and scores a matrix whose last row is a genuinely new extreme point, which is what the scoring rule is defined for. This is a stricter assertion than before, not a weaker one. KernelPCADetector also cited only Scholkopf 1998, which gives kernel PCA. The novelty score is Hoffmann 2007; that citation is added alongside. RCDAlgorithm's ConfoundingEvidenceCutoff documentation pointed at "the RCD calibration tests, which verify a clean LiNGAM DAG scores well under the cutoff while a latent-confounder structure scores well over it". Those tests do not exist -- what exists is an integration test asserting a non-empty graph on linear data. The documentation now states the real coverage and says plainly that the default is reasoned rather than measured, with a note that ConfoundingRatio is internal precisely so those cases can be asserted directly. Stockformer.ComputeLoss is documented as what it is: a DIAGNOSTIC. It returns plain doubles through ToVector, which severs the tape, and training runs through PredictCore, which returns the fused return head alone -- so the direction head, the low-frequency heads and the classification term receive no gradient. Only the return head is trained. Wiring the multi-task objective into training is the paper's contribution and is still to do; until then a falling total here must not be read as the direction head learning, because nothing updates it. Verified: build error count unchanged at the branch baseline of 92, no new diagnostics. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…er asset per step Both helper tensors depend only on the position count. StockformerAttention.CausalBias allocated and filled a [positions, positions] mask, and StockformerDualEncoder.CausalWindow a [time, time] operator, on every call -- which is every asset, every layer, every training step. CausalBias is cached statically because it is a pure function of the position count. CausalWindow's operator is cached per encoder instance, since it also depends on _kernelWidth, which is per-encoder. Neither tensor is mutated after construction, so one instance serves every call at that size. The static cache takes a lock and keeps whichever instance wins a race; either is equally valid. NOT done here, and left as the open half of this thread: replacing AssetSlice and TimeSlice's one-hot matmuls with a tape-connected slice operation. Those make the encoder forward pass quadratic in inputs before any attention arithmetic runs, but swapping them requires a slice op that provably preserves the gradient tape, and this branch cannot be compiled or run standalone to verify that. That is a change to make with the model runnable. Verified: build error count unchanged at the branch baseline of 92 on both net8.0 and net471, no new diagnostics. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nch's own commits Three were outright bugs introduced here and are the reason for this commit: ProphetModel.SerializeCore wrote RegressorCount TWICE -- once in its original position and again after the residual statistics, left behind when a misplaced edit was repaired. DeserializeCore reads it once, so the trailing four bytes were never consumed and every Prophet payload was corrupted from that offset on. DECIAlgorithm kept a leftover second Sort with a strength-only comparator immediately after the new total-order one, which discarded the deterministic ordering the line above had just established -- reinstating exactly the introsort dependence that commit set out to remove. FactorVAE restored KlWeight, Seed and UseAMSGrad into _options, but _random and _optimizer are built from those values at construction and were readonly, so two of the three were dead on arrival. Both fields are now assignable, _random is rebuilt from the restored seed, and the default optimizer is rebuilt -- only when this instance built its own, since a caller-supplied optimizer carries state the saved model knows nothing about. The rest were correct findings against earlier commits: GraphAttentionPortfolio and SignatureInformedTransformer passed Architecture to their clones, but InitializeLayers adds Architecture.Layers into Layers BY REFERENCE and ILayer<T> has no Clone, so a layer-carrying architecture gave both models the same layer objects. A clone that silently shares trainable state is worse than one that rebuilds defaults, so the architecture now carries across only when it holds no layers. AssetGraphBuilder's centring cache held assets x steps^2 doubles at once -- about 250 MB at 500 assets over 250 steps, against a class documented for ~5,000 firms. Columns are now processed in blocks sized to a 64 MB budget: every pair is still visited once, and each column is centred once per block rather than once per partner. DAGMALinear halved its learning rate on an M-matrix domain violation and applied one replacement step without checking it. If that step was also outside the domain, the next iteration computed a log-determinant on an invalid matrix. It now retries from the last valid weights until the step is accepted, and stops the solve as unconverged at the rate floor. ICALiNGAM's guard admitted n == 1, which is degenerate in the worst way: centring one sample gives exactly zero, so the covariance is the zero matrix, every eigenvalue floors to 1e-12 and the whitening scale becomes 1e6. Now requires d + 3 samples, matching RCDAlgorithm. Stockformer.Restore folded a non-rank-2 filter result into the window-length mismatch, whose message then claimed the filter "restores to 0, which is the configured SequenceLength" and advised passing a window of 0 steps. SequenceLength is validated positive, so both were false; the two failures are now reported separately. DirectionalityAsymmetryThreshold gained the <value> element and the For Beginners explanation the options convention requires. Verified: build error count unchanged at the branch baseline of 92 on both net8.0 and net471. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both sides had independently made KernelPCA_OutlierGetsHighestScore a novelty test rather than an in-sample one, so the conflict was two spellings of the same fix. Kept CreateCleanTrainingData -- the name the other two call sites and the integration base already use -- and deleted the duplicate CreateInliersOnly generator, which had drifted back to an inline copy of the three feature formulas. That is exactly the duplication FillNormalRow's own remarks argue against: two generators of "the 29 normal rows" with nothing holding them in step. Kept this slice's structural change: score a separate matrix (scored) so the fitted set and the scored set are visibly different objects, rather than fitting on one and silently reusing the name for the other. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… premise Two defects the merge created that only a build could find; neither side has them alone. CausalDiscoveryOptions.DirectionalityAsymmetryThreshold was declared on both sides (CS0102), with docs that disagree about what the number means. Kept the integration copy, whose formula matches CCMAlgorithm: the asymmetry is RELATIVE, |f - b| / (max(f, b) + eps). The copy removed here described an absolute gap -- "the winning direction must beat the other by 0.2" -- which is a different rule. Skills of 0.5 and 0.4 clear the real threshold and fail the documented one. Kept the fuller beginner explanation, corrected to describe the relative margin and to say which pairs each rule actually orients. FeatureWiseTransformation was narrowed to internal here on the grounds that nothing exposed it. That was true only because LFTAlgorithm lives in another slice and was not on this branch; it returns the type from a public property, so the two halves meeting produced CS0053. Restored to public with a note saying why it cannot be narrowed again without changing the property first. Verified: no CS0102 and no CS0053 remain, and every error kind left in the merge is one integration/1789 already has on its own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
Slice 17 of 18 splitting #1789 into review-sized PRs.
#1789 reached 1,720 changed files, so it has never received an automated review. CodeRabbit caps reviews at 100 changed files and rate-limits, so the split is packed close to the cap: 18 slices of 95-96 files each, which is the minimum PR count possible.
Scope: Finance + TimeSeries + CausalDiscovery + CausalInference + AnomalyDetection + SurvivalAnalysis + ReinforcementLearning + MetaLearning
Files: 95
Why an integration branch
These slices target
integration/1789, notmaster. The test generator and the model fixes are coupled in both directions - the generator's smoke constructors are what surfaced the model bugs, and itsFp32/HeavyTimeoutmitigation lists only make sense once those models are fixed. Making each slice independently green againstmasterwould mean re-verifying the full shard matrix 18 times with red intermediate states. Each slice is reviewed at reviewable size;integration/1789is what must be green before it merges tomaster.Integrity
The 18 slices were cut by file scope from a single snapshot and verified mechanically: their union is exactly the 1,720 files, with zero overlaps and zero gaps, and every slice is byte-identical to the snapshot for the files it carries.
Individual slices do not build standalone
Expected under this model - the slices are cut by scope, not by compilability, and cross-reference each other.
integration/1789is the unit that must build and go green.Related: #1789
Summary by CodeRabbit