Repository navigation
feat: physics-informed neural networks and advanced algebra and geometry support - #449
Conversation
|
Warning Rate limit exceeded@ooples has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 14 minutes and 54 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a large Physics‑Informed ML subsystem: PDE interfaces and base class, many PDE implementations with analytic residuals/gradients, PINN/DeepRitz/Variational solvers, neural operators (DeepONet, FNO, GNO), advanced algebra/manifolds/sparse types and engines, specialized layers/networks, autodiff and finite‑difference derivatives, JIT IR ops, benchmarks, tests, and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PINN as PhysicsInformedNeuralNetwork<T>
participant Network as NeuralNetworkBase<T>
participant AutoDiff as NeuralNetworkDerivatives<T>
participant PDE as IPDESpecification<T>
participant Optimizer as IGradientBasedOptimizer<T>
User->>PINN: Solve(data?, epochs)
PINN->>PINN: Generate collocation points
loop per collocation/batch
PINN->>Network: Forward(point)
Network->>AutoDiff: Request derivatives
AutoDiff-->>Network: derivatives (analytic or FD)
Network-->>PDE: PDEDerivatives
PDE->>PDE: ComputeResidual(...)
PDE-->>PINN: residual
end
PINN->>PINN: Aggregate loss (data + pde + boundary + initial)
PINN->>Network: Backward(total loss)
Network-->>Optimizer: Gradients
Optimizer->>Network: Update parameters
PINN-->>User: Return TrainingHistory
sequenceDiagram
participant User
participant DeepONet as DeepOperatorNetwork<T>
participant Branch as BranchNet
participant Trunk as TrunkNet
participant Optimizer as Optimizer
User->>DeepONet: EvaluateMultiple(inputFunction, queryLocations)
DeepONet->>Branch: Forward(inputFunction)
Branch-->>DeepONet: coefficients
loop each queryLocation
DeepONet->>Trunk: Forward(queryLocation)
Trunk-->>DeepONet: basis
DeepONet->>DeepONet: Dot(coefficients, basis)
DeepONet-->>User: output
end
Note right of Optimizer: Training backpropagates through both Branch and Trunk networks
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a comprehensive Physics-Informed Machine Learning framework with implementations of Physics-Informed Neural Networks (PINNs), Neural Operators, and Scientific ML methods for solving partial differential equations and learning physical systems.
- Adds core infrastructure for PINNs including interfaces, automatic differentiation, and loss functions
- Implements multiple PDE solvers (Heat, Wave, Poisson, Burgers equations)
- Adds advanced neural operators (FNO, DeepONet, Graph Neural Operators) for learning function-to-function mappings
- Includes physics-aware architectures (Hamiltonian/Lagrangian Neural Networks, Universal Differential Equations)
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| IPDESpecification.cs | Defines interfaces for PDE specifications, boundary conditions, initial conditions, and derivative structures |
| AutomaticDifferentiation.cs | Implements finite difference-based automatic differentiation for computing gradients and Hessians |
| HeatEquation.cs | Implements the heat/diffusion equation PDE specification |
| BurgersEquation.cs | Implements Burgers' equation with nonlinear convection and diffusion |
| PoissonEquation.cs | Implements Poisson/Laplace equation for steady-state problems |
| WaveEquation.cs | Implements the wave equation for oscillatory phenomena |
| PhysicsInformedLoss.cs | Combines data loss, PDE residual, boundary conditions, and initial conditions |
| PhysicsInformedNeuralNetwork.cs | Main PINN implementation with collocation point sampling and training |
| VariationalPINN.cs | Variational formulation using weak form of PDEs |
| DeepRitzMethod.cs | Energy minimization approach for solving variational problems |
| FourierNeuralOperator.cs | Implements FNO for learning operators in Fourier space |
| DeepOperatorNetwork.cs | Implements DeepONet with branch-trunk architecture |
| GraphNeuralOperator.cs | Neural operators for graph-structured domains |
| HamiltonianNeuralNetwork.cs | Physics-aware network preserving Hamiltonian structure |
| LagrangianNeuralNetwork.cs | Network using Lagrangian mechanics formulation |
| UniversalDifferentialEquations.cs | Combines known physics with learned neural network components |
| SymbolicPhysicsLearner.cs | Symbolic regression for discovering interpretable equations |
Comments suppressed due to low confidence (1)
src/PhysicsInformed/NeuralOperators/DeepOperatorNetwork.cs:1
- The code references
FeedForwardNeuralNetwork<T>which is not imported or defined in the visible scope. This will cause a compilation error. Ensure the proper using statement or namespace is added, or use the fully qualified type name.
using System;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
🤖 PR Title Auto-Fixed Your PR title was automatically updated to follow Conventional Commits format. Original title: New title: Detected type: Valid types and their effects:
If the detected type is incorrect, you can manually edit the PR title. |
🤖 Commit Messages Auto-FixedThe commitlint check failed because one or more commit messages didn't follow Conventional Commits format. Action taken — All non-compliant commits have been fixed to follow the conventional commits format. Changes made:
The PR branch has been force-pushed with the fixed commits. If you had local changes, you may need to |
82574a9 to
7865ca8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
♻️ Duplicate comments (9)
src/PhysicsInformed/ScientificML/LagrangianNeuralNetwork.cs (1)
119-131: Critical: ComputeAcceleration returns zeros instead of computing actual accelerations.This method is non-functional and makes the Lagrangian Neural Network unable to produce dynamics. The placeholder implementation returns a zero-initialized array rather than applying the Euler-Lagrange equations: d/dt(∂L/∂q̇) - ∂L/∂q = 0.
Note: Past reviews have already provided two detailed implementation suggestions using finite differences. Please verify whether automatic differentiation infrastructure exists in the codebase to support a proper implementation, or if the finite-difference approach from previous reviews should be adopted.
Run the following script to check for automatic differentiation capabilities:
#!/bin/bash # Search for automatic differentiation infrastructure in the codebase echo "Searching for AD/gradient computation methods..." rg -n --type=cs -C3 'ComputeGradient|AutomaticDifferentiation|Jacobian|BackwardDiff|ForwardDiff' echo -e "\nSearching for existing differentiation in PhysicsInformed namespace..." rg -n --type=cs -C3 'class.*Differentiation|interface.*Differentiation' src/PhysicsInformed/ echo -e "\nSearching for any finite difference implementations..." rg -n --type=cs -C3 'FiniteDifference|CentralDifference' src/src/PhysicsInformed/NeuralOperators/FourierNeuralOperator.cs (2)
125-131: Nullable parameter with null default could be clearer.The
spatialDimensionsparameter defaults tonullbut is then assigned a non-null default in the constructor body. Consider usingint[]?type annotation for clarity, or providing the default array directly in the parameter declaration.
345-359:Forwardis a placeholder returning input unchanged.The
FourierLayer.Forwardmethod returns the input tensor without applying any FFT, spectral convolution, or other transformations. This means the FNO will not perform any Fourier-based operations. The method should either be implemented or throwNotImplementedExceptionto make the incomplete state explicit.src/PhysicsInformed/PDEs/PoissonEquation.cs (1)
87-89: The_sourceFunction == nullcheck is always false.As noted in a previous review, the constructor assigns a default lambda when
sourceFunctionis null, so this condition never evaluates to true. TheNameproperty will always return "Poisson Equation" even for Laplace's equation.src/PhysicsInformed/ScientificML/UniversalDifferentialEquations.cs (1)
56-56: Base constructor receives null loss function.As noted in a previous review, passing
nullfor the loss function may cause issues if the base class expects a valid loss function.src/PhysicsInformed/PINNs/DeepRitzMethod.cs (1)
297-316: Training loop does not update parameters — method is non-functional.The
Solvemethod computes energy each epoch but never performs gradient computation or parameter updates. ThelearningRateparameter is unused. This renders training non-functional.As noted in a previous review, consider either implementing the optimization step or marking this as a placeholder with
NotImplementedException.src/PhysicsInformed/PhysicsInformedLoss.cs (1)
92-92:ComputeLosssignature does not matchILossFunction<T>interface.The method signature
ComputeLoss(T[] predictions, T[]? targets, PDEDerivatives<T> derivatives, T[] inputs)differs from the typicalILossFunction<T>interface which expectsComputeLoss(T[] predictions, T[] targets). This overload won't satisfy the interface contract.Consider adding an explicit interface implementation or clarifying the relationship between this method and the interface.
src/PhysicsInformed/PINNs/VariationalPINN.cs (1)
301-326: Training loop does not update parameters — method is non-functional.The
Solvemethod computes residuals each epoch but never performs gradient computation or parameter updates. ThelearningRateparameter is unused.As noted in a previous review, consider either implementing the optimization step or marking this as a placeholder.
src/PhysicsInformed/PINNs/PhysicsInformedNeuralNetwork.cs (1)
269-353: Critical: Training loop computes losses but never updates network weights.The
Solvemethod iterates over epochs and collocation points, computing losses, but there is no backpropagation or weight update step. The_optimizerfield created in the constructor is never invoked. Without gradient computation and parameter updates, the network cannot learn to satisfy the PDE.A complete training loop requires:
- Compute gradients of the loss with respect to network parameters
- Apply the optimizer to update weights (e.g.,
_optimizer.Step(gradients, learningRate))🔎 Conceptual fix outline
T loss = _physicsLoss.ComputeLoss(output, null, derivatives, point); totalLoss += loss; numBatches++; - // Note: In a real implementation, you'd accumulate gradients and update in batches - // This simplified version updates after each point + // Backpropagate and accumulate gradients + // var gradients = ComputeGradients(loss); + // accumulatedGradients.Add(gradients); } + // Update parameters after batch + // _optimizer.Step(accumulatedGradients, T.CreateChecked(learningRate)); }The comment on line 308-309 acknowledges this is incomplete. If this is intentionally a placeholder, consider throwing
NotImplementedExceptionor clearly marking the class as experimental.
🧹 Nitpick comments (15)
src/PhysicsInformed/ScientificML/SymbolicPhysicsLearner.cs (3)
104-112: Document this as a stub implementation.The
Simplifymethod promises algebraic simplification but simply returns the input unchanged. Users may expect actual simplification rules to be applied. Consider adding a remark similar to theDiscoverEquationdocumentation to indicate this is a placeholder.🔎 Suggested documentation enhancement
/// <summary> -/// Simplifies an expression using symbolic algebra rules. +/// [STUB] Simplifies an expression using symbolic algebra rules. /// </summary> +/// <remarks> +/// <b>Note:</b> This method is a stub implementation and does not perform actual simplification. +/// It currently returns the input expression unchanged. A full implementation would apply +/// algebraic rules such as x + 0 = x, x * 1 = x, x * 0 = 0, etc. +/// </remarks> public SymbolicExpression<T> Simplify(SymbolicExpression<T> expression) { - // Apply algebraic simplification rules: - // x + 0 = x, x * 1 = x, x * 0 = 0, etc. + // STUB: This is a placeholder implementation. + // A full implementation would apply algebraic simplification rules. return expression; }
114-120: Method name implies LaTeX formatting but delivers plain text.The
ToLatexmethod name suggests LaTeX-formatted output (e.g.,\frac{x}{y},x^2), but it simply callsToString(). This may confuse users expecting formatted output for scientific publications. Consider documenting this as a stub or renaming to reflect actual behavior.🔎 Suggested documentation fix
/// <summary> -/// Converts expression to human-readable string. +/// [STUB] Converts expression to LaTeX format. /// </summary> +/// <remarks> +/// <b>Note:</b> This method is a stub and currently returns plain text via ToString(). +/// A full implementation would produce properly formatted LaTeX output (e.g., \frac{x}{y}, x^2, etc.). +/// </remarks> public string ToLatex(SymbolicExpression<T> expression) { + // STUB: This placeholder returns plain text. A full implementation would format as LaTeX. return expression.ToString(); }
128-129: Consider making properties init-only for immutability.The public setters on
ExpressionandTypeallow mutation after construction, which could lead to inconsistent state. Since symbolic expressions are typically immutable once constructed, consider usinginitaccessors instead.🔎 Suggested immutability improvement
-public string Expression { get; set; } -public SymbolicExpressionType Type { get; set; } +public string Expression { get; init; } +public SymbolicExpressionType Type { get; init; }src/PhysicsInformed/ScientificML/LagrangianNeuralNetwork.cs (1)
97-114: ComputeLagrangian implementation is correct.The method properly concatenates position and velocity, creates the input tensor, and returns the scalar Lagrangian output.
Optional: Extract state concatenation to reduce duplication
The state concatenation pattern is repeated in both
ComputeLagrangianandComputeAcceleration. Consider extracting to a helper method:+ private T[] ConcatenateState(T[] q, T[] qDot) + { + T[] state = new T[2 * _configurationDim]; + Array.Copy(q, 0, state, 0, _configurationDim); + Array.Copy(qDot, 0, state, _configurationDim, _configurationDim); + return state; + } + public T ComputeLagrangian(T[] q, T[] qDot) { - T[] state = new T[2 * _configurationDim]; - Array.Copy(q, 0, state, 0, _configurationDim); - Array.Copy(qDot, 0, state, _configurationDim, _configurationDim); + T[] state = ConcatenateState(q, qDot);src/PhysicsInformed/NeuralOperators/FourierNeuralOperator.cs (1)
328-343: Spectral weights initialized but never used.
_spectralWeightsis initialized with random values but is never referenced inForwardorBackward. Additionally, using a hardcoded seed (Random(42)) prevents weight variability across instances, which may limit training diversity if multiple FourierLayers are intended to learn different representations.src/PhysicsInformed/NeuralOperators/DeepOperatorNetwork.cs (1)
103-108: Consider whether NeuralNetworkBase inheritance is appropriate.DeepONet's architecture differs significantly from traditional feedforward networks—it uses separate branch and trunk networks rather than a sequential layer structure. The empty
InitializeLayers()override (line 169-173) indicates that the base class's layer abstraction isn't utilized.While inheriting from
NeuralNetworkBase<T>may provide access to shared utilities, consider whether composition (containing neural networks as components) would be a clearer design than inheritance. This would make the architectural difference more explicit and avoid inheriting unused abstractions.Also applies to: 169-173
src/PhysicsInformed/ScientificML/UniversalDifferentialEquations.cs (2)
101-104: Consider validatingstatelength inComputeDerivative.Similar to
Simulate, validating thatstate.Length == _stateDimwould provide clearer error messages than downstream index errors.
155-161: Euler integration may accumulate significant error for stiff or long-duration simulations.The current first-order Euler method is simple but can be inaccurate. Consider documenting this limitation or offering an optional higher-order method (e.g., RK4) for users who need better accuracy.
src/PhysicsInformed/AutomaticDifferentiation.cs (2)
7-37: Class name is misleading: this implements numerical differentiation, not automatic differentiation.The documentation correctly explains automatic differentiation (AD) and notes that this implementation uses finite differences (line 49), but the class name
AutomaticDifferentiationis misleading since the actual implementation uses numerical differentiation. Consider renaming toNumericalDifferentiationorFiniteDifferenceHelperto accurately reflect the implementation.
128-140: Redundant function evaluations for diagonal second derivatives.When
i == k, the code computesinputsPlus[i] += handinputsMinus[i] -= hand callsnetworkFunctionagain — these are the same evaluations already performed in the first derivatives loop (lines 82-89). Consider cachingoutputPlusandoutputMinusfrom the first derivatives computation to avoid redundant function calls.🔎 Proposed optimization
Cache the forward evaluations during first derivative computation:
+ // Cache for reuse in second derivative computation + T[][] cachedOutputPlus = new T[inputDim][]; + T[][] cachedOutputMinus = new T[inputDim][]; + // Compute first derivatives using central differences for (int i = 0; i < inputDim; i++) { T[] inputsPlus = (T[])inputs.Clone(); T[] inputsMinus = (T[])inputs.Clone(); inputsPlus[i] += h; inputsMinus[i] -= h; T[] outputPlus = networkFunction(inputsPlus); T[] outputMinus = networkFunction(inputsMinus); + cachedOutputPlus[i] = outputPlus; + cachedOutputMinus[i] = outputMinus; for (int j = 0; j < outputDim; j++) { derivatives.FirstDerivatives[j, i] = (outputPlus[j] - outputMinus[j]) / (T.CreateChecked(2) * h); } }Then reuse in the diagonal case:
if (i == k) { - T[] inputsPlus = (T[])inputs.Clone(); - T[] inputsMinus = (T[])inputs.Clone(); - inputsPlus[i] += h; - inputsMinus[i] -= h; - - T[] outputPlus = networkFunction(inputsPlus); - T[] outputMinus = networkFunction(inputsMinus); + T[] outputPlus = cachedOutputPlus[i]; + T[] outputMinus = cachedOutputMinus[i]; derivatives.SecondDerivatives[j, i, k] = (outputPlus[j] - T.CreateChecked(2) * baseOutput[j] + outputMinus[j]) / (h * h); }src/PhysicsInformed/PINNs/DeepRitzMethod.cs (1)
249-249: Hardcoded boundary penalty weight should be configurable.The penalty weight
100.0is hardcoded. This value significantly affects convergence and solution quality. Consider making it a constructor parameter or property.🔎 Proposed fix
Add a constructor parameter:
public DeepRitzMethod( NeuralNetworkArchitecture<T> architecture, Func<T[], T[], T[,], T> energyFunctional, Func<T[], bool>? boundaryCheck = null, Func<T[], T[]>? boundaryValue = null, - int numQuadraturePoints = 10000) + int numQuadraturePoints = 10000, + T? boundaryPenaltyWeight = null) : base(architecture, null, 1.0) { _energyFunctional = energyFunctional ?? throw new ArgumentNullException(nameof(energyFunctional)); _boundaryCheck = boundaryCheck; _boundaryValue = boundaryValue; + _boundaryPenaltyWeight = boundaryPenaltyWeight ?? T.CreateChecked(100.0); // ... }Then use
_boundaryPenaltyWeightinstead of the hardcoded value.src/PhysicsInformed/PINNs/VariationalPINN.cs (2)
265-277: Limited polynomial basis may be insufficient for complex problems.The multi-index generation only produces polynomials with degrees 0, 1, or 2 per dimension, limiting the basis to
3^dimensionfunctions. For a 2D problem withnumTestFunctions = 10, only 9 unique basis functions exist.Consider:
- Documenting this limitation
- Using higher-degree polynomials or different basis functions (e.g., Legendre, Chebyshev)
- Validating that
numTestFunctions <= 3^dimension
253-260: Analytical gradient is available for polynomial test functions.Using
AutomaticDifferentiation<T>.ComputeGradient(finite differences) for polynomial test functions is inefficient when the analytical derivative can be computed directly from the multi-index. For monomialx^n, the derivative isn * x^(n-1).🔎 Proposed analytical implementation
private T[,] ComputeTestFunctionGradient(T[] point, int index) { int[] multiIndex = GenerateMultiIndex(index, point.Length); T[,] gradient = new T[Architecture.OutputSize, point.Length]; for (int k = 0; k < Architecture.OutputSize; k++) { for (int d = 0; d < point.Length; d++) { if (multiIndex[d] == 0) { gradient[k, d] = T.Zero; } else { // Derivative of product: d/dx_d (x_0^a * x_1^b * ...) = a * x_d^(a-1) * product of others T value = T.CreateChecked(multiIndex[d]); for (int j = 0; j < point.Length; j++) { int power = (j == d) ? multiIndex[j] - 1 : multiIndex[j]; for (int p = 0; p < power; p++) { value *= point[j]; } } gradient[k, d] = value; } } } return gradient; }src/PhysicsInformed/PINNs/PhysicsInformedNeuralNetwork.cs (2)
110-126: Consider validatingnumCollocationPointsandboundaryConditions.Length.The constructor validates null references but doesn't check that
numCollocationPoints > 0or thatboundaryConditionscontains at least one condition. PassingnumCollocationPoints = 0would skip PDE training entirely, and an empty boundary conditions array may be unintended.🔎 Proposed validation
: base(architecture, null, 1.0) { _pdeSpecification = pdeSpecification ?? throw new ArgumentNullException(nameof(pdeSpecification)); _boundaryConditions = boundaryConditions ?? throw new ArgumentNullException(nameof(boundaryConditions)); + if (numCollocationPoints <= 0) + { + throw new ArgumentOutOfRangeException(nameof(numCollocationPoints), "Must be greater than zero."); + } _initialCondition = initialCondition; _numCollocationPoints = numCollocationPoints;
206-222: Fixed seed limits sampling diversity across runs.Using
Random(42)ensures reproducibility, which is useful for debugging. However, users might expect different collocation points on each instantiation. Consider making the seed configurable or documenting this behavior more prominently.🔎 Optional: Allow seed configuration
- private void GenerateCollocationPoints() + private void GenerateCollocationPoints(int? seed = 42) { int inputDim = _pdeSpecification.InputDimension; _collocationPoints = new T[_numCollocationPoints, inputDim]; - var random = new Random(42); // Fixed seed for reproducibility + var random = seed.HasValue ? new Random(seed.Value) : new Random();
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/PhysicsInformed/AutomaticDifferentiation.cs(1 hunks)src/PhysicsInformed/Interfaces/IPDESpecification.cs(1 hunks)src/PhysicsInformed/NeuralOperators/DeepOperatorNetwork.cs(1 hunks)src/PhysicsInformed/NeuralOperators/FourierNeuralOperator.cs(1 hunks)src/PhysicsInformed/NeuralOperators/GraphNeuralOperator.cs(1 hunks)src/PhysicsInformed/PDEs/BurgersEquation.cs(1 hunks)src/PhysicsInformed/PDEs/HeatEquation.cs(1 hunks)src/PhysicsInformed/PDEs/PoissonEquation.cs(1 hunks)src/PhysicsInformed/PDEs/WaveEquation.cs(1 hunks)src/PhysicsInformed/PINNs/DeepRitzMethod.cs(1 hunks)src/PhysicsInformed/PINNs/PhysicsInformedNeuralNetwork.cs(1 hunks)src/PhysicsInformed/PINNs/VariationalPINN.cs(1 hunks)src/PhysicsInformed/PhysicsInformedLoss.cs(1 hunks)src/PhysicsInformed/ScientificML/HamiltonianNeuralNetwork.cs(1 hunks)src/PhysicsInformed/ScientificML/LagrangianNeuralNetwork.cs(1 hunks)src/PhysicsInformed/ScientificML/SymbolicPhysicsLearner.cs(1 hunks)src/PhysicsInformed/ScientificML/UniversalDifferentialEquations.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
src/PhysicsInformed/PDEs/PoissonEquation.cssrc/PhysicsInformed/PDEs/HeatEquation.cssrc/PhysicsInformed/Interfaces/IPDESpecification.cssrc/PhysicsInformed/ScientificML/HamiltonianNeuralNetwork.cssrc/PhysicsInformed/ScientificML/SymbolicPhysicsLearner.cssrc/PhysicsInformed/PDEs/WaveEquation.cssrc/PhysicsInformed/PDEs/BurgersEquation.cssrc/PhysicsInformed/NeuralOperators/GraphNeuralOperator.cssrc/PhysicsInformed/PhysicsInformedLoss.cssrc/PhysicsInformed/AutomaticDifferentiation.cssrc/PhysicsInformed/ScientificML/LagrangianNeuralNetwork.cssrc/PhysicsInformed/NeuralOperators/DeepOperatorNetwork.cssrc/PhysicsInformed/PINNs/DeepRitzMethod.cssrc/PhysicsInformed/ScientificML/UniversalDifferentialEquations.cssrc/PhysicsInformed/PINNs/VariationalPINN.cssrc/PhysicsInformed/PINNs/PhysicsInformedNeuralNetwork.cssrc/PhysicsInformed/NeuralOperators/FourierNeuralOperator.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
src/PhysicsInformed/PDEs/PoissonEquation.cssrc/PhysicsInformed/PDEs/HeatEquation.cssrc/PhysicsInformed/Interfaces/IPDESpecification.cssrc/PhysicsInformed/ScientificML/HamiltonianNeuralNetwork.cssrc/PhysicsInformed/ScientificML/SymbolicPhysicsLearner.cssrc/PhysicsInformed/PDEs/WaveEquation.cssrc/PhysicsInformed/PDEs/BurgersEquation.cssrc/PhysicsInformed/NeuralOperators/GraphNeuralOperator.cssrc/PhysicsInformed/PhysicsInformedLoss.cssrc/PhysicsInformed/AutomaticDifferentiation.cssrc/PhysicsInformed/ScientificML/LagrangianNeuralNetwork.cssrc/PhysicsInformed/NeuralOperators/DeepOperatorNetwork.cssrc/PhysicsInformed/PINNs/DeepRitzMethod.cssrc/PhysicsInformed/ScientificML/UniversalDifferentialEquations.cssrc/PhysicsInformed/PINNs/VariationalPINN.cssrc/PhysicsInformed/PINNs/PhysicsInformedNeuralNetwork.cssrc/PhysicsInformed/NeuralOperators/FourierNeuralOperator.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build (Windows)
- GitHub Check: CodeQL Analysis
🔇 Additional comments (29)
src/PhysicsInformed/ScientificML/SymbolicPhysicsLearner.cs (1)
149-159: LGTM!The
SymbolicExpressionTypeenum appropriately categorizes the different types of symbolic expressions. The members cover the expected categories for expression trees.src/PhysicsInformed/ScientificML/LagrangianNeuralNetwork.cs (2)
8-58: Excellent educational documentation and proper class structure.The comprehensive documentation provides clear explanations of Lagrangian mechanics, making this accessible for beginners. The class structure with generic constraints and field initialization is well-designed.
60-95: Correct layer initialization logic.The method properly handles both custom layers and default architecture construction. The input dimension correctly accommodates concatenated position and velocity vectors (2 × configurationDim), and the scalar output is appropriate for the Lagrangian.
src/PhysicsInformed/NeuralOperators/FourierNeuralOperator.cs (2)
188-203: LGTM!The forward pass correctly implements the lift → Fourier layers → project pipeline as documented.
80-86: Overall: FNO implementation is structurally complete but functionally incomplete.The architecture and documentation are well-designed, but the core functionality (Fourier transforms, spectral convolution, backpropagation, and weight updates) are placeholders. Consider:
- Adding
[Obsolete("Not yet implemented")]or similar attribute to the class- Documenting the incomplete state in the class summary
- Throwing
NotImplementedExceptioninTrainuntil backprop is implementedThis would prevent users from unknowingly using a non-functional operator.
src/PhysicsInformed/ScientificML/HamiltonianNeuralNetwork.cs (2)
119-129: LGTM!The Hamiltonian computation correctly forwards the state through the network and extracts the scalar energy value.
147-170: LGTM!Hamilton's equations are correctly implemented:
dq/dt = ∂H/∂panddp/dt = -∂H/∂q. The gradient indexing properly maps the momentum components (indicesnto2n-1) to position derivatives and vice versa.src/PhysicsInformed/NeuralOperators/GraphNeuralOperator.cs (1)
53-64: The review comment raises valid concerns about constructor parameter validation and base class initialization, but verification requires access to the complete codebase to examine the base class signature, existing validation logic, and layer initialization implementation.src/PhysicsInformed/PDEs/BurgersEquation.cs (1)
33-48: LGTM!Constructor validation for non-negative viscosity is correct, allowing both viscous (ν > 0) and inviscid (ν = 0) variants. Documentation is excellent.
src/PhysicsInformed/PDEs/HeatEquation.cs (2)
43-58: LGTM!The derivative validation and residual computation are correct. Properly throws when required derivatives are missing, unlike the silent fallback pattern.
33-40: LGTM!Constructor correctly enforces positive thermal diffusivity with a clear error message.
src/PhysicsInformed/PDEs/PoissonEquation.cs (1)
57-78: LGTM!The Laplacian computation correctly iterates over spatial dimensions, and the residual formulation
∇²u - f = 0is accurate.src/PhysicsInformed/PDEs/WaveEquation.cs (2)
66-90: LGTM!The wave equation residual computation is correct. Time is properly indexed as the last dimension, and the spatial Laplacian correctly excludes the time dimension.
49-63: LGTM!Constructor validation is robust with clear error messages for both wave speed and spatial dimension constraints.
src/PhysicsInformed/ScientificML/UniversalDifferentialEquations.cs (1)
63-96: LGTM!Layer initialization logic correctly handles both custom layers and default architecture with appropriate input/output sizing.
src/PhysicsInformed/AutomaticDifferentiation.cs (1)
242-299: LGTM!The Hessian computation correctly exploits symmetry by only computing the upper triangle and copying to the lower triangle (line 293). The finite difference formulas are correctly implemented.
src/PhysicsInformed/Interfaces/IPDESpecification.cs (3)
16-43: LGTM!The
IPDESpecification<T>interface is well-designed with clear method signatures for computing residuals and appropriate properties for input/output dimensions. The documentation is excellent for educational purposes.
56-74: LGTM!The
PDEDerivatives<T>class appropriately uses nullable properties for optional derivative data, with clear shape documentation in the comments.
89-143: LGTM!The
IBoundaryCondition<T>andIInitialCondition<T>interfaces provide clean abstractions for boundary and initial condition handling with appropriate method signatures.src/PhysicsInformed/PINNs/DeepRitzMethod.cs (1)
97-131: LGTM!The layer initialization correctly handles both custom layers and default MLP configuration with appropriate activation functions.
src/PhysicsInformed/PhysicsInformedLoss.cs (2)
204-206: Assumption that time is always the last input dimension may not hold.The code assumes
inputs[inputs.Length - 1]is always the time coordinate. This assumption should be documented or made configurable, as some problem formulations may use different conventions.
59-77: LGTM!The constructor properly initializes optional components with sensible defaults. The nullable parameters allow flexible composition of the loss function.
src/PhysicsInformed/PINNs/VariationalPINN.cs (1)
106-139: LGTM!Layer initialization correctly handles both custom layers and default MLP configuration with Tanh activation and linear output layer.
src/PhysicsInformed/PINNs/PhysicsInformedNeuralNetwork.cs (6)
148-187: LGTM!The layer initialization logic correctly handles both custom layers and default architecture creation. The default 4-layer tanh network with linear output is a reasonable choice for PINNs solving smooth PDEs.
228-236: LGTM!The validation ensures collocation points have the correct dimensionality for the PDE specification.
358-380: LGTM for single-point evaluation.The implementation correctly wraps array input into a tensor, performs the forward pass, and extracts the result. The allocation overhead is acceptable for inference; the training loop performance was addressed separately.
387-390: LGTM!Clean public API for querying the solution at arbitrary points.
397-406: LGTM!Useful validation utility that exposes the PDE residual for solution quality assessment.
413-421: LGTM!Simple and effective history tracking. Consider extending with component-wise losses (data, PDE, BC, IC) in a future iteration for better training diagnostics.
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (11)
scripts/run-tests-sharded.ps1 (1)
251-253: Consider using filter helpers for consistency.Lines 251-253 still use inline filter string construction, while the rest of the file has been refactored to use the new helper functions (
New-NamespaceFilter, etc.). For consistency and maintainability, consider refactoring these to use the helper functions as well.🔎 Example refactoring for line 253
-$shards.Add((New-TestShard -Name "AiDotNet.Tests - Other - 15 Recovery/Concurrency" -Project "tests\AiDotNet.Tests\AiDotNetTests.csproj" -Filter "$categoryFilter&FullyQualifiedName~AiDotNet.Tests.Concurrency|$categoryFilter&FullyQualifiedName~AiDotNet.Tests.Recovery")) +$shards.Add((New-TestShard -Name "AiDotNet.Tests - Other - 15 Recovery/Concurrency" -Project "tests\AiDotNet.Tests\AiDotNetTests.csproj" -Filter (New-NamespaceFilter -CategoryFilter $categoryFilter -Namespaces @("AiDotNet.Tests.Concurrency", "AiDotNet.Tests.Recovery"))))src/AiDotNet.Tensors/LinearAlgebra/SparseTensor.cs (2)
12-26: Consider defensive immutability for exposed arrays.The public array properties (
RowIndices,ColumnIndices,Values, etc.) expose internal state directly, allowing callers to mutate the sparse tensor's data. This is a common trade-off for performance in numeric libraries. If immutability is important for your use cases, consider returningReadOnlySpan<T>orIReadOnlyList<T>, or documenting that callers must not modify the returned arrays.
303-316: Consider behavior with duplicate entries.When the tensor contains duplicate coordinates (e.g., uncoalesced COO),
ToDensewill overwrite with the last value for each coordinate rather than summing. This may be intentional, but consider documenting this behavior or callingCoalesce()internally for predictable results.src/Autodiff/NeuralNetworkDerivatives.cs (1)
335-339: Finite-difference step size should be configurable or documented.The
stepparameter (default1e-5) is a private implementation detail. For float (single precision), this may cause numerical issues due to limited precision. Consider:
- Making this configurable via an optional public parameter
- Documenting the precision implications
src/AiDotNet.Tensors/Groups/ILieAlgebra.cs (1)
9-12: Consider expanding the Lie algebra interface.The interface only exposes
Coordinates. Typical Lie algebra abstractions also include:
int Dimension { get; }— dimension of the algebra- A bracket operation
[X, Y]for the Lie bracketThis may be intentional for a minimal first iteration. If so, consider adding a TODO or expanding the XML documentation to indicate planned extensions.
src/AiDotNet.Tensors/Groups/So3Group.cs (1)
83-114: Potential numerical instability when θ ≈ π.The
Logimplementation handles the small-angle case (θ < 1e-8) but doesn't address the case when θ approaches π, wheresin(theta)approaches zero andfactor = theta / (2 * sin(theta))becomes unstable.For production robustness, consider adding handling for rotations near 180°.
🔎 Proposed handling for near-π rotations
double theta = Math.Acos(cosTheta); if (theta < 1e-8) { return new Vector<T>(new[] { _ops.Zero, _ops.Zero, _ops.Zero }); } + // Near π rotation: sin(theta) ≈ 0, use alternative extraction + if (Math.PI - theta < 1e-6) + { + // Extract axis from R + I (diagonal elements give axis²) + double r01 = Convert.ToDouble(value.Matrix[0, 1]); + double r02 = Convert.ToDouble(value.Matrix[0, 2]); + double r12 = Convert.ToDouble(value.Matrix[1, 2]); + double wx = Math.Sqrt(Math.Max(0, (r00 + 1) / 2)) * Math.Sign(r12); + double wy = Math.Sqrt(Math.Max(0, (r11 + 1) / 2)) * Math.Sign(r02); + double wz = Math.Sqrt(Math.Max(0, (r22 + 1) / 2)) * Math.Sign(r01); + double norm = Math.Sqrt(wx * wx + wy * wy + wz * wz); + if (norm > 1e-10) + { + wx = wx / norm * theta; + wy = wy / norm * theta; + wz = wz / norm * theta; + } + return new Vector<T>(new[] + { + _ops.FromDouble(wx), + _ops.FromDouble(wy), + _ops.FromDouble(wz) + }); + } + double factor = theta / (2.0 * Math.Sin(theta));src/AiDotNet.Tensors/Topology/SimplicialComplex.cs (1)
34-40: Consider caching sorted simplices for repeated access.
GetSimplicessorts by vertex string representation on every call. If this method is called frequently (e.g., during boundary operator construction), consider caching the sorted results or using aSortedSetwith a custom comparer.src/AiDotNet.Tensors/NumericOperations/MultivectorOperations.cs (1)
176-194: MinValue/MaxValue creates multivectors with extreme values in all blades.This may not be mathematically meaningful for multivectors since
MinValuewith all components at minimum doesn't represent a "smallest" multivector in any ordering sense. Consider whether these properties should throwNotSupportedExceptioninstead, similar to other non-scalar operations.src/AiDotNet.Tensors/LinearAlgebra/CliffordAlgebra.cs (1)
146-155: Consider usingBitOperations.PopCountfor better performance.
System.Numerics.BitOperations.PopCountprovides hardware-accelerated bit counting on modern CPUs.🔎 Suggested improvement
+using System.Numerics; + // ... - private static int CountBits(int value) - { - int count = 0; - while (value != 0) - { - count += value & 1; - value >>= 1; - } - return count; - } + private static int CountBits(int value) + => BitOperations.PopCount((uint)value);src/AiDotNet.Tensors/LinearAlgebra/Multivector.cs (1)
183-219: InnerProduct implements left contraction only.The condition
gradeA > gradeB(line 200-201) skips terms where the left operand has higher grade, which is the definition of left contraction. If symmetric inner product or right contraction is needed, this should be documented or alternative methods provided.Consider adding a doc comment clarifying this is left contraction:
/// <summary> /// Computes the left contraction (inner product) of this multivector with another. /// </summary>src/AiDotNet.Tensors/Manifolds/HyperboloidManifold.cs (1)
177-183: Consider usingMath.Acoshif targeting .NET Core 2.1+..NET Core 2.1 (and .NET Standard 2.1) and later provide
Math.Acosh(double)natively. If backward compatibility to earlier versions isn't required, you could remove this helper and use the built-in method.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
.github/workflows/automated-release.yml.github/workflows/docs.yml.github/workflows/sonarcloud.ymlREADME.mdSPRINT_PLAN_PR449_ADVANCED_ALGEBRA.mdSPRINT_PLAN_PR449_ISSUE400.mddocs/PhysicsInformedBenchmarks.mdscripts/run-tests-sharded.ps1src/AiDotNet.Tensors/Groups/ILieAlgebra.cssrc/AiDotNet.Tensors/Groups/ILieGroup.cssrc/AiDotNet.Tensors/Groups/Se3.cssrc/AiDotNet.Tensors/Groups/Se3Group.cssrc/AiDotNet.Tensors/Groups/So3.cssrc/AiDotNet.Tensors/Groups/So3Group.cssrc/AiDotNet.Tensors/Groups/Su2.cssrc/AiDotNet.Tensors/Groups/Su2Group.cssrc/AiDotNet.Tensors/Helpers/MathHelper.cssrc/AiDotNet.Tensors/LinearAlgebra/CliffordAlgebra.cssrc/AiDotNet.Tensors/LinearAlgebra/Multivector.cssrc/AiDotNet.Tensors/LinearAlgebra/Octonion.cssrc/AiDotNet.Tensors/LinearAlgebra/SparseStorageFormat.cssrc/AiDotNet.Tensors/LinearAlgebra/SparseTensor.cssrc/AiDotNet.Tensors/Manifolds/HyperboloidManifold.cssrc/AiDotNet.Tensors/Manifolds/PoincareBallManifold.cssrc/AiDotNet.Tensors/NumericOperations/MultivectorOperations.cssrc/AiDotNet.Tensors/NumericOperations/OctonionOperations.cssrc/AiDotNet.Tensors/Topology/Simplex.cssrc/AiDotNet.Tensors/Topology/SimplicialComplex.cssrc/Autodiff/NeuralNetworkDerivatives.cs
✅ Files skipped from review due to trivial changes (3)
- README.md
- SPRINT_PLAN_PR449_ISSUE400.md
- .github/workflows/docs.yml
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
src/AiDotNet.Tensors/Groups/Se3.cssrc/AiDotNet.Tensors/Groups/ILieAlgebra.cssrc/AiDotNet.Tensors/Groups/So3.cssrc/AiDotNet.Tensors/Helpers/MathHelper.cssrc/AiDotNet.Tensors/Groups/Su2.cssrc/AiDotNet.Tensors/Groups/ILieGroup.cssrc/AiDotNet.Tensors/Groups/Su2Group.cssrc/AiDotNet.Tensors/Groups/Se3Group.cssrc/AiDotNet.Tensors/Manifolds/HyperboloidManifold.cssrc/AiDotNet.Tensors/Groups/So3Group.cssrc/AiDotNet.Tensors/LinearAlgebra/Octonion.cssrc/Autodiff/NeuralNetworkDerivatives.cssrc/AiDotNet.Tensors/LinearAlgebra/SparseTensor.cssrc/AiDotNet.Tensors/LinearAlgebra/CliffordAlgebra.cssrc/AiDotNet.Tensors/NumericOperations/MultivectorOperations.cssrc/AiDotNet.Tensors/Manifolds/PoincareBallManifold.cssrc/AiDotNet.Tensors/LinearAlgebra/SparseStorageFormat.cssrc/AiDotNet.Tensors/NumericOperations/OctonionOperations.cssrc/AiDotNet.Tensors/LinearAlgebra/Multivector.cssrc/AiDotNet.Tensors/Topology/SimplicialComplex.cssrc/AiDotNet.Tensors/Topology/Simplex.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
src/AiDotNet.Tensors/Groups/Se3.cssrc/AiDotNet.Tensors/Groups/ILieAlgebra.cssrc/AiDotNet.Tensors/Groups/So3.cssrc/AiDotNet.Tensors/Helpers/MathHelper.cssrc/AiDotNet.Tensors/Groups/Su2.cssrc/AiDotNet.Tensors/Groups/ILieGroup.cssrc/AiDotNet.Tensors/Groups/Su2Group.cssrc/AiDotNet.Tensors/Groups/Se3Group.cssrc/AiDotNet.Tensors/Manifolds/HyperboloidManifold.cssrc/AiDotNet.Tensors/Groups/So3Group.cssrc/AiDotNet.Tensors/LinearAlgebra/Octonion.cssrc/Autodiff/NeuralNetworkDerivatives.cssrc/AiDotNet.Tensors/LinearAlgebra/SparseTensor.cssrc/AiDotNet.Tensors/LinearAlgebra/CliffordAlgebra.cssrc/AiDotNet.Tensors/NumericOperations/MultivectorOperations.cssrc/AiDotNet.Tensors/Manifolds/PoincareBallManifold.cssrc/AiDotNet.Tensors/LinearAlgebra/SparseStorageFormat.cssrc/AiDotNet.Tensors/NumericOperations/OctonionOperations.cssrc/AiDotNet.Tensors/LinearAlgebra/Multivector.cssrc/AiDotNet.Tensors/Topology/SimplicialComplex.cssrc/AiDotNet.Tensors/Topology/Simplex.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Codacy Security Scan
- GitHub Check: CodeQL Analysis
🔇 Additional comments (69)
scripts/run-tests-sharded.ps1 (6)
76-87: LGTM! Clean filter composition helper.The function correctly filters out empty entries, trims whitespace, and composes filter parts with AND logic. Edge cases are handled appropriately.
89-138: LGTM! Excellent refactoring of filter construction logic.The three new filter builder functions (
New-UnitCoreFilter,New-UnitShardFilter,New-NamespaceFilter) effectively centralize and standardize test filter construction. This improves maintainability and reduces duplication compared to inline string building.
197-250: LGTM! Well-structured data-driven shard definitions.The refactoring to use explicit data arrays (
$unitNamespaceRoots,$unitExcludedSegments) with the new filter builders makes the shard definitions much more maintainable. The logic correctly distributes test namespaces across 12 shards for better parallelization.
254-259: LGTM! Clean addition of namespace-based shards.The six new shards use the
New-NamespaceFilterhelper consistently and follow the established naming convention. This provides better test isolation for these specific namespaces.
275-277: LGTM! Conditional shard creation aligns with build logic.The framework check correctly ensures that the
AiDotNet.Serving.Testsshard is only added when the framework isnet8*, consistent with the conditional build at lines 186-188. This prevents attempting to run tests for a project that wasn't built.
186-188: Verify the framework restriction for AiDotNet.Serving.Tests aligns with the project's target frameworks.The conditional build at lines 186-188 only executes for
net8*frameworks. Confirm this restriction matches the project's actualTargetFramework(s)setting and is intentional.src/AiDotNet.Tensors/LinearAlgebra/SparseStorageFormat.cs (1)
6-11: LGTM!The enum cleanly defines the three standard sparse storage formats (COO, CSR, CSC). Consider adding brief XML doc summaries on each member for IntelliSense discoverability, but this is optional.
src/AiDotNet.Tensors/LinearAlgebra/SparseTensor.cs (6)
52-63: LGTM!Private constructor appropriately trusts validated input from factory methods.
99-134: LGTM!The
FromDensemethods correctly filter out values within the tolerance threshold. The default tolerance ofops.Zeroensures exact zeros are excluded.
136-171: LGTM!The
ToCooconversion correctly reconstructs row/column indices from CSR/CSC pointer arrays and clones the values for immutability.
173-229: LGTM!The
ToCsrandToCscconversions correctly use the counting-sort approach to build compressed row/column pointer arrays from coalesced COO data.
231-286: LGTM!The
Coalesceimplementation correctly sorts entries, sums duplicate coordinates, and filters out zeros. The algorithm handles edge cases properly.
288-301: LGTM!The
Transposeimplementation correctly handles all three storage formats by exploiting the duality between CSR and CSC representations..github/workflows/sonarcloud.yml (1)
118-118: LGTM: Consistent artifact upload action updates.All four artifact upload steps have been updated to the same newer commit of
actions/upload-artifact@v4. The updates are consistent across:
- Build artifacts upload (Line 118)
- NuGet package upload (Line 130)
- Coverage and test results upload (Line 297)
- Test results upload for net471 (Line 461)
All inputs and configurations remain unchanged, ensuring no behavioral changes beyond the action version update. This aligns with the update in
automated-release.yml.Also applies to: 130-130, 297-297, 461-461
.github/workflows/automated-release.yml (1)
319-319: [Your rewritten review comment text here]
[Exactly ONE classification tag]docs/PhysicsInformedBenchmarks.md (1)
1-141: Well-structured benchmark documentation.The documentation provides clear examples for PDE and operator learning benchmarks with appropriate metrics. The code examples follow C# conventions and reference the expected namespace structure.
SPRINT_PLAN_PR449_ADVANCED_ALGEBRA.md (1)
1-174: Comprehensive implementation roadmap.The sprint plan thoroughly documents scope, constraints, architecture targets, and deliverables for advanced algebra features. The model coverage matrix and verification matrix provide clear acceptance criteria.
src/Autodiff/NeuralNetworkDerivatives.cs (3)
1-59: Good fallback strategy with comprehensive input validation.The public API provides appropriate null checks and argument validation. The exception-based fallback from analytic to finite-difference derivatives is a sensible pattern for robustness.
274-294: Gradient accumulation may be incorrect across output indices.Each iteration calls
scalar.Backward()on Line 283, but gradients may accumulate ininputNode.Gradientacross iterations without being reset. This could cause incorrect gradient values for outputs after the first.The concern depends on whether
ComputationNode<T>maintains gradient state across multipleBackward()calls. If gradients are accumulated (not overwritten) by design, then resetting with a method likeZeroGrad()before each iteration would be necessary.
695-727: Add finite-difference unit tests to validate the GELU second derivative approximation.The implementation uses the tanh-based GELU approximation and correctly applies the chain rule for its second derivative. To ensure numerical accuracy, add tests comparing the computed second derivative against finite-difference estimates.
src/AiDotNet.Tensors/Helpers/MathHelper.cs (1)
77-98: Consistent pattern for Octonion and Multivector operations.The new branches correctly follow the established pattern used for
Complex<>type handling, including proper null checks and exception messages.src/AiDotNet.Tensors/Groups/So3Group.cs (3)
1-23: LGTM - Core structure and basic operations.The class structure, constructor initialization, and basic operations (
Identity,Compose,Inverse) are correctly implemented. The transpose-based inverse is mathematically correct for orthogonal rotation matrices.
28-81: LGTM - Rodrigues formula implementation.The exponential map correctly implements Rodrigues' formula with appropriate small-angle Taylor series approximations (using threshold 1e-8). The skew-symmetric matrix construction and the formula
R = I + a*K + b*K²are mathematically sound.
116-133: LGTM - Adjoint and matrix multiplication helper.The
Adjointmethod correctly returns the rotation matrix as SO(3)'s adjoint representation. The privateMultiplyhelper is a clean 3x3 matrix multiplication.src/AiDotNet.Tensors/Groups/Su2Group.cs (4)
1-31: LGTM - SU(2) core operations.The
Identity,Compose(quaternion multiplication), andInverse(conjugate) implementations are correct. The quaternion product formula follows the standard Hamilton product convention.
36-63: LGTM - Exponential map implementation.The
Expcorrectly maps a 3D tangent vector to SU(2) using the half-angle quaternion formula. The small-angle Taylor approximation(1, 0.5x, 0.5y, 0.5z)is mathematically sound.
65-87: LGTM - Logarithm map.The
Logcorrectly usesatan2(norm, w)to extract the rotation angle, handling the correct quadrant. The small-norm fallback returning zero vector is appropriate.
89-121: LGTM - Adjoint representation.The
Adjointcorrectly computes the 3×3 rotation matrix from quaternion components using the standard formula.src/AiDotNet.Tensors/Groups/Se3Group.cs (4)
1-53: LGTM - SE(3) core operations and exponential map.The
Identity,Compose,Inverse, andExpimplementations correctly handle SE(3) group operations. The V-matrix construction for the exponential map uses the correct closed-form formula with appropriate small-angle handling.
55-101: LGTM - Logarithm map.The
Logcorrectly computes the V-inverse matrix and extracts the 6D tangent vector. The small-angle coefficienta = 1/12is the correct Taylor expansion term.
103-121: LGTM - Adjoint representation.The 6×6 adjoint matrix is correctly constructed with the rotation matrix in the top-left and bottom-right blocks, and the
[t]×Rterm in the bottom-left.
183-254: LGTM - Helper methods.The vector/matrix multiplication overloads,
Skew,Add,Negate, and the staticdouble[,]multiplication helper are all correctly implemented with appropriate dimension validation.src/AiDotNet.Tensors/Topology/SimplicialComplex.cs (4)
1-32: LGTM - Core storage and AddSimplex.The
AddSimplexmethod correctly handles recursive face inclusion with proper null validation. The storage structure usingDictionary<int, HashSet<Simplex>>is appropriate for organizing simplices by dimension.
42-69: LGTM - Boundary operator construction.The boundary operator correctly maps k-simplices to (k-1)-simplices using the signed boundary faces from
Simplex.Boundary(). The row mapping ensures consistent ordering.
71-86: LGTM - Incidence matrix.The incidence matrix correctly converts the boundary operator to a binary matrix by replacing non-zero entries with ones.
88-126: LGTM - Hodge Laplacian.The Hodge Laplacian
L_k = B_k^T B_k + B_{k+1} B_{k+1}^Tis correctly computed with proper boundary checks fork = 0andk = MaxDimension.src/AiDotNet.Tensors/Groups/ILieGroup.cs (1)
1-23: LGTM - Clean Lie group interface.The interface provides a well-designed abstraction for Lie group operations with appropriate method signatures. The dual type parameters (
Tfor numerics,TElementfor group elements) enable flexible implementations.src/AiDotNet.Tensors/Manifolds/PoincareBallManifold.cs (7)
1-34: LGTM - Constructor and initialization.Proper initialization with curvature validation, epsilon for numerical stability, and dual constructors (default and parameterized).
35-52: LGTM - Möbius addition.The Möbius addition formula for the Poincaré ball model is correctly implemented with the standard numerator/denominator structure.
54-71: LGTM - Exponential map.The exponential map correctly uses
tanh(sqrt(c) * lambda * ||v|| / 2)scaling for the tangent direction, with proper zero-norm handling.
73-89: LGTM - Logarithm map.The logarithm map correctly uses
atanh(sqrt(c) * ||diff||)and applies the inverse conformal factor scaling.
91-100: LGTM - Distance function.The hyperbolic distance formula
2 * atanh(sqrt(c) * ||x ⊕ (-y)||) / sqrt(c)is correctly implemented.
102-128: LGTM - Parallel transport and Lambda.The parallel transport using gyration with lambda-ratio scaling is correctly implemented. The conformal factor
Lambda = 2 / (1 - c*||x||²)is standard.
130-189: LGTM - Helper methods.All helper methods (
Dot,Norm,Add,Scale,Negate,CreateZeroVector,SafeDenominator,EnsureSameLength,ValidateCurvature) are well-implemented with appropriate validation.src/AiDotNet.Tensors/Topology/Simplex.cs (3)
1-34: LGTM - Constructor with validation.The constructor properly validates input (null, empty, duplicates), sorts vertices for canonical form, and uses the modern collection expression syntax for array initialization.
35-57: LGTM - Boundary computation.The
Boundary()method correctly computes oriented (k-1)-faces by omitting each vertex in turn with alternating signs(-1)^i. Empty boundary for 0-simplices is properly handled.
59-91: LGTM - Equality, hashing, and string representation.The
Equals,GetHashCode, andToStringimplementations are correct. Since vertices are stored in sorted order, element-wise comparison is sufficient for equality.src/AiDotNet.Tensors/LinearAlgebra/Octonion.cs (6)
1-52: LGTM - Structure and basic properties.The readonly struct design is appropriate for an immutable algebraic type. The
Opsfallback pattern handles default struct initialization gracefully.NormSquaredcorrectly sums squares of all 8 components.
54-84: LGTM - Derived properties.
Magnitude,VectorMagnitude, andIsScalarare correctly implemented.
86-124: LGTM - Conjugate, Scale, and Inverse.
Conjugatecorrectly negates only imaginary components.Scaleapplies uniform scaling.Inverseuses the standard formulaq* / ||q||²with proper zero-norm validation.
126-152: LGTM - Addition and subtraction operators.Component-wise addition and subtraction are correctly implemented.
250-293: LGTM - Division, equality, and string representation.Division via inverse multiplication, equality operators,
GetHashCodewith null-safety, andToStringare all correctly implemented.
154-248: Document the octonion multiplication convention used.The implementation follows a standard octonion multiplication pattern with 64 terms organized by the Cayley-Dickson construction. Verify this matches your intended convention and add a comment referencing the specific convention (e.g., cite the cyclic triples: (1,2,3), (1,4,5), (1,7,6), (2,4,6), (2,5,7), (3,4,7), (3,6,5)) or the authoritative source used.
src/AiDotNet.Tensors/Manifolds/HyperboloidManifold.cs (3)
19-25: Redundant curvature validation in default constructor.The default constructor sets
Curvature = _ops.FromDouble(1.0)which is always positive, soValidateCurvature()will never fail. While not incorrect, this is slightly redundant.
66-76: Potential sign error in LogMap tangent vector computation.The formula on line 75 computes
pointScaled + baseScaled * alpha, but the standard LogMap formula for hyperboloid models typically computes the difference to project out the base point component. The correct formula should be:v = point - base * <base, point>_MWhere
<·,·>_Mis the Minkowski inner product. Sincedot = MinkowskiDot(baseScaled, pointScaled)andalpha = -dot, the termbaseScaled * alphaequals-baseScaled * dot. Adding this effectively computespointScaled - baseScaled * dot, which would be correct. However, verify this matches your intended mathematical formulation.
109-119: Minkowski dot product implementation looks correct.The Minkowski inner product negates the time-like (first) component and sums the space-like components, which is the standard convention for hyperbolic geometry with signature (-,+,+,...).
src/AiDotNet.Tensors/NumericOperations/MultivectorOperations.cs (3)
16-25: Constructor design is clean and follows defensive programming.The null check on
algebraand delegation toMathHelper.GetNumericOperations<T>()aligns with the project's patterns.
65-73: Sqrt correctly restricts to scalar multivectors.Non-scalar multivector square roots are undefined in general Clifford algebras, so throwing
NotSupportedExceptionis appropriate.
277-356: Vectorized fallback implementations are correctly delegated.All span-based operations properly delegate to
VectorizedOperationsFallback, maintaining consistency with the interface contract.src/AiDotNet.Tensors/LinearAlgebra/CliffordAlgebra.cs (2)
21-54: Constructor validation and initialization are thorough.The bounds checking (non-negative p/q/r, positive dimension, max 30 for 32-bit indices) and metric/grade precomputation are well-implemented.
72-87: ReorderingSign algorithm correctly computes permutation parity.The algorithm counts how many basis vectors in
bladeAneed to pass through basis vectors inbladeB(those at lower indices), which determines the sign from anti-commutativity.src/AiDotNet.Tensors/NumericOperations/OctonionOperations.cs (4)
15-18: Constructor correctly initializes the backing numeric operations.
45-73: Sqrt implementation for octonions is mathematically sound.The formula correctly handles the scalar and vector parts using the half-angle approach, with proper zero-magnitude edge case handling.
125-131: Power(0, 0) returning One is a reasonable convention.Mathematically, 0^0 is indeterminate, but returning 1 is a common convention in many numeric libraries. This is acceptable but worth documenting.
265-343: Vectorized fallback implementations are consistent with MultivectorOperations.All span-based operations properly delegate to
VectorizedOperationsFallback.src/AiDotNet.Tensors/LinearAlgebra/Multivector.cs (4)
23-36: Constructors provide flexible initialization options.The three public constructors (zero-initialized, from list, from sparse dictionary) cover common use cases well. The delegation to a private constructor with validated components is a clean pattern.
112-122: Reverse operation correctly applies grade-dependent sign.The formula
sign = ((-1)^(grade*(grade-1)/2))is the standard reversal sign for Clifford algebras.
221-234: Inverse correctly validates scalar product requirement.The implementation properly checks that
this * this.Reverse()is scalar before computing the inverse, and handles the zero-norm case with an appropriate exception.
287-296: GetHashCode handles null coefficients safely.Using
_coefficients[i]?.GetHashCode() ?? 0is defensive, though with numeric types T, null values are unlikely unless T is a nullable reference type.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (16)
tests/AiDotNet.Tests/UnitTests/Engines/CpuSparseEngineTests.cs (1)
10-12: Incomplete test coverage for ISparseEngine methods.The test suite covers SpMV, SpMM, SparseToDense, DenseToSparse, Coalesce, SparseTranspose, and AddSparseDense. However, several methods from the ISparseEngine interface lack test coverage:
SpMVTransposeSpSpMMMultiplySparseDenseSparseGatherSparseScatterSparseScatterAddConsider adding tests for these methods to ensure comprehensive coverage.
Would you like me to help generate test cases for the missing methods?
src/AiDotNet.Tensors/Engines/ISparseEngine.cs (2)
28-45: Consider making documentation format-agnostic.The parameter documentation states "sparse matrix A in CSR format", but since
SparseTensor<T>supports multiple storage formats (COO, CSR, CSC), the implementation handles format conversion internally. Consider updating the doc to say "sparse matrix A" without specifying the format, since the caller doesn't need to pre-convert.
156-169: Inconsistent index parameter type withSparseScatter.
SparseScatterusesSparseTensor<T> indiceswhileSparseScatterAdduses(int[] rows, int[] cols) indices. This asymmetry may confuse API consumers. If intentional (e.g., for performance or in-place semantics), consider adding a brief remark explaining why the tuple form is preferred here.src/AiDotNet.Tensors/Engines/CpuSparseEngine.cs (4)
145-164: Consider reordering loops for better cache locality.The current loop order iterates columns of the dense matrix (
k) before sparse non-zeros (idx). Reordering so that the sparse iteration is outer can improve cache locality when accessingdense[j, k]sequentially across columns.🔎 Proposed optimization
for (int i = 0; i < sparse.Rows; i++) { int start = rowPtrs[i]; int end = rowPtrs[i + 1]; - for (int k = 0; k < dense.Columns; k++) + for (int idx = start; idx < end; idx++) { - T sum = ops.Zero; - - for (int idx = start; idx < end; idx++) + int j = colIndices[idx]; + T aVal = values[idx]; + + for (int k = 0; k < dense.Columns; k++) { - int j = colIndices[idx]; - T aVal = values[idx]; - sum = ops.Add(sum, ops.Multiply(aVal, dense[j, k])); + result[i, k] = ops.Add(result[i, k], ops.Multiply(aVal, dense[j, k])); } - - result[i, k] = sum; } }
219-226: Nullable check is redundant for numeric types.For numeric value types (the expected use case),
TryGetValuewill always return a valid value when it returnstrue. Theexisting is not nullcheck is unnecessary and slightly misleading. However, it's harmless for the typical numericTconstraint.🔎 Simplified pattern
- if (rowAccum.TryGetValue(j, out T? existing) && existing is not null) + if (rowAccum.TryGetValue(j, out T existing)) { rowAccum[j] = ops.Add(existing, product); }
366-398: Consider documenting scatter behavior with duplicate indices.
SparseScatteruses "last write wins" semantics when theindicessparse tensor contains duplicates. This is standard scatter behavior but could be clarified in the interface documentation to set expectations for callers.
455-488: Threshold comparison uses strict greater-than.Values with
|val| == thresholdare excluded (treated as zero). This is fine if intentional, but the interface documentation says "below this" which could be interpreted either way. Consider clarifying the semantics in the doc (exclusive vs inclusive threshold).tests/AiDotNet.Tests/UnitTests/Engines/CpuHyperbolicManifoldEngineTests.cs (1)
221-243: Batch tests could be strengthened by comparing against individual operations.The batch operation tests only verify output dimensions. Consider adding assertions that compare batch results with the results of calling individual operations in a loop, to ensure the batch implementation is functionally equivalent.
🔎 Example strengthening for PoincareExpMapBatch test
// Assert Assert.Equal(2, result.Rows); Assert.Equal(2, result.Columns); + + // Verify each row matches individual operation + for (int i = 0; i < basePoints.Rows; i++) + { + var basePoint = new Vector<double>(new[] { basePoints[i, 0], basePoints[i, 1] }); + var tangent = new Vector<double>(new[] { tangentVectors[i, 0], tangentVectors[i, 1] }); + var expected = _engine.PoincareExpMap(basePoint, tangent, curvature); + Assert.Equal(expected[0], result[i, 0], precision: 10); + Assert.Equal(expected[1], result[i, 1], precision: 10); + }src/AiDotNet.Tensors/Engines/CpuHyperbolicManifoldEngine.cs (4)
80-85: Dead code:directionarray is computed but never used.The
directionarray is computed on lines 81-85 but is never referenced afterward. The actual scaled tangent vector usesv[i]directly on line 91.🔎 Remove unused code
- // Compute direction = v / ||v|| - double[] direction = new double[dim]; - for (int i = 0; i < dim; i++) - { - direction[i] = v[i] / (vNorm + Epsilon); - } - // Compute scaled direction for Möbius addition
487-487: Unused variableops.The
opsvariable is retrieved but never used in this method.🔎 Remove unused variable
- var ops = MathHelper.GetNumericOperations<T>(); var result = new Matrix<T>(basePoints.Rows, basePoints.Columns);
515-515: Unused variableops.Same issue as
PoincareExpMapBatch- theopsvariable is retrieved but never used.🔎 Remove unused variable
- var ops = MathHelper.GetNumericOperations<T>(); var result = new T[x.Rows];
639-650: Consider throwing for empty arrays in MinkowskiInnerProduct.Returning 0.0 for empty arrays silently hides a likely programming error. Consider throwing
ArgumentExceptionfor consistency with other validation in this class.🔎 Proposed change
private static double MinkowskiInnerProduct(double[] a, double[] b) { // <a,b>_L = -a0*b0 + sum(ai*bi) for i > 0 - if (a.Length < 1 || b.Length < 1) return 0.0; + if (a.Length < 1 || b.Length < 1) + { + throw new ArgumentException("Vectors must have at least one dimension for Minkowski inner product."); + } double result = -a[0] * b[0];src/AiDotNet.Tensors/Engines/IAdvancedAlgebraEngine.cs (2)
76-89: Consider documenting dimension order convention for OctonionMatMul.The signature uses
Octonion<T>[,]with dimensions described as(batch x features)for input and(output x input features)for weight. This is correct for typical neural network conventions, but consider adding an explicit note that the weight matrix is transposed relative to some conventions (output features as rows).
247-261: Consider adding Se3AdjointBatch for symmetry with SO(3).The interface provides
So3AdjointBatchbut lacks the correspondingSe3AdjointBatch. The SE(3) adjoint representation (a 6×6 matrix) is commonly used in robotics and SLAM applications. This could be deferred to a future iteration if not immediately needed.tests/AiDotNet.Tests/UnitTests/Engines/CpuAdvancedAlgebraEngineTests.cs (2)
110-135: Non-associativity test uses operators directly, not engine batch method.This test validates the fundamental
Octonion<T>operator behavior, which is valuable. However, it doesn't exerciseCpuAdvancedAlgebraEnginemethods. Consider clarifying the test name or adding a separate batch-focused test if the intent is to test engine functionality.
15-382: Consider adding tests for untested engine methods and error cases.The test suite covers core functionality well but has some gaps:
- Untested methods:
WedgeProductBatch,InnerProductBatch,OctonionMatMul,Se3ComposeBatch- No error-case tests: null inputs, mismatched array lengths (which should throw
ArgumentException)- No empty array tests: verify correct handling of zero-length inputs
These could be added in a follow-up to improve coverage.
Would you like me to generate additional test cases for the untested methods and error scenarios?
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/AiDotNet.Tensors/Engines/CpuAdvancedAlgebraEngine.cssrc/AiDotNet.Tensors/Engines/CpuHyperbolicManifoldEngine.cssrc/AiDotNet.Tensors/Engines/CpuSparseEngine.cssrc/AiDotNet.Tensors/Engines/IAdvancedAlgebraEngine.cssrc/AiDotNet.Tensors/Engines/IHyperbolicManifoldEngine.cssrc/AiDotNet.Tensors/Engines/ISparseEngine.cstests/AiDotNet.Tests/UnitTests/Engines/CpuAdvancedAlgebraEngineTests.cstests/AiDotNet.Tests/UnitTests/Engines/CpuHyperbolicManifoldEngineTests.cstests/AiDotNet.Tests/UnitTests/Engines/CpuSparseEngineTests.cs
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:30.125Z
Learning: The AiDotNet project uses project-level global usings (configured in AiDotNet.csproj with `<Using Include=AiDotNet.Tensors.LinearAlgebra />`), making Vector<T>, Matrix<T>, and Tensor<T> available in all files without explicit per-file using directives. Do not flag missing using directives for these types in this project.
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:50:00.720Z
Learning: The AiDotNet project uses global using directives in src/AiDotNet.csproj via <Using Include="..." /> for AiDotNet.Tensors.LinearAlgebra, AiDotNet.Tensors.Engines, AiDotNet.Tensors.Interfaces, AiDotNet.Tensors.NumericOperations, AiDotNet.Tensors.Helpers, AiDotNet.Autodiff, System.Text, and AiDotNet.Helpers. Types like Vector<T>, Matrix<T>, Tensor<T>, and related linear algebra types are available project-wide without per-file using statements.
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
tests/AiDotNet.Tests/UnitTests/Engines/CpuHyperbolicManifoldEngineTests.cstests/AiDotNet.Tests/UnitTests/Engines/CpuAdvancedAlgebraEngineTests.cssrc/AiDotNet.Tensors/Engines/CpuSparseEngine.cssrc/AiDotNet.Tensors/Engines/ISparseEngine.cstests/AiDotNet.Tests/UnitTests/Engines/CpuSparseEngineTests.cssrc/AiDotNet.Tensors/Engines/CpuAdvancedAlgebraEngine.cssrc/AiDotNet.Tensors/Engines/IAdvancedAlgebraEngine.cssrc/AiDotNet.Tensors/Engines/CpuHyperbolicManifoldEngine.cssrc/AiDotNet.Tensors/Engines/IHyperbolicManifoldEngine.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
tests/AiDotNet.Tests/UnitTests/Engines/CpuHyperbolicManifoldEngineTests.cstests/AiDotNet.Tests/UnitTests/Engines/CpuAdvancedAlgebraEngineTests.cssrc/AiDotNet.Tensors/Engines/CpuSparseEngine.cssrc/AiDotNet.Tensors/Engines/ISparseEngine.cstests/AiDotNet.Tests/UnitTests/Engines/CpuSparseEngineTests.cssrc/AiDotNet.Tensors/Engines/CpuAdvancedAlgebraEngine.cssrc/AiDotNet.Tensors/Engines/IAdvancedAlgebraEngine.cssrc/AiDotNet.Tensors/Engines/CpuHyperbolicManifoldEngine.cssrc/AiDotNet.Tensors/Engines/IHyperbolicManifoldEngine.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build (Windows)
- GitHub Check: CodeQL Analysis
🔇 Additional comments (26)
tests/AiDotNet.Tests/UnitTests/Engines/CpuSparseEngineTests.cs (4)
16-70: LGTM!The SpMV test cases cover important scenarios: identity matrix, single-row sparse (dot product), and empty sparse matrix. The expected values are correctly calculated and assertions are properly structured.
76-118: LGTM!The SpMM tests correctly validate identity and scaling matrix operations. Assertions verify both shape and element values.
124-173: LGTM!The SparseToDense and DenseToSparse tests correctly verify bidirectional conversion between sparse and dense representations.
178-245: LGTM!The Coalesce, SparseTranspose, and AddSparseDense tests properly validate duplicate entry summation, matrix transposition, and element-wise addition respectively.
src/AiDotNet.Tensors/Engines/ISparseEngine.cs (2)
5-23: LGTM!Excellent interface documentation with beginner-friendly explanations. The remarks clearly explain the purpose of sparse tensors and provide helpful memory comparison examples.
24-217: LGTM!The interface provides a comprehensive and well-organized API for sparse tensor operations. The grouping via regions improves readability, and the method signatures are consistent with typical sparse linear algebra libraries.
src/AiDotNet.Tensors/Engines/CpuSparseEngine.cs (7)
22-27: LGTM!Singleton pattern is appropriate for a stateless CPU engine implementation.
32-75: LGTM!The SpMV implementation correctly uses CSR format for efficient row-major access. The algorithm properly computes y[i] = Σ A[i,j] * x[j] with O(nnz) complexity.
78-119: LGTM!Efficient transpose-multiply implementation that avoids explicit transposition by iterating the original matrix and accumulating into the result vector at column positions.
260-331: LGTM!Element-wise operations are correctly implemented.
AddSparseDenseproperly copies the dense matrix first then adds sparse entries.MultiplySparseDenseefficiently computes the Hadamard product at sparse positions only.
400-427: LGTM!
SparseScatterAddcorrectly implements scatter-add semantics by accumulating values at duplicate positions, which is essential for gradient computation in sparse operations.
434-452:SparseToDensemay produce unexpected results with duplicate indices.If the input sparse tensor has duplicate indices (not coalesced), the last value at each position wins. This differs from
Coalescewhich sums duplicates. Consider either documenting this behavior or callingsparse.Coalesce()first for consistency.
490-506: LGTM!Clean delegation to
SparseTensor.Coalesce()andSparseTensor.Transpose()methods avoids code duplication and ensures consistent behavior.src/AiDotNet.Tensors/Engines/IHyperbolicManifoldEngine.cs (1)
1-245: Well-designed interface with comprehensive documentation.The interface provides a clean abstraction for hyperbolic manifold operations with excellent mathematical documentation. The API is suitable for hyperbolic neural network implementations.
src/AiDotNet.Tensors/Engines/CpuHyperbolicManifoldEngine.cs (1)
22-27: Solid implementation with good numerical stability safeguards.The engine demonstrates careful attention to numerical stability with epsilon guards, domain clamping for inverse hyperbolic functions, and projection safeguards. The singleton pattern is appropriate for this stateless computation engine.
src/AiDotNet.Tensors/Engines/IAdvancedAlgebraEngine.cs (2)
1-23: Well-structured interface with comprehensive documentation.The interface design cleanly separates three algebraic domains (Octonions, Multivectors, Lie Groups) with consistent batch operation patterns. The XML documentation with "For Beginners" sections is excellent for accessibility.
93-175: Solid Clifford algebra interface coverage.The six operations cover the fundamental geometric algebra primitives. The grade projection with an integer parameter is a clean design choice.
src/AiDotNet.Tensors/Engines/CpuAdvancedAlgebraEngine.cs (6)
23-28: Singleton pattern is appropriate for stateless CPU engine.The sealed class with a static
Instanceproperty is a clean choice for a stateless computation engine.
33-49: Clean batch operation pattern with proper validation.The implementation correctly validates null inputs and array length matching before processing. The delegation to
Octonion<T>.operator*is appropriate.
99-137: OctonionMatMul correctly implements the neural network layer convention.The triple-nested loop correctly computes the output as
sum_i(weight[o,i] * input[b,i]). The explicit comment about non-associativity (line 129) is valuable documentation. The zero initialization usingMathHelper.GetNumericOperations<T>()properly handles generic numeric types.
234-268: GradeProjectBatch implementation is correct and well-validated.The method properly validates the grade parameter, iterates over basis elements, and uses
algebra.GetGrade(blade)to selectively keep coefficients of the requested grade. The use ofMathHelper.GetNumericOperations<T>().Zeroensures type-safe zero initialization.
275-287: Lie group batch operations correctly delegate to group methods.The implementation pattern is consistent: validate inputs, allocate result array, loop and delegate. This is clean and maintainable.
42-46: Verify whether algebraic types are value types or reference types.The batch operation loop assumes all array elements are non-null. Determine whether
Octonion<T>,Multivector<T>,So3<T>, andSe3<T>are structs (value types) or classes (reference types). If they are classes, null elements could causeNullReferenceExceptionin operations likeleft[i] * right[i]. If they are structs, this is not a concern.tests/AiDotNet.Tests/UnitTests/Engines/CpuAdvancedAlgebraEngineTests.cs (3)
11-13: Test class setup follows project conventions.Using the singleton instance for testing is appropriate since the engine is stateless.
209-232: GradeProjectBatch test has good coverage of grade extraction.The test creates a multivector with coefficients at all grades (0, 1, 1, 2) and verifies only grade-0 survives projection. The assertions at lines 228-231 correctly verify that higher-grade components are zeroed.
264-287: Round-trip tests validate numerical consistency of Exp/Log maps.The
So3ExpLogBatch_RoundTriptest correctly verifies thatLog(Exp(v)) ≈ vfor small tangent vectors. The precision of 8 decimal places is appropriate for double-precision floating-point.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (14)
src/JitCompiler/IR/Operations/GradMobiusAddOp.cs (1)
24-30: Missing curvature validation for consistency with forward op.
MobiusAddOp.Validate()rejects non-negative curvature, butGradMobiusAddOpdoes not. Since the gradient computation depends on the curvature parameter, consider adding the same constraint for consistency.🔎 Proposed fix
public override bool Validate() { if (!base.Validate()) return false; // Inputs: x, y, gradient of output if (InputIds.Length != 3) return false; + // Curvature should be negative for hyperbolic space + if (Curvature >= 0) return false; return true; }src/JitCompiler/IR/Operations/GradPoincareExpMapOp.cs (1)
24-30: Missing curvature validation for consistency with forward op.Same issue as
GradMobiusAddOp: the corresponding forward operationPoincareExpMapOpvalidates that curvature is negative, but this gradient operation does not.🔎 Proposed fix
public override bool Validate() { if (!base.Validate()) return false; // Inputs: base point, tangent vector, gradient of output if (InputIds.Length != 3) return false; + // Curvature should be negative for hyperbolic space + if (Curvature >= 0) return false; return true; }src/JitCompiler/IR/Operations/GradSpMMOp.cs (1)
34-40: Consider adding dimension validation.The dimension properties (
SparseRows,SparseColumns,DenseColumns) are not validated. For consistency withSpMVOp(which validatesRows > 0 && Columns > 0), consider adding similar checks.🔎 Proposed fix
public override bool Validate() { if (!base.Validate()) return false; // Inputs: sparse matrix components, dense matrix, gradient of output if (InputIds.Length != 5) return false; + if (SparseRows <= 0 || SparseColumns <= 0 || DenseColumns <= 0) return false; return true; }src/JitCompiler/IR/Operations/SpMVOp.cs (1)
37-44: Consider validatingNonZeroCount.The
RowsandColumnsproperties are validated to be positive, butNonZeroCountis not checked. A negative value would be invalid. Zero might be acceptable for an empty sparse matrix, but negative values should likely be rejected.🔎 Proposed fix
public override bool Validate() { if (!base.Validate()) return false; // Inputs: sparse matrix (represented as row_ptr, col_idx, values), dense vector if (InputIds.Length != 4) return false; - if (Rows <= 0 || Columns <= 0) return false; + if (Rows <= 0 || Columns <= 0 || NonZeroCount < 0) return false; return true; }src/JitCompiler/IR/Operations/GradSpMVOp.cs (1)
29-35: Missing dimension validation for consistency with forward op.
SpMVOp.Validate()checksRows > 0 && Columns > 0, butGradSpMVOpdoes not validate these properties despite exposing them.🔎 Proposed fix
public override bool Validate() { if (!base.Validate()) return false; // Inputs: sparse matrix components, input vector, gradient of output if (InputIds.Length != 5) return false; + if (Rows <= 0 || Columns <= 0) return false; return true; }src/JitCompiler/IR/Operations/SpMMOp.cs (1)
42-48: Consider validating NonZeroCount for robustness.The validation checks dimension constraints but doesn't validate
NonZeroCount. While an empty sparse matrix (NonZeroCount = 0) might be valid in some contexts, consider addingNonZeroCount >= 0for consistency with other property validations, or document why it's unchecked.🔎 Proposed fix
public override bool Validate() { if (!base.Validate()) return false; // Inputs: sparse matrix (row_ptr, col_idx, values), dense matrix if (InputIds.Length != 4) return false; if (SparseRows <= 0 || SparseColumns <= 0 || DenseColumns <= 0) return false; + if (NonZeroCount < 0) return false; return true; }src/NeuralNetworks/Layers/SparseLinearLayer.cs (2)
143-182: Performance concern with HashSet collision at low sparsity.When
_sparsityis close to 0 (meaning most weights are non-zero), thewhileloop can become very slow due to hash collisions as the set fills up. For example, with 90% non-zeros on a 1000×1000 matrix, you'd need 900,000 unique pairs, and the rejection rate increases significantly as the set grows.Consider using Fisher-Yates shuffle on a linearized index array for better worst-case performance, or at minimum document that this layer is designed for high sparsity (≥0.5).
🔎 Alternative approach using shuffle
// More efficient approach for lower sparsity values var allIndices = Enumerable.Range(0, totalWeights).ToArray(); RandomHelper.Shuffle(allIndices, random); var selectedIndices = allIndices.Take(nonZeroCount) .Select(idx => (row: idx / InputFeatures, col: idx % InputFeatures)) .OrderBy(x => x.row).ThenBy(x => x.col) .ToList();
278-288: O(batchSize × OutputFeatures × NonZeroCount) complexity is inefficient.The inner loop scans all non-zero weights for each
(batch, output)pair just to find entries whererow == o. This results in O(B × O × NNZ) complexity, which can be very slow for large layers.Consider pre-grouping indices by row during initialization, or restructuring to iterate over non-zeros once per batch sample.
🔎 Example: Pre-group indices by row
Add a field during initialization:
private readonly int[][] _nonZeroIndicesByRow; // _nonZeroIndicesByRow[row] = array of nz indices for that rowThen in backward:
for (int o = 0; o < OutputFeatures; o++) { T gradOut = outputGradient[b, o]; _biasesGradient[o] = _numOps.Add(_biasesGradient[o], gradOut); foreach (int nz in _nonZeroIndicesByRow[o]) { int col = _weights.ColumnIndices[nz]; T inputVal = _lastInput.Rank == 1 ? _lastInput[col] : _lastInput[b, col]; var contrib = _numOps.Multiply(gradOut, inputVal); _weightsGradient[_weights.RowIndices[nz], col] = _numOps.Add(_weightsGradient[_weights.RowIndices[nz], col], contrib); } }src/JitCompiler/IR/Operations/GradOctonionMultiplyOp.cs (1)
24-30: Consider adding input type validation.The validation does not check whether the inputs are compatible types (e.g., octonion tensors). Type mismatches could lead to runtime errors during execution.
Consider validating input types if the IR framework provides type metadata, similar to other gradient operations in the codebase. This would catch type errors during IR construction rather than at execution time.
#!/bin/bash # Search for similar gradient operations to check if type validation is a common pattern rg -n -C3 --type=cs 'class Grad.*Op : IROp' -A 10 | rg -C2 'Validate|Type'src/NeuralNetworks/Layers/OctonionLinearLayer.cs (2)
127-141: Consider making the random seed configurable.Using a hardcoded seed (
42) ensures reproducibility during testing, but production scenarios may benefit from configurable or non-deterministic initialization. This is a minor point since the current approach is valid for development.
275-313: Consider extracting octonion scalar multiplication to a helper method.The pattern of multiplying each octonion component by a scalar is repeated for both weights and biases. This could be extracted into a helper method for clarity and maintainability.
🔎 Suggested helper method
private Octonion<T> ScaleOctonion(Octonion<T> oct, T scalar) { return new Octonion<T>( _numOps.Multiply(scalar, oct.Scalar), _numOps.Multiply(scalar, oct.E1), _numOps.Multiply(scalar, oct.E2), _numOps.Multiply(scalar, oct.E3), _numOps.Multiply(scalar, oct.E4), _numOps.Multiply(scalar, oct.E5), _numOps.Multiply(scalar, oct.E6), _numOps.Multiply(scalar, oct.E7)); }src/NeuralNetworks/Layers/HyperbolicLinearLayer.cs (3)
83-87: SimplifyParameterCountexpression.The expression
(OutputFeatures * InputFeatures) + (OutputFeatures * InputFeatures)can be simplified to2 * OutputFeatures * InputFeaturesfor clarity.🔎 Suggested fix
public override int ParameterCount => - (OutputFeatures * InputFeatures) + (OutputFeatures * InputFeatures); + 2 * OutputFeatures * InputFeatures;
193-226: Cache the origin vector outside the nested loops.
CreateOriginVector(InputFeatures)is called inside the double loop over batch and output features. Since it always returns the same zero vector, it can be created once before the loops.🔎 Suggested fix
+ // Create origin vector once (reused for all samples/outputs) + var origin = CreateOriginVector(InputFeatures); + // For each sample in batch for (int b = 0; b < batchSize; b++) { // ... (extract and project input) // For each output feature for (int o = 0; o < OutputFeatures; o++) { // ... (get weight and bias vectors) // Compute hyperbolic linear transformation: // 1. Apply exponential map from origin with weight as tangent vector - var origin = CreateOriginVector(InputFeatures); var weightPoint = _engine.PoincareExpMap(origin, weightVec, _curvature);
317-346: Bias update is inefficient: one ExpMap per component instead of per output.The inner loop creates a tangent vector with only one non-zero component (
j == i) and performs a fullPoincareExpMapfor each. This results inO(OutputFeatures * InputFeatures)exponential map operations. Consider computing the full tangent vector for all components at once and performing a single ExpMap per output feature row.🔎 Suggested optimization
for (int o = 0; o < OutputFeatures; o++) { + // Prepare full bias point and tangent vector for this output + var biasPoint = new Vector<T>(InputFeatures); + var tangentVec = new Vector<T>(InputFeatures); + for (int i = 0; i < InputFeatures; i++) + { + biasPoint[i] = _biases[o, i]; + tangentVec[i] = _numOps.Negate(_numOps.Multiply(learningRate, _biasesGradient[o, i])); + } + + // Single projection and update + var projectedBias = _engine.PoincareProject(biasPoint, _curvature, epsilon); + var updatedBias = _engine.PoincareExpMap(projectedBias, tangentVec, _curvature); + updatedBias = _engine.PoincareProject(updatedBias, _curvature, epsilon); + + for (int i = 0; i < InputFeatures; i++) + { + _biases[o, i] = updatedBias[i]; + } + + // Update weights normally for (int i = 0; i < InputFeatures; i++) { - // Update weights in tangent space var grad = _weightsGradient[o, i]; var scaledGrad = _numOps.Multiply(learningRate, grad); _weights[o, i] = _numOps.Subtract(_weights[o, i], scaledGrad); - - // Update biases using Riemannian gradient descent - // ... (remove per-component ExpMap) } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
src/Enums/OperationType.cssrc/JitCompiler/IR/Operations/GeometricProductOp.cssrc/JitCompiler/IR/Operations/GradGeometricProductOp.cssrc/JitCompiler/IR/Operations/GradMobiusAddOp.cssrc/JitCompiler/IR/Operations/GradOctonionMultiplyOp.cssrc/JitCompiler/IR/Operations/GradPoincareExpMapOp.cssrc/JitCompiler/IR/Operations/GradSpMMOp.cssrc/JitCompiler/IR/Operations/GradSpMVOp.cssrc/JitCompiler/IR/Operations/MobiusAddOp.cssrc/JitCompiler/IR/Operations/OctonionMatMulOp.cssrc/JitCompiler/IR/Operations/OctonionMultiplyOp.cssrc/JitCompiler/IR/Operations/PoincareExpMapOp.cssrc/JitCompiler/IR/Operations/PoincareLogMapOp.cssrc/JitCompiler/IR/Operations/SpMMOp.cssrc/JitCompiler/IR/Operations/SpMVOp.cssrc/JitCompiler/IR/Operations/WedgeProductOp.cssrc/NeuralNetworks/Layers/HyperbolicLinearLayer.cssrc/NeuralNetworks/Layers/OctonionLinearLayer.cssrc/NeuralNetworks/Layers/SparseLinearLayer.cstests/AiDotNet.Tests/UnitTests/JitCompiler/AdvancedAlgebraIROpTests.cstests/AiDotNet.Tests/UnitTests/Layers/AdvancedAlgebraLayerTests.cs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
src/JitCompiler/IR/Operations/GradPoincareExpMapOp.cssrc/JitCompiler/IR/Operations/GradSpMMOp.cssrc/JitCompiler/IR/Operations/OctonionMatMulOp.cssrc/JitCompiler/IR/Operations/GradSpMVOp.cssrc/JitCompiler/IR/Operations/WedgeProductOp.cssrc/JitCompiler/IR/Operations/GradGeometricProductOp.cssrc/JitCompiler/IR/Operations/SpMVOp.cssrc/JitCompiler/IR/Operations/GradMobiusAddOp.cssrc/JitCompiler/IR/Operations/PoincareLogMapOp.cssrc/JitCompiler/IR/Operations/MobiusAddOp.cssrc/JitCompiler/IR/Operations/GeometricProductOp.cssrc/JitCompiler/IR/Operations/OctonionMultiplyOp.cssrc/JitCompiler/IR/Operations/SpMMOp.cssrc/NeuralNetworks/Layers/OctonionLinearLayer.cssrc/JitCompiler/IR/Operations/PoincareExpMapOp.cssrc/NeuralNetworks/Layers/HyperbolicLinearLayer.cssrc/JitCompiler/IR/Operations/GradOctonionMultiplyOp.cssrc/Enums/OperationType.cssrc/NeuralNetworks/Layers/SparseLinearLayer.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
src/JitCompiler/IR/Operations/GradPoincareExpMapOp.cssrc/JitCompiler/IR/Operations/GradSpMMOp.cssrc/JitCompiler/IR/Operations/OctonionMatMulOp.cssrc/JitCompiler/IR/Operations/GradSpMVOp.cssrc/JitCompiler/IR/Operations/WedgeProductOp.cssrc/JitCompiler/IR/Operations/GradGeometricProductOp.cssrc/JitCompiler/IR/Operations/SpMVOp.cssrc/JitCompiler/IR/Operations/GradMobiusAddOp.cssrc/JitCompiler/IR/Operations/PoincareLogMapOp.cssrc/JitCompiler/IR/Operations/MobiusAddOp.cssrc/JitCompiler/IR/Operations/GeometricProductOp.cssrc/JitCompiler/IR/Operations/OctonionMultiplyOp.cssrc/JitCompiler/IR/Operations/SpMMOp.cssrc/NeuralNetworks/Layers/OctonionLinearLayer.cssrc/JitCompiler/IR/Operations/PoincareExpMapOp.cssrc/NeuralNetworks/Layers/HyperbolicLinearLayer.cssrc/JitCompiler/IR/Operations/GradOctonionMultiplyOp.cssrc/Enums/OperationType.cssrc/NeuralNetworks/Layers/SparseLinearLayer.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL Analysis
- GitHub Check: Build (Windows)
🔇 Additional comments (25)
src/JitCompiler/IR/Operations/OctonionMultiplyOp.cs (1)
17-29: LGTM!The validation correctly enforces 16 inputs (8 components per octonion). The documentation clearly explains the non-associative nature of octonion multiplication and provides a helpful beginner-friendly explanation.
src/JitCompiler/IR/Operations/MobiusAddOp.cs (1)
17-36: LGTM!The implementation correctly validates the curvature constraint for hyperbolic space and enforces the two-input requirement. The mathematical formula in the documentation provides useful reference.
src/JitCompiler/IR/Operations/PoincareExpMapOp.cs (1)
16-34: LGTM!The exponential map operation correctly validates both the input count (base point and tangent vector) and the curvature constraint. Documentation clearly explains the geometric role in hyperbolic optimization.
src/JitCompiler/IR/Operations/OctonionMatMulOp.cs (1)
1-45: LGTM! Clean IR operation implementation.The validation logic correctly enforces positive dimensions and the expected two-input structure for octonion matrix multiplication. Documentation is clear and beginner-friendly.
src/JitCompiler/IR/Operations/PoincareLogMapOp.cs (1)
1-36: LGTM! Correct validation for hyperbolic geometry.The curvature validation correctly enforces negative curvature (line 33:
Curvature >= 0returns false), which is mathematically appropriate for hyperbolic space. The two-input requirement for base and target points is correct.src/JitCompiler/IR/Operations/WedgeProductOp.cs (1)
1-44: LGTM! Consistent validation for geometric algebra operations.The validation correctly enforces non-negative signature counts and the two-input structure for wedge products. The implementation is consistent with related geometric algebra operations in the PR.
src/Enums/OperationType.cs (1)
398-522: LGTM! Comprehensive operation type taxonomy.The new enum members are well-documented, logically categorized, and align with the IR operations introduced in this PR. The naming is consistent and the XML documentation clearly describes each operation type.
src/NeuralNetworks/Layers/SparseLinearLayer.cs (6)
327-359: LGTM!The parameter update correctly maintains the sparsity pattern by only updating non-zero weight positions. The gradient descent formula
w = w - lr * gradis properly implemented.
365-383: LGTM!Correctly serializes non-zero weights followed by biases, consistent with
ParameterCount.
389-417: LGTM!Symmetric with
GetParameters, correctly validates parameter count and restores weights/biases in the same order.
422-428: LGTM!Properly clears all cached state for reuse.
436-441: LGTM!Correctly throws
NotSupportedExceptionwith a clear message, consistent withSupportsJitCompilationreturningfalse.
446-457: LGTM!Simple and correct matrix transpose implementation.
src/JitCompiler/IR/Operations/GradOctonionMultiplyOp.cs (1)
18-31: Verify execution model and consistency with other gradient ops.The class provides only validation logic without a visible
Executemethod. Ensure this follows the established pattern for IR operations in the JIT compiler—check whetherIROprequires anExecutemethod or if execution is handled by an interpreter, whether other gradient operations (e.g.,GradGeometricProductOp,GradSpMVOp) follow the same pattern, and how this operation will be invoked during backpropagation.src/NeuralNetworks/Layers/OctonionLinearLayer.cs (4)
31-95: LGTM!Class structure, fields, and properties are well-designed. The
ParameterCountformula correctly accounts for 8 components per octonion for both weights and biases.
168-204: LGTM!Forward pass correctly validates input shape, performs octonion matrix multiplication, adds biases, and applies activation. Caching inputs/outputs for backpropagation is properly implemented.
319-391: LGTM!
GetParametersandSetParameterscorrectly serialize/deserialize octonion parameters with consistent ordering. The index increment of 8 per octonion aligns with the 8-component structure.
420-469: LGTM!The tensor-to-octonion and octonion-to-tensor conversion methods are correctly implemented with symmetric packing/unpacking of the 8 components per octonion.
src/NeuralNetworks/Layers/HyperbolicLinearLayer.cs (4)
107-133: LGTM!Constructor properly validates negative curvature requirement for hyperbolic space and initializes the layer with appropriate dependencies.
253-256: Gradient approximation is documented but may limit training quality.The comment correctly states this is a "first-order approximation valid for small curvature." For production use with significant curvature values, consider implementing proper Riemannian gradients to improve training stability and convergence.
353-420: LGTM!
GetParameters,SetParameters, andResetStateare correctly implemented with consistent ordering and proper validation.
438-446: LGTM!The
CreateOriginVectorhelper correctly creates a zero vector for the hyperbolic origin.src/JitCompiler/IR/Operations/GeometricProductOp.cs (3)
1-17: LGTM! Clear documentation.The class declaration and documentation are well-structured. The beginner-friendly remarks align with the PR's educational objectives.
38-46: Consider validating OutputIds — verify if output validation is necessary.The
Validate()method checksInputIdsbut doesn't validateOutputIds. For a geometric product operation, confirm whether output validation should be added (e.g., expecting exactly one output tensor) and whether this is a standard pattern in the IR operation implementations.
19-32: Verify property mutability pattern against existing IR operations.The signature properties use public setters, allowing mutation after construction. Additionally, without a constructor, they default to (0,0,0).
Please confirm:
- Does the mutability pattern match other
IROpimplementations in the codebase?- Is (0,0,0) a valid default signature for a geometric product, or should a constructor enforce explicit initialization?
In many IR designs, operation properties are immutable after construction to prevent accidental modification.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
src/NeuralNetworks/Layers/SparseLinearLayer.cs (1)
197-206: Missing input size validation for 1D case.The 2D branch validates
inputLen != InputFeatures(line 212), but the 1D branch copies input values without validating thatinput.Shape[0] == InputFeatures. This could cause index-out-of-bounds or incorrect results.🔎 Proposed fix
if (wasSingleSample) { // Single sample batchSize = 1; + if (input.Shape[0] != InputFeatures) + { + throw new ArgumentException( + $"Input size {input.Shape[0]} does not match expected {InputFeatures}."); + } inputMatrix = new Matrix<T>(1, InputFeatures); for (int i = 0; i < InputFeatures; i++) { inputMatrix[0, i] = input[i]; } }src/AiDotNet.Tensors/Manifolds/HyperboloidManifold.cs (1)
79-89: Inconsistent curvature scaling in Distance method.
ExpMap,LogMap, andParallelTransportscale input vectors bysqrt(Curvature)before computing, butDistanceuses unscaled vectors forMinkowskiDotand then multiplies byCurvatureafterward (line 84). This inconsistency may produce incorrect results for non-unit curvature.🔎 Suggested fix to align with other methods
public T Distance(Vector<T> x, Vector<T> y) { EnsureSameLength(x, y); - T dot = MinkowskiDot(x, y); - T scaled = _ops.Multiply(Curvature, dot); - double alpha = -Convert.ToDouble(scaled); + T sqrtC = _ops.Sqrt(Curvature); + var xScaled = Scale(x, sqrtC); + var yScaled = Scale(y, sqrtC); + + T dot = MinkowskiDot(xScaled, yScaled); + double alpha = -Convert.ToDouble(dot); alpha = Math.Max(alpha, 1.0); double dist = Acosh(alpha); return _ops.Divide(_ops.FromDouble(dist), _ops.Sqrt(Curvature)); }src/NeuralNetworks/Layers/OctonionLinearLayer.cs (1)
538-543: Activation gradient handling is incomplete for non-linear activations.
ComputeActivationGradientalways returns the gradient unchanged, which is only correct for identity/linear activation. If a non-linear activation function is provided to the constructor, the backward pass will produce incorrect gradients.🔎 Suggested approach
Either restrict the constructor to only accept identity activation, or implement proper activation derivative computation:
private Tensor<T> ComputeActivationGradient(Tensor<T> outputGradient, Octonion<T>[,] preActivation) { - // For linear activation, gradient passes through unchanged - // For other activations, would need to compute derivative - return outputGradient; + // Convert preActivation to tensor for activation derivative + int batchSize = preActivation.GetLength(0); + int features = preActivation.GetLength(1); + var preActTensor = OctonionsToTensor(preActivation, batchSize, features); + + // Apply activation derivative element-wise + var activationDerivative = ActivationFunction.Derivative(preActTensor); + + // Element-wise multiply gradient with derivative + var result = new Tensor<T>(outputGradient.Shape); + for (int i = 0; i < outputGradient.Shape[0]; i++) + { + for (int j = 0; j < outputGradient.Shape[1]; j++) + { + result[i, j] = _numOps.Multiply(outputGradient[i, j], activationDerivative[i, j]); + } + } + return result; }
🧹 Nitpick comments (1)
src/NeuralNetworks/Layers/HyperbolicLinearLayer.cs (1)
368-397: Bias update applies geodesic step per-element rather than as a vector.The inner loop creates a tangent vector with only a single non-zero component (at index
j == i) and applies the full exponential map. This performsInputFeaturesexponential maps per output feature, each moving in a single coordinate direction. A more efficient approach would accumulate the tangent vector and apply one exponential map per bias row.🔎 Potential optimization
for (int o = 0; o < OutputFeatures; o++) { + // Accumulate full tangent vector for this bias row + var biasPoint = new Vector<T>(InputFeatures); + var tangentVec = new Vector<T>(InputFeatures); for (int i = 0; i < InputFeatures; i++) { - // Update weights in tangent space - var grad = _weightsGradient[o, i]; - var scaledGrad = _numOps.Multiply(learningRate, grad); - _weights[o, i] = _numOps.Subtract(_weights[o, i], scaledGrad); - - // Update biases using Riemannian gradient descent - var biasPoint = new Vector<T>(InputFeatures); - var tangentVec = new Vector<T>(InputFeatures); - for (int j = 0; j < InputFeatures; j++) - { - biasPoint[j] = _biases[o, j]; - tangentVec[j] = j == i ? _numOps.Negate(_numOps.Multiply(learningRate, _biasesGradient[o, j])) : _numOps.Zero; - } - - var projectedBias = _engine.PoincareProject(biasPoint, _curvature, epsilon); - var updatedBias = _engine.PoincareExpMap(projectedBias, tangentVec, _curvature); - updatedBias = _engine.PoincareProject(updatedBias, _curvature, epsilon); - - for (int j = 0; j < InputFeatures; j++) - { - _biases[o, j] = updatedBias[j]; - } + biasPoint[i] = _biases[o, i]; + tangentVec[i] = _numOps.Negate(_numOps.Multiply(learningRate, _biasesGradient[o, i])); + + // Update weights in tangent space + var grad = _weightsGradient[o, i]; + var scaledGrad = _numOps.Multiply(learningRate, grad); + _weights[o, i] = _numOps.Subtract(_weights[o, i], scaledGrad); } + + // Single exponential map for the full tangent vector + var projectedBias = _engine.PoincareProject(biasPoint, _curvature, epsilon); + var updatedBias = _engine.PoincareExpMap(projectedBias, tangentVec, _curvature); + updatedBias = _engine.PoincareProject(updatedBias, _curvature, epsilon); + + for (int j = 0; j < InputFeatures; j++) + { + _biases[o, j] = updatedBias[j]; + } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/AiDotNet.Tensors/Manifolds/HyperboloidManifold.cssrc/NeuralNetworks/HyperbolicNeuralNetwork.cssrc/NeuralNetworks/Layers/HyperbolicLinearLayer.cssrc/NeuralNetworks/Layers/OctonionLinearLayer.cssrc/NeuralNetworks/Layers/SparseLinearLayer.cssrc/NeuralNetworks/OctonionNeuralNetwork.cssrc/NeuralNetworks/SparseNeuralNetwork.cstests/AiDotNet.Tests/UnitTests/NeuralNetworks/AdvancedAlgebraNetworkTests.cs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
src/NeuralNetworks/HyperbolicNeuralNetwork.cssrc/AiDotNet.Tensors/Manifolds/HyperboloidManifold.cssrc/NeuralNetworks/OctonionNeuralNetwork.cssrc/NeuralNetworks/Layers/SparseLinearLayer.cssrc/NeuralNetworks/Layers/HyperbolicLinearLayer.cssrc/NeuralNetworks/SparseNeuralNetwork.cssrc/NeuralNetworks/Layers/OctonionLinearLayer.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
src/NeuralNetworks/HyperbolicNeuralNetwork.cssrc/AiDotNet.Tensors/Manifolds/HyperboloidManifold.cssrc/NeuralNetworks/OctonionNeuralNetwork.cssrc/NeuralNetworks/Layers/SparseLinearLayer.cssrc/NeuralNetworks/Layers/HyperbolicLinearLayer.cssrc/NeuralNetworks/SparseNeuralNetwork.cssrc/NeuralNetworks/Layers/OctonionLinearLayer.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL Analysis
- GitHub Check: Build (Windows)
🔇 Additional comments (11)
src/NeuralNetworks/HyperbolicNeuralNetwork.cs (3)
1-82: Well-structured constructor with proper validation.The constructor correctly validates negative curvature and provides sensible defaults for optimizer and loss function. The documentation is comprehensive and beginner-friendly.
84-119: Layer initialization logic is correct.The
InitializeLayersmethod properly handles both custom layers and auto-generated layer stacks. The hidden layer iteration and edge cases are handled correctly.
219-245: Training loop correctly aggregates auxiliary losses.The training method properly handles primary loss computation, auxiliary loss aggregation from layers implementing
IAuxiliaryLossLayer<T>, and gradient propagation.src/NeuralNetworks/Layers/SparseLinearLayer.cs (2)
268-352: Backward pass correctly handles 1D/2D tensor shapes.The backward pass now properly converts 1D gradients to 2D for processing and handles the input gradient computation with correct indexing for both tensor ranks. The sparse gradient computation only updates non-zero weight positions as intended.
143-182: Sparse weight initialization is well-implemented.The Xavier/Glorot scaling adapted for sparse initialization, random index selection via HashSet, and COO format construction are all correct. The sorted ordering ensures consistent sparse tensor structure.
src/AiDotNet.Tensors/Manifolds/HyperboloidManifold.cs (1)
35-77: ExpMap and LogMap implementations are mathematically sound.The exponential and logarithmic map implementations correctly handle edge cases (zero norm tangent, near-identity points) and use appropriate scaling for the curvature parameter. The Minkowski inner product formulation is correct for the hyperboloid model.
src/NeuralNetworks/Layers/OctonionLinearLayer.cs (1)
283-319: Octonion gradient computation uses correct conjugate formulation.The weight gradient computation correctly uses the conjugate of the gradient times input, and the input gradient uses the conjugate of weights. This is the proper formulation for non-commutative octonion algebra backpropagation.
src/NeuralNetworks/Layers/HyperbolicLinearLayer.cs (2)
161-265: Forward pass correctly handles 1D and 2D input tensors.The implementation now clearly distinguishes single-sample (1D) from batch (2D) inputs using the
wasSingleSampleflag. Input validation, Poincaré ball projection, and hyperbolic linear transformation are correctly applied. The output shape matches the input rank.
304-349: Gradient approximation is documented as simplified.The backward pass explicitly notes it uses a "first-order approximation valid for small curvature" rather than full Riemannian gradients. This is acceptable for initial implementation but may affect training quality for large curvature values.
Consider adding a TODO or documenting the approximation's limitations in the class-level remarks. Users should be aware this may not converge well for strongly curved spaces.
src/NeuralNetworks/SparseNeuralNetwork.cs (2)
67-84: Constructor correctly validates sparsity range and initializes components.The sparsity validation
[0, 1)is appropriate, and the fallback to Adam optimizer and MSE loss function follows established patterns in the codebase.
86-121: Layer initialization correctly propagates sparsity to all layers.The
InitializeLayersmethod properly passes the configured sparsity to eachSparseLinearLayer<T>, ensuring consistent sparsity across the network when auto-generating layers.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/NeuralNetworks/SparseNeuralNetwork.cs (1)
287-300: Optimizer and loss function types are still not restored during deserialization.While
_sparsityis now properly restored (line 289), theoptimizerTypeandlossFunctionTypeare still read but discarded (lines 298-299). This breaks model restoration fidelity—a deserialized model will use the optimizer and loss function provided during construction rather than restoring the originally serialized configuration.Consider implementing a type activation pattern to reconstruct the optimizer and loss function instances from their type names, similar to other network implementations in the codebase.
src/NeuralNetworks/Layers/OctonionLinearLayer.cs (1)
245-278: Activation gradients now correctly respect non-linear activationsThe updated backward path and
ComputeActivationGradientcorrectly:
- Use cached pre-activation outputs (
_lastOutput) to build a tensor,- Compute the activation derivative via
DerivativeTensor(ScalarActivation, ...), and- Apply it element-wise to the upstream gradient before converting to octonions.
This fixes the previous behavior where gradients were effectively assuming an identity activation, and keeps identity/no-activation as a fast path.
Also applies to: 538-566
🧹 Nitpick comments (10)
src/AiDotNet.Tensors/Groups/Su2.cs (1)
45-61: Consider making tolerance configurable or type-dependent.The hardcoded
1e-6tolerance works for typical cases but may be too strict forfloator too loose for higher-precision types. Consider parameterizing tolerance based on the numeric type or exposing it as a configuration option.🔎 Potential approach: type-dependent tolerance
private static void ValidateUnitQuaternion(T w, T x, T y, T z) { - const double tolerance = 1e-6; + double tolerance = typeof(T) == typeof(float) ? 1e-5 : 1e-6; double wD = NumOps.ToDouble(w); double xD = NumOps.ToDouble(x); double yD = NumOps.ToDouble(y); double zD = NumOps.ToDouble(z); double normSquared = wD * wD + xD * xD + yD * yD + zD * zD; if (Math.Abs(normSquared - 1.0) > tolerance) { throw new ArgumentException( $"Quaternion must be unit length for SU(2). Got norm² = {normSquared}, expected 1.0."); } }src/AiDotNet.Tensors/LinearAlgebra/CliffordAlgebra.cs (1)
152-161: Consider usingBitOperations.PopCountfor efficiency.While the current implementation is correct,
System.Numerics.BitOperations.PopCountis hardware-accelerated and would be slightly more efficient. Since the grades are pre-computed at construction time, the impact is minimal.🔎 Suggested refactor
+using System.Numerics; + // Replace CountBits method with: - private static int CountBits(int value) - { - int count = 0; - while (value != 0) - { - count += value & 1; - value >>= 1; - } - return count; - } + private static int CountBits(int value) => BitOperations.PopCount((uint)value);src/AiDotNet.Tensors/LinearAlgebra/Multivector.cs (3)
86-95: ClarifyMagnitudesemantics in documentation.The current implementation computes the Euclidean norm of coefficients (√Σcᵢ²). In geometric algebra, the "magnitude" or "norm" is typically defined as √|v·ṽ| where ṽ is the reverse. Consider adding XML documentation to clarify this is the coefficient norm, not the geometric algebra norm, to avoid confusion for users familiar with GA conventions.
🔎 Suggested documentation
+ /// <summary> + /// Gets the Euclidean norm of the coefficient vector (√Σcᵢ²). + /// </summary> + /// <remarks> + /// This is the L2 norm of the coefficient array, not the geometric algebra norm. + /// For the GA norm, use <c>GeometricProduct(Reverse()).Scalar</c> and take the square root. + /// </remarks> public T Magnitude
184-220: Document which inner product variant is implemented.This implements the left contraction (A ⌋ B), which requires grade(result) = grade(B) - grade(A). Since geometric algebra has multiple inner product definitions (left contraction, right contraction, Hestenes inner product, etc.), consider adding XML documentation to clarify the semantics.
🔎 Suggested documentation
+ /// <summary> + /// Computes the left contraction (A ⌋ B) of this multivector with another. + /// </summary> + /// <remarks> + /// The left contraction is defined such that the result has grade(B) - grade(A). + /// Terms where grade(A) > grade(B) or where the grade condition is not met are excluded. + /// </remarks> public Multivector<T> InnerProduct(Multivector<T> other)
245-267: Add null checks to operator overloads.The operators don't validate null operands, which will throw
NullReferenceExceptioninstead ofArgumentNullException. Consider adding explicit null checks for better error messages.🔎 Suggested fix
public static Multivector<T> operator +(Multivector<T> left, Multivector<T> right) { + ArgumentNullException.ThrowIfNull(left); + ArgumentNullException.ThrowIfNull(right); left.EnsureSameAlgebra(right); // ... rest unchanged } public static Multivector<T> operator -(Multivector<T> left, Multivector<T> right) { + ArgumentNullException.ThrowIfNull(left); + ArgumentNullException.ThrowIfNull(right); left.EnsureSameAlgebra(right); // ... rest unchanged }src/NeuralNetworks/Layers/GraphConvolutionalLayer.cs (1)
263-267: Parameter/gradient API wiring looks consistent with layer internals
ParameterCount,GetParameterGradients, andClearGradientsalign with how_weights/_biasand their gradients are shaped and populated (both in manual and autodiff backprop). Returning a zero‑initialized vector when gradients are null is a reasonable default for “no grads yet”; just be aware this hides misuse vs throwing if a caller expects grads only afterBackward.Also applies to: 919-949
src/AiDotNet.Tensors/LinearAlgebra/SparseTensor.cs (1)
28-52: Consider strengthening index and tolerance validation in sparse constructionThe core COO/CSR/CSC logic looks correct, but two edge cases are currently unchecked:
SparseTensor(int rows, int columns, int[] rowIndices, int[] columnIndices, T[] values)assumes all indices are within[0, rows)/[0, columns). If a caller passes out‑of‑range indices, you’ll end up with silent corruption later. A cheap pass over the indices here (or in a debug build) would fail fast.FromDense(Tensor<T> dense, T tolerance)accepts negativetolerance. Withtolerance < 0,Abs(value) <= toleranceis never true, so even exact zeros are treated as non‑zero and stored, which is surprising for a “tolerance” parameter. Guardingtolerance >= 0(or documenting the behavior) would avoid misuse.These are not correctness bugs with well‑behaved callers, but tightening validation would make the API more robust against accidental misuse.
Also applies to: 115-141
src/NeuralNetworks/HyperbolicNeuralNetwork.cs (2)
223-245: Auxiliary losses currently don’t influence gradients
Traincorrectly aggregatesComputeAuxiliaryLoss()intoLastLoss, but the backward pass uses only_lossFunction.CalculateDerivativeforpredictionvsexpectedOutput. No gradient from auxiliary losses is ever propagated into the layers.If the intent is for auxiliary losses (e.g., graph smoothness) to regularize training, you’ll need to either:
- Have
IAuxiliaryLossLayer<T>update its own gradients whenComputeAuxiliaryLoss()is called, or- Include auxiliary-loss derivatives explicitly in the gradient computation path.
Right now auxiliary loss is effectively for monitoring, not optimization.
303-311: Deep copies reuse the same optimizer instance
CreateNewInstancepasses_optimizerdirectly into the newHyperbolicNeuralNetwork<T>. If the optimizer maintains internal state (e.g., Adam moments), the original network and its deep copy will share that state, which is usually undesirable.Consider:
- Passing only configuration into the constructor and letting it create a fresh optimizer, or
- Introducing an
IGradientBasedOptimizer.Clone()/factory so each network gets its own instance.src/NeuralNetworks/OctonionNeuralNetwork.cs (1)
262-297: Deep copies and serialization should avoid sharing optimizer instances
CreateNewInstancepasses the existing_optimizerand_lossFunctioninto the newOctonionNeuralNetwork<T>. As with the hyperbolic network, this risks shared optimizer state between the original and its clone.Consider creating new optimizer/loss instances (based on stored config or type names written in
SerializeNetworkSpecificData) so that each model instance has independent training state.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
src/AiDotNet.Tensors/Engines/CpuHyperbolicManifoldEngine.cssrc/AiDotNet.Tensors/Groups/Se3.cssrc/AiDotNet.Tensors/Groups/Se3Group.cssrc/AiDotNet.Tensors/Groups/So3.cssrc/AiDotNet.Tensors/Groups/Su2.cssrc/AiDotNet.Tensors/LinearAlgebra/CliffordAlgebra.cssrc/AiDotNet.Tensors/LinearAlgebra/Multivector.cssrc/AiDotNet.Tensors/LinearAlgebra/SparseTensor.cssrc/AiDotNet.Tensors/Manifolds/HyperboloidManifold.cssrc/AiDotNet.Tensors/NumericOperations/OctonionOperations.cssrc/JitCompiler/IR/IROp.cssrc/JitCompiler/IR/Operations/GradGeometricProductOp.cssrc/JitCompiler/IR/Operations/GradOctonionMultiplyOp.cssrc/NeuralNetworks/HyperbolicNeuralNetwork.cssrc/NeuralNetworks/Layers/GraphConvolutionalLayer.cssrc/NeuralNetworks/Layers/OctonionLinearLayer.cssrc/NeuralNetworks/Layers/SparseLinearLayer.cssrc/NeuralNetworks/NeuralNetworkBase.cssrc/NeuralNetworks/OctonionNeuralNetwork.cssrc/NeuralNetworks/SparseNeuralNetwork.cs
🚧 Files skipped from review as they are similar to previous changes (5)
- src/AiDotNet.Tensors/Groups/Se3.cs
- src/JitCompiler/IR/Operations/GradGeometricProductOp.cs
- src/AiDotNet.Tensors/Groups/Se3Group.cs
- src/JitCompiler/IR/Operations/GradOctonionMultiplyOp.cs
- src/AiDotNet.Tensors/NumericOperations/OctonionOperations.cs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
src/AiDotNet.Tensors/Manifolds/HyperboloidManifold.cssrc/NeuralNetworks/OctonionNeuralNetwork.cssrc/NeuralNetworks/NeuralNetworkBase.cssrc/AiDotNet.Tensors/LinearAlgebra/SparseTensor.cssrc/NeuralNetworks/Layers/GraphConvolutionalLayer.cssrc/JitCompiler/IR/IROp.cssrc/AiDotNet.Tensors/Groups/Su2.cssrc/NeuralNetworks/Layers/OctonionLinearLayer.cssrc/AiDotNet.Tensors/Engines/CpuHyperbolicManifoldEngine.cssrc/NeuralNetworks/Layers/SparseLinearLayer.cssrc/NeuralNetworks/HyperbolicNeuralNetwork.cssrc/AiDotNet.Tensors/Groups/So3.cssrc/AiDotNet.Tensors/LinearAlgebra/Multivector.cssrc/NeuralNetworks/SparseNeuralNetwork.cssrc/AiDotNet.Tensors/LinearAlgebra/CliffordAlgebra.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
src/AiDotNet.Tensors/Manifolds/HyperboloidManifold.cssrc/NeuralNetworks/OctonionNeuralNetwork.cssrc/NeuralNetworks/NeuralNetworkBase.cssrc/AiDotNet.Tensors/LinearAlgebra/SparseTensor.cssrc/NeuralNetworks/Layers/GraphConvolutionalLayer.cssrc/JitCompiler/IR/IROp.cssrc/AiDotNet.Tensors/Groups/Su2.cssrc/NeuralNetworks/Layers/OctonionLinearLayer.cssrc/AiDotNet.Tensors/Engines/CpuHyperbolicManifoldEngine.cssrc/NeuralNetworks/Layers/SparseLinearLayer.cssrc/NeuralNetworks/HyperbolicNeuralNetwork.cssrc/AiDotNet.Tensors/Groups/So3.cssrc/AiDotNet.Tensors/LinearAlgebra/Multivector.cssrc/NeuralNetworks/SparseNeuralNetwork.cssrc/AiDotNet.Tensors/LinearAlgebra/CliffordAlgebra.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL Analysis
- GitHub Check: Build (Windows)
🔇 Additional comments (29)
src/JitCompiler/IR/IROp.cs (4)
32-54: LGTM!The
OutputIdsproperty is well-designed for multi-output support. UsingArray.Empty<int>()as the default is memory-efficient, and the documentation clearly explains both single and multi-output scenarios.
68-72: Verify the destructive setter behavior is intentional.The
OutputIdsetter replaces the entireOutputIdsarray. For multi-output operations, this silently discards secondary outputs:op.OutputIds = [1, 2, 3]; // Multi-output gradient op op.OutputId = 5; // Now OutputIds = [5] — outputs 2 and 3 are lostIf this is intentional (as documented), consider whether throwing an
InvalidOperationExceptionwhenOutputIds.Length > 1would be safer to prevent accidental misuse.
186-204: LGTM!The validation logic correctly handles the multi-output scenario by checking that
OutputIdsis non-empty and all IDs are non-negative. The null-check ordering is correct.
225-230: LGTM!The
ToString()format now correctly represents multi-output operations (e.g.,t3, t4 = Gradient(t0, t1)). Single-output operations remain visually consistent.src/AiDotNet.Tensors/Groups/So3.cs (5)
14-14: LGTM: NumOps field follows standard pattern.The static readonly NumericOperations field is correctly initialized and follows the project's established pattern for generic numeric type handling.
16-16: LGTM: Property correctly enforces immutability.The get-only Matrix property appropriately enforces immutability for the readonly struct.
18-36: LGTM: Constructor validation addresses previous concern.The constructor now properly validates rotation matrix properties when
validateis true, addressing the previous review comment about missing SO(3) validation. The optional parameter allows trusted code paths (likeIdentity) to skip validation for performance.
85-85: LGTM: Identity property correctly optimizes validation.The
Identityproperty correctly skips validation since the identity matrix is guaranteed to be a valid SO(3) rotation matrix, providing a performance optimization for a common case.
41-83: Hardcoded tolerance and precision loss for generic type T.The validation method has two major concerns for a generic numeric type:
Fixed 1e-6 tolerance: This value is inappropriate across different numeric types:
- Too strict for
float(machine epsilon ~1e-7)- Too lenient for
decimal(28-29 significant digits) or other high-precision types- May cause false positives rejecting valid float rotations or false negatives accepting invalid decimal rotations
Precision loss: Converting all elements to
doubleviaNumOps.ToDouble()defeats the purpose of using high-precision types likedecimal, losing the very precision guarantees users expect.Consider making tolerance type-aware (e.g.,
NumOps.FromDouble(1e-6)scaled by type characteristics) or exposing a tolerance parameter in the constructor. For comparisons, prefer working in typeTthroughout when possible.src/AiDotNet.Tensors/Groups/Su2.cs (3)
29-40: Validation added—addresses prior review concern.The constructor now validates unit quaternion invariants by default, which resolves the missing validation flagged in the previous review. The optional
validateparameter allows bypassing checks for known-valid instances (e.g., Identity), which is a reasonable performance optimization.
66-66: LGTM!The Identity element is mathematically correct (quaternion identity is 1 + 0i + 0j + 0k), and skipping validation for this known-valid constant is appropriate.
12-67: Default struct constructor creates invalid SU(2) elements.As a C# struct,
Su2<T>has an implicit parameterless constructor that initializes all fields to their default values (all zeros). This creates an invalid unit quaternion with norm² = 0, violating the SU(2) invariant. While the explicit constructor validates,default(Su2<T>)or uninitialized struct instances bypass this check entirely.Verify whether default construction of
Su2<T>occurs in practice (e.g., array initialization, field defaults, collection operations). If found, consider defensive coding patterns or documentation warnings.src/AiDotNet.Tensors/LinearAlgebra/CliffordAlgebra.cs (6)
1-25: LGTM! Class declaration and Default property are well-designed.The
Defaultproperty is now correctly read-only, addressing the previous thread-safety concern. The class is properly sealed and implementsIEquatable<CliffordAlgebra>.
27-60: LGTM! Constructor validation and initialization are correct.The signature validation correctly ensures non-negative components, positive dimension, and the 30-dimension limit for 32-bit blade indices. The metric and grade arrays are properly initialized.
62-76: LGTM! Accessor methods have proper validation.Both
GetGradeandGetMetricSigncorrectly validate their inputs and return pre-computed values efficiently.
78-93: LGTM! Reordering sign algorithm is correct.The algorithm correctly computes the sign factor resulting from reordering basis vectors in the geometric product by counting parity of necessary transpositions.
95-126: LGTM! Blade multiplication logic is correct.The method correctly implements geometric algebra blade multiplication: handling zero-metric annihilation, negative metric sign flips, and computing the result blade via XOR.
128-150: LGTM! Equality and hashing implementations are correct.Value-based equality on (P, Q, R) is appropriate for the algebra signature, and the hash code follows standard practices.
src/AiDotNet.Tensors/LinearAlgebra/Multivector.cs (8)
1-43: LGTM! Constructors are well-designed with proper null validation.The constructor overloading pattern is clean, and the private constructor ensures all dependencies are validated.
45-58: LGTM! Indexer is now correctly immutable.The previous concern about the mutable setter has been addressed. The indexer is now get-only with proper blade validation.
97-123: LGTM! Negate, Scale, and Reverse operations are correct.The Reverse implementation correctly applies the sign factor
(-1)^(k(k-1)/2)for grade-k blades.
125-152: LGTM! Geometric product implementation is correct.The algorithm correctly iterates over all blade pairs, applies the algebra's multiplication rules via
TryMultiplyBlades, and accumulates results immutably.
154-182: LGTM! Outer product implementation is correct.The algorithm correctly filters blade pairs with common basis vectors and applies only the reordering sign (no metric contribution), as expected for the wedge product.
222-235: LGTM! Inverse implementation is correct with appropriate constraints.The formula
v⁻¹ = ṽ / (v·ṽ)is correctly implemented for blades. The exceptions appropriately handle non-blade inputs and zero-norm cases.
269-297: LGTM! Equality and hashing are correctly implemented.The use of
_ops.Equalsfor coefficient comparison is appropriate for handling numeric types correctly.
304-358: LGTM! Helper methods have proper validation and defensive checks.All helper methods correctly validate inputs and maintain the class invariants.
src/AiDotNet.Tensors/Engines/CpuHyperbolicManifoldEngine.cs (1)
215-277: Gyration-based parallel transport is now correctly implemented.The implementation now properly includes the Möbius gyration operator
gyr[y, -x](v)as required by the parallel transport formula. The gyration coefficients (lines 264-265) correctly account for the curvature and norm terms, and the final result (lines 269-273) properly combines gyration with conformal factor scaling.This addresses the concern from the previous review.
src/NeuralNetworks/NeuralNetworkBase.cs (1)
666-705: Single-layer validation change is reasonable and keeps existing safety checksAllowing
layers.Count == 1while guarding the connection loop onlayers.Count > 1makes single-layer networks valid without weakening input/output validation. Just be aware that for 1‑layer architectures the same layer must pass bothIsValidInputLayerandIsValidOutputLayer; check your specialized layer overrides accordingly.src/NeuralNetworks/OctonionNeuralNetwork.cs (1)
78-105: Octonion dimension divisibility validation is correct and importantThe new checks enforcing that input, output, and hidden layer sizes are divisible by 8 before dividing to octonion features are necessary to prevent silent truncation and mismatched shapes. The exception messages are clear and point at
Architecture, which is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
src/NeuralNetworks/Layers/SparseLinearLayer.cs (2)
145-145: Consider making the random seed configurable.The fixed seed (42) ensures reproducible initialization, but it means all
SparseLinearLayerinstances will have identical weight patterns. For experimentation or ensemble training, you might want different initializations.🔎 Optional: Add seed parameter
public SparseLinearLayer( int inputFeatures, int outputFeatures, double sparsity = 0.9, - IActivationFunction<T>? activationFunction = null) + IActivationFunction<T>? activationFunction = null, + int? seed = null) : base( [inputFeatures], [outputFeatures], activationFunction ?? new ReLUActivation<T>()) { // ... - _weights = InitializeSparseWeights(); + _weights = InitializeSparseWeights(seed); } - private SparseTensor<T> InitializeSparseWeights() + private SparseTensor<T> InitializeSparseWeights(int? seed = null) { - var random = RandomHelper.CreateSeededRandom(42); + var random = seed.HasValue + ? RandomHelper.CreateSeededRandom(seed.Value) + : RandomHelper.CreateSeededRandom();
318-329: Consider optimizing weight gradient computation.The nested loop iterates through all non-zero weights for each output feature, resulting in O(OutputFeatures × NonZeroCount) complexity. For large sparse matrices, this could impact training performance.
🔎 More efficient approach
Instead of checking
if (row == o)for every output feature, iterate through non-zero weights once and accumulate gradients directly:// Compute gradients for (int b = 0; b < batchSize; b++) { - for (int o = 0; o < OutputFeatures; o++) + for (int nz = 0; nz < _weights.NonZeroCount; nz++) { - T gradOut = gradTensor[b, o]; + int row = _weights.RowIndices[nz]; + int col = _weights.ColumnIndices[nz]; + + T gradOut = gradTensor[b, row]; // Bias gradient: sum over batch - _biasesGradient[o] = _numOps.Add(_biasesGradient[o], gradOut); + _biasesGradient[row] = _numOps.Add(_biasesGradient[row], gradOut); - // Weight gradient: only update non-zero positions - for (int nz = 0; nz < _weights.NonZeroCount; nz++) - { - int row = _weights.RowIndices[nz]; - int col = _weights.ColumnIndices[nz]; - if (row == o) - { - T inputVal = _lastInput.Rank == 1 ? _lastInput[col] : _lastInput[b, col]; - var contrib = _numOps.Multiply(gradOut, inputVal); - _weightsGradient[row, col] = _numOps.Add(_weightsGradient[row, col], contrib); - } - } + // Weight gradient + T inputVal = _lastInput.Rank == 1 ? _lastInput[col] : _lastInput[b, col]; + var contrib = _numOps.Multiply(gradOut, inputVal); + _weightsGradient[row, col] = _numOps.Add(_weightsGradient[row, col], contrib); } }This reduces complexity to O(batchSize × NonZeroCount).
src/NeuralNetworks/OctonionNeuralNetwork.cs (1)
211-237: Consider try/finally for training mode state.For consistency with
Predictand to ensureIsTrainingModeis reset even if training fails partway through, wrap the training logic in try/finally.🔎 Proposed pattern
public override void Train(Tensor<T> input, Tensor<T> expectedOutput) { IsTrainingMode = true; - - var prediction = Forward(input); - // ... rest of training logic ... - _optimizer.UpdateParameters(Layers); - - IsTrainingMode = false; + try + { + var prediction = Forward(input); + // ... rest of training logic ... + _optimizer.UpdateParameters(Layers); + } + finally + { + IsTrainingMode = false; + } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/NeuralNetworks/HyperbolicNeuralNetwork.cssrc/NeuralNetworks/Layers/SparseLinearLayer.cssrc/NeuralNetworks/OctonionNeuralNetwork.cssrc/PhysicsInformed/ScientificML/SymbolicPhysicsLearner.cs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
src/NeuralNetworks/OctonionNeuralNetwork.cssrc/NeuralNetworks/Layers/SparseLinearLayer.cssrc/NeuralNetworks/HyperbolicNeuralNetwork.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
src/NeuralNetworks/OctonionNeuralNetwork.cssrc/NeuralNetworks/Layers/SparseLinearLayer.cssrc/NeuralNetworks/HyperbolicNeuralNetwork.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build (Windows)
- GitHub Check: CodeQL Analysis
🔇 Additional comments (13)
src/NeuralNetworks/Layers/SparseLinearLayer.cs (2)
197-212: LGTM! 1D input validation has been properly added.The validation at lines 200-204 correctly addresses the previous review comment about missing input size validation for single-sample inputs. This prevents potential index-out-of-bounds errors.
282-282: LGTM! Activation derivative is now correctly applied.The fix at line 282 correctly addresses the previous critical review comment. The layer now properly applies the activation derivative via
ApplyActivationDerivativebefore computing gradients, ensuring correct backpropagation for non-linear activations like ReLU.src/NeuralNetworks/OctonionNeuralNetwork.cs (8)
9-40: Clear, beginner-friendly documentation.The XML documentation provides excellent context for developers unfamiliar with octonion algebra while keeping the implementation details concise.
54-64: Constructor logic is sound.The default optimizer and loss function selections are appropriate. Note that when
lossFunctionis null, a new instance is created both for the base class and this class's field—this is fine since loss functions are typically stateless.
83-104: Dimension validation properly implemented.The validation now correctly ensures all dimensions are divisible by 8 before layer construction, preventing silent truncation issues. Good use of descriptive exception messages.
157-169: Forward pass implementation is correct.The layer iteration and tensor propagation follow standard neural network patterns.
176-185: Backward pass correctly iterates layers in reverse.
191-204: Parameter slicing and distribution logic is correct.
243-261: Metadata captures comprehensive network state.Good coverage of architecture details and layer information for debugging and model introspection.
308-327: Layer validation correctly accepts octonion layers while preserving extensibility.The fallback to
base.IsValidInputLayer/base.IsValidOutputLayerallows flexibility for mixed architectures.src/NeuralNetworks/HyperbolicNeuralNetwork.cs (3)
133-146: LGTM! Training mode preservation implemented correctly.The
Predictmethod now properly captures the previous training mode state before setting it to false and restores it afterward. This addresses the previous review concern about unconditionally forcing training mode back to true.
304-312: Verify that sharing optimizer and loss function instances is intentional.
CreateNewInstancepasses the same_optimizer(line 309) and_lossFunction(line 310) references to the new network instance. Since optimizers typically maintain stateful information (momentum, adaptive learning rates, parameter history), sharing the optimizer between network instances could lead to:
- Unintended state updates affecting both networks
- Training one instance inadvertently updating the optimizer state used by the other
- Confusion about which network's parameters the optimizer is tracking
Is this sharing intentional (e.g., for transfer learning or parameter-tied ensembles), or should each instance have independent optimizer and loss function instances?
If independent instances are needed, consider creating new optimizer and loss function instances:
🔎 Alternative implementation for independent instances
protected override IFullModel<T, Tensor<T>, Tensor<T>> CreateNewInstance() { + // Create new optimizer and loss function instances for independence + var newOptimizer = new AdamOptimizer<T, Tensor<T>, Tensor<T>>(this); + var newLossFunction = new MeanSquaredErrorLoss<T>(); + return new HyperbolicNeuralNetwork<T>( Architecture, _curvature, - _optimizer, - _lossFunction, + newOptimizer, + newLossFunction, Convert.ToDouble(MaxGradNorm)); }Note: This assumes default optimizer/loss types. If the original types need to be preserved, a factory or cloning mechanism would be required.
153-165: LGTM! Core network operations implemented correctly.The forward pass, backward pass, parameter update, and layer validation methods are all implemented correctly:
Forwardproperly chains layers and validates input shapesBackwardcorrectly propagates gradients in reverse orderUpdateParametersappropriately slices and distributes parameters to layers- Input/output layer validation correctly identifies
HyperbolicLinearLayeras validAlso applies to: 179-188, 194-207, 322-341
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (18)
src/PhysicsInformed/Interfaces/IMultiFidelityTrainingHistory.cs (1)
22-42: Consider IReadOnlyList for better encapsulation.The properties currently return
List<T>, which exposes mutable collections. While this provides flexibility for consumers to analyze or manipulate the data, it also allows unintended modifications to the training history.🔎 Optional refactor using IReadOnlyList
+using System.Collections.Generic; + /// <summary> /// Gets the total losses per epoch (combined from all fidelity levels). /// </summary> -List<T> Losses { get; } +IReadOnlyList<T> Losses { get; } /// <summary> /// Gets the low-fidelity data losses per epoch. /// </summary> -List<T> LowFidelityLosses { get; } +IReadOnlyList<T> LowFidelityLosses { get; } /// <summary> /// Gets the high-fidelity data losses per epoch. /// </summary> -List<T> HighFidelityLosses { get; } +IReadOnlyList<T> HighFidelityLosses { get; } /// <summary> /// Gets the correlation losses per epoch (measures agreement between fidelity levels). /// </summary> -List<T> CorrelationLosses { get; } +IReadOnlyList<T> CorrelationLosses { get; } /// <summary> /// Gets the PDE residual losses per epoch. /// </summary> -List<T> PhysicsLosses { get; } +IReadOnlyList<T> PhysicsLosses { get; }This would prevent external modifications while still allowing read access and enumeration. The implementation would need corresponding updates.
src/PhysicsInformed/Interfaces/IGpuAcceleratedPINN.cs (1)
19-47: Consider whether generic type parameterTis needed.The type parameter
Tis declared but not used in any interface members. If it's intended for consistency with other PINN interfaces or for future extension, this is acceptable. Otherwise, consider removing it to simplify the interface.Additionally, for resource management, consider having implementers also implement
IDisposableto integrate with C#'susingpattern for deterministic cleanup.src/PhysicsInformed/PDEs/MaxwellEquations.cs (1)
117-127: Minor: Some extracted derivatives are unused.
dExdx(Line 117) anddEydy(Line 122) are extracted but not used in the residual computation. While this doesn't affect correctness, removing them would improve code clarity.🔎 Suggested cleanup
- T dExdx = derivatives.FirstDerivatives[0, 0]; T dExdy = derivatives.FirstDerivatives[0, 1]; T dExdt = derivatives.FirstDerivatives[0, 2]; T dEydx = derivatives.FirstDerivatives[1, 0]; - T dEydy = derivatives.FirstDerivatives[1, 1]; T dEydt = derivatives.FirstDerivatives[1, 2];src/PhysicsInformed/DomainDecompositionTrainingHistory.cs (1)
64-70: Consider validatingsubdomainLossescount matchesSubdomainCount.The
AddEpochmethod doesn't validate thatsubdomainLosses.CountmatchesSubdomainCount. Passing a mismatched list would create inconsistent data.🔎 Suggested fix
public void AddEpoch(T totalLoss, List<T> subdomainLosses, T interfaceLoss, T physicsLoss) { + if (subdomainLosses == null) + { + throw new ArgumentNullException(nameof(subdomainLosses)); + } + if (subdomainLosses.Count != SubdomainCount) + { + throw new ArgumentException($"Expected {SubdomainCount} subdomain losses, got {subdomainLosses.Count}.", nameof(subdomainLosses)); + } AddEpoch(totalLoss); // Base class tracks total loss SubdomainLosses.Add(new List<T>(subdomainLosses)); InterfaceLosses.Add(interfaceLoss); PhysicsLosses.Add(physicsLoss); }src/PhysicsInformed/PDEs/AdvectionDiffusionEquation.cs (1)
123-177: Residual returns linear value - verify consistency intent.Like
BlackScholesEquation, this returns the linear residual rather than squared. For training loss, this may require the caller to square it. Consider documenting whether callers should square this value, or align with other PDEs that return sum-of-squared residuals.The physics implementation is correct for both 1D and 2D cases.
src/PhysicsInformed/GpuPINNTrainingOptions.cs (1)
86-89: Consider cachingDefaultpreset to avoid repeated allocations.The
Defaultproperty creates a new instance on every access. If frequently accessed, consider caching it or documenting that callers should cache the result.- public static GpuPINNTrainingOptions Default => new GpuPINNTrainingOptions(); + private static readonly GpuPINNTrainingOptions _default = new GpuPINNTrainingOptions(); + public static GpuPINNTrainingOptions Default => _default;Alternatively, if mutability is intended, keep the current pattern but be aware callers modifying the "default" would affect all users sharing that instance if cached.
src/PhysicsInformed/Interfaces/IMultiScalePDE.cs (1)
1-1: Unused using directive.The
using System;directive appears unnecessary as no types from System namespace are directly used in this file.src/PhysicsInformed/GpuPINNTrainer.cs (1)
103-106: Verbose logging uses Console.WriteLine.For library code, consider using a logging abstraction or events instead of direct
Console.WriteLinecalls, allowing consumers to control log output.Also applies to: 115-118
src/PhysicsInformed/Interfaces/IPDESpecification.cs (2)
44-58: Consider havingIPDEResidualGradient<T>extendIPDESpecification<T>.The gradient interface doesn't inherit from
IPDESpecification<T>, so implementations must explicitly implement both. If gradient computation always requires residual computation, consider:- public interface IPDEResidualGradient<T> + public interface IPDEResidualGradient<T> : IPDESpecification<T>This would ensure any type providing gradients also provides the base residual computation.
87-93:PDEResidualGradientalways allocates third derivatives, even for second-order PDEs.For most common PDEs (heat, Poisson, wave), third derivatives are unnecessary. Consider lazy initialization or a constructor overload:
public PDEResidualGradient(int outputDimension, int inputDimension) + : this(outputDimension, inputDimension, allocateThirdDerivatives: false) + { + } + + public PDEResidualGradient(int outputDimension, int inputDimension, bool allocateThirdDerivatives) { OutputGradients = new T[outputDimension]; FirstDerivatives = new T[outputDimension, inputDimension]; SecondDerivatives = new T[outputDimension, inputDimension, inputDimension]; - ThirdDerivatives = new T[outputDimension, inputDimension, inputDimension, inputDimension]; + ThirdDerivatives = allocateThirdDerivatives + ? new T[outputDimension, inputDimension, inputDimension, inputDimension] + : new T[0, 0, 0, 0]; }src/PhysicsInformed/Interfaces/IInverseProblem.cs (1)
122-136:MeasurementNoiseLevellacks null-safety enforcement.The property
MeasurementNoiseLevelreturnsdefault(T)when unknown, but callers must remember to checkHasMeasurementNoiseLevelfirst. Consider making this a nullableT?or throwing when accessed without checking the guard.- T MeasurementNoiseLevel { get; } + T? MeasurementNoiseLevel { get; }This would provide compile-time enforcement rather than relying on documentation.
src/PhysicsInformed/MultiFidelityTrainingHistory.cs (1)
28-46: Consider exposing collections as IReadOnlyList for better encapsulation.The properties currently return
List<T>directly, allowing external callers to modify the collections (add, remove, clear). If external mutation isn't intended, consider returningIReadOnlyList<T>instead while keeping the backing field asList<T>.🔎 Proposed refactor
- public List<T> LowFidelityLosses { get; } = new List<T>(); + private readonly List<T> _lowFidelityLosses = new List<T>(); + public IReadOnlyList<T> LowFidelityLosses => _lowFidelityLosses; - public List<T> HighFidelityLosses { get; } = new List<T>(); + private readonly List<T> _highFidelityLosses = new List<T>(); + public IReadOnlyList<T> HighFidelityLosses => _highFidelityLosses; - public List<T> CorrelationLosses { get; } = new List<T>(); + private readonly List<T> _correlationLosses = new List<T>(); + public IReadOnlyList<T> CorrelationLosses => _correlationLosses; - public List<T> PhysicsLosses { get; } = new List<T>(); + private readonly List<T> _physicsLosses = new List<T>(); + public IReadOnlyList<T> PhysicsLosses => _physicsLosses;Then update the
AddEpochmethod to use the backing fields:public void AddEpoch(T totalLoss, T lowFidelityLoss, T highFidelityLoss, T correlationLoss, T physicsLoss) { AddEpoch(totalLoss); // Base class tracks total loss - LowFidelityLosses.Add(lowFidelityLoss); - HighFidelityLosses.Add(highFidelityLoss); - CorrelationLosses.Add(correlationLoss); - PhysicsLosses.Add(physicsLoss); + _lowFidelityLosses.Add(lowFidelityLoss); + _highFidelityLosses.Add(highFidelityLoss); + _correlationLosses.Add(correlationLoss); + _physicsLosses.Add(physicsLoss); }src/PhysicsInformed/NeuralOperators/FourierNeuralOperator.cs (1)
269-394: FNO training pipeline looks correct; consider minor cleanup and logging control.The
Trainmethod now does a full forward → loss → derivative → backprop → optimizer step loop, andComputeMSEis shape-agnostic with proper shape equality checks, so the earlier issues around “no learning” and 2D‑only MSE are resolved.Two optional improvements:
ComputeAverageLossis currently unused; either wire it into evaluation/metrics or remove it to reduce dead code.- Console logging every 10 epochs is unconditional; consider adding a
verbose(or logger) parameter likeDeepOperatorNetwork.Trainso library users can suppress stdout noise in production runs.src/PhysicsInformed/NeuralOperators/DeepOperatorNetwork.cs (3)
912-921: Avoid sharing the same optimizer instance between cloned DeepONet models.
CreateNewInstancecurrently passes the existing_optimizerinto the newDeepOperatorNetwork:return new DeepOperatorNetwork<T>( Architecture, _branchNet.Architecture, _trunkNet.Architecture, _p, _numSensors, _optimizer);If
IGradientBasedOptimizerholds internal state (e.g., Adam moments) and/or a reference to the original model, this can couple the original and cloned networks in unexpected ways.Safer options:
- Pass
optimizer: nullso the clone creates its own default optimizer, or- Construct a fresh optimizer instance with the same options but bound to the new model.
This keeps cloned models independent and avoids cross‑contamination of optimizer state.
528-589: Remove unused locals in the mainTrainloop to reduce noise.Inside the per‑sample loop of
Train(T[,] inputFunctions, T[,,] queryLocations, T[,] targetValues, ...)you allocate arrays that are never used:
var inputFunction = new T[_numSensors];var targetValuesSample = new T[numQueries];They’re populated but never read, since you operate directly on
branchInput,trunkInput, andtargets. Dropping these will simplify the method and avoid unnecessary allocations.
266-317: Evaluation path looks consistent; small stylistic nit on reshaping.
Evaluatedoes appropriate validation (null checks, sensor count, trunk input dimension) and computes
- branch output
b ∈ ℝ^{1×p},- trunk output
t ∈ ℝ^{1×p},- elementwise product followed by
ReduceSumover axis 1.The conditional reshapes to 2D if needed:
var branchOutput2D = branchOutput.Rank == 2 ? branchOutput : branchOutput.Reshape(1, _p); var trunkOutput2D = trunkOutput.Rank == 2 ? trunkOutput : trunkOutput.Reshape(1, _p);are correct but could be factored into a small helper (e.g.,
EnsureRowVector(Tensor<T> x, int length)) to avoid duplicating this pattern acrossEvaluate,EvaluateMultiple, and training.src/Helpers/LayerHelper.cs (2)
2990-3015:numModesis currently ignored in the Fourier Neural Operator layer factory.In
CreateDefaultFourierNeuralOperatorLayers, the signature exposesnumFourierLayers,hiddenChannels, andnumModes, but the implementation:
- Never uses
numModes.- Builds only stacks of
DenseLayer<T>with GELU activations (no Fourier/spectral layers), despite the XML doc explaining FNO behavior in frequency space.That’s fine as a temporary “dense proxy” for an FNO, but the unused
numModesargument and Fourier‑centric documentation are misleading.Options:
- If this is meant as a simplified dense baseline, either remove
numModesor make it clear in the remarks that the helper does not yet use spectral layers.- If the intent is to align with
FourierNeuralOperator<T>/FourierLayer<T>, consider wiringnumModesinto the architecture (or deprecate this helper in favor of usingFourierNeuralOperator<T>directly).
3035-3149: PINN/Deep Ritz/Variational PINN layer stacks look appropriate for physics‑informed use.The default factories for VPINN, Deep Ritz, and standard PINN all:
- Validate basic hyperparameters via
ValidateLayerParameters.- Flatten multi‑dimensional inputs to a single dense input size.
- Use Tanh hidden activations with linear outputs, which is standard for PDE residual/energy minimization where smooth derivatives are required.
No functional issues stand out; these are reasonable defaults. If anything, exposing an optional final activation (e.g., Sigmoid/Tanh for bounded states) could be a future extension, but not necessary for this PR.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
docs/PHYSICS_AI_IMPLEMENTATION_PLAN.mdsrc/Helpers/LayerHelper.cssrc/PhysicsInformed/DomainDecompositionTrainingHistory.cssrc/PhysicsInformed/FiniteDifferenceGradient.cssrc/PhysicsInformed/GpuPINNTrainer.cssrc/PhysicsInformed/GpuPINNTrainingOptions.cssrc/PhysicsInformed/Interfaces/IDomainDecompositionTrainingHistory.cssrc/PhysicsInformed/Interfaces/IGpuAcceleratedPINN.cssrc/PhysicsInformed/Interfaces/IInverseProblem.cssrc/PhysicsInformed/Interfaces/IMultiFidelityTrainingHistory.cssrc/PhysicsInformed/Interfaces/IMultiScalePDE.cssrc/PhysicsInformed/Interfaces/IPDESpecification.cssrc/PhysicsInformed/MultiFidelityTrainingHistory.cssrc/PhysicsInformed/NeuralOperators/DeepOperatorNetwork.cssrc/PhysicsInformed/NeuralOperators/FourierNeuralOperator.cssrc/PhysicsInformed/PDEs/AdvectionDiffusionEquation.cssrc/PhysicsInformed/PDEs/BlackScholesEquation.cssrc/PhysicsInformed/PDEs/KortewegDeVriesEquation.cssrc/PhysicsInformed/PDEs/LinearElasticityEquation.cssrc/PhysicsInformed/PDEs/MaxwellEquations.cssrc/PhysicsInformed/PDEs/NavierStokesEquation.cssrc/PhysicsInformed/PDEs/SchrodingerEquation.cssrc/PhysicsInformed/PINNs/DeepRitzMethod.cssrc/PhysicsInformed/PINNs/DomainDecompositionPINN.cssrc/PhysicsInformed/PINNs/InverseProblemPINN.cssrc/PhysicsInformed/PINNs/MultiFidelityPINN.cssrc/PhysicsInformed/PINNs/MultiScalePINN.cssrc/PhysicsInformed/PINNs/PhysicsInformedNeuralNetwork.cssrc/PhysicsInformed/PINNs/VariationalPINN.cssrc/PhysicsInformed/ScientificML/HamiltonianNeuralNetwork.cssrc/PhysicsInformed/ScientificML/LagrangianNeuralNetwork.cssrc/PhysicsInformed/ScientificML/UniversalDifferentialEquations.cstests/AiDotNet.Tests/UnitTests/PhysicsInformed/GpuAccelerationTests.cstests/AiDotNet.Tests/UnitTests/PhysicsInformed/MultiFidelityAndDomainDecompositionTests.cstests/AiDotNet.Tests/UnitTests/PhysicsInformed/MultiScaleAndInverseProblemTests.cstests/AiDotNet.Tests/UnitTests/PhysicsInformed/PDEs/AdvancedPDETests.cstests/AiDotNet.Tests/UnitTests/PhysicsInformed/PDEs/PDEAnalyticalSolutionTests.cstests/AiDotNet.Tests/UnitTests/PhysicsInformed/PINNs/PinnTrainingTests.cs
✅ Files skipped from review due to trivial changes (1)
- docs/PHYSICS_AI_IMPLEMENTATION_PLAN.md
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
src/PhysicsInformed/FiniteDifferenceGradient.cssrc/PhysicsInformed/PDEs/NavierStokesEquation.cssrc/PhysicsInformed/Interfaces/IGpuAcceleratedPINN.cssrc/PhysicsInformed/MultiFidelityTrainingHistory.cssrc/PhysicsInformed/Interfaces/IDomainDecompositionTrainingHistory.cssrc/PhysicsInformed/PDEs/MaxwellEquations.cssrc/PhysicsInformed/PDEs/BlackScholesEquation.cssrc/PhysicsInformed/PDEs/SchrodingerEquation.cssrc/PhysicsInformed/Interfaces/IMultiFidelityTrainingHistory.cssrc/PhysicsInformed/PDEs/KortewegDeVriesEquation.cssrc/PhysicsInformed/GpuPINNTrainer.cssrc/PhysicsInformed/Interfaces/IMultiScalePDE.cssrc/PhysicsInformed/DomainDecompositionTrainingHistory.cssrc/PhysicsInformed/PDEs/LinearElasticityEquation.cssrc/PhysicsInformed/PDEs/AdvectionDiffusionEquation.cssrc/PhysicsInformed/GpuPINNTrainingOptions.cssrc/PhysicsInformed/NeuralOperators/DeepOperatorNetwork.cssrc/PhysicsInformed/NeuralOperators/FourierNeuralOperator.cssrc/Helpers/LayerHelper.cssrc/PhysicsInformed/Interfaces/IPDESpecification.cssrc/PhysicsInformed/Interfaces/IInverseProblem.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
src/PhysicsInformed/FiniteDifferenceGradient.cssrc/PhysicsInformed/PDEs/NavierStokesEquation.cssrc/PhysicsInformed/Interfaces/IGpuAcceleratedPINN.cssrc/PhysicsInformed/MultiFidelityTrainingHistory.cssrc/PhysicsInformed/Interfaces/IDomainDecompositionTrainingHistory.cssrc/PhysicsInformed/PDEs/MaxwellEquations.cssrc/PhysicsInformed/PDEs/BlackScholesEquation.cssrc/PhysicsInformed/PDEs/SchrodingerEquation.cssrc/PhysicsInformed/Interfaces/IMultiFidelityTrainingHistory.cssrc/PhysicsInformed/PDEs/KortewegDeVriesEquation.cssrc/PhysicsInformed/GpuPINNTrainer.cssrc/PhysicsInformed/Interfaces/IMultiScalePDE.cssrc/PhysicsInformed/DomainDecompositionTrainingHistory.cssrc/PhysicsInformed/PDEs/LinearElasticityEquation.cssrc/PhysicsInformed/PDEs/AdvectionDiffusionEquation.cssrc/PhysicsInformed/GpuPINNTrainingOptions.cssrc/PhysicsInformed/NeuralOperators/DeepOperatorNetwork.cssrc/PhysicsInformed/NeuralOperators/FourierNeuralOperator.cssrc/Helpers/LayerHelper.cssrc/PhysicsInformed/Interfaces/IPDESpecification.cssrc/PhysicsInformed/Interfaces/IInverseProblem.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build (Windows)
- GitHub Check: CodeQL Analysis
🔇 Additional comments (29)
src/PhysicsInformed/Interfaces/IMultiFidelityTrainingHistory.cs (1)
1-53: Excellent interface design with comprehensive documentation.The interface is well-structured and the beginner-friendly remarks clearly explain multi-fidelity training concepts. The separation of concerns (tracking different fidelity levels and correlation) is appropriate for multi-fidelity PINN workflows.
src/PhysicsInformed/FiniteDifferenceGradient.cs (4)
7-35: Excellent documentation.The XML remarks provide clear, explicit warnings about appropriate use cases and explain why this should not be used for neural network training. This level of documentation helps prevent misuse and guides developers to the correct approach.
59-77: Comprehensive null validation.All input parameters are properly validated with appropriate
ArgumentNullExceptionchecks.
83-97: Correct implementation of central finite difference.The loop correctly implements the central difference formula df/dx ≈ (f(x+h) - f(x-h)) / (2h) for each parameter. The perturbation and restoration logic is sound, and the use of INumericOperations ensures type-safe arithmetic.
99-101: Final parameter restoration is correct and necessary.After all gradient components are computed, the restored parameter vector must be applied via
applyParameters(parameters)to ensure the model/system returns to its original state. Without this, the system would remain in the final perturbed state from the loop.src/PhysicsInformed/Interfaces/IDomainDecompositionTrainingHistory.cs (1)
20-56: Well-designed interface for domain decomposition training history.The interface is clean, well-documented, and captures the essential metrics for domain decomposition PINN training. The beginner-friendly remarks are helpful.
One consideration: exposing
List<T>properties allows consumers to mutate the collections directly. If immutability is desired, consider returningIReadOnlyList<T>instead. However, for internal training history tracking, the current design is acceptable.src/PhysicsInformed/PDEs/BlackScholesEquation.cs (1)
83-98: LGTM on the residual computation logic.The Black-Scholes PDE residual computation correctly implements:
- ∂V/∂t + ½σ²S²∂²V/∂S² + rS∂V/∂S - rV = 0
The derivative indexing and term assembly are accurate.
src/PhysicsInformed/PDEs/SchrodingerEquation.cs (1)
106-153: Physics implementation is correct.The split real/imaginary form of the Schrödinger equation is correctly implemented:
- Real: ∂ψ_r/∂t + ½∂²ψ_i/∂x² - V·ψ_i = 0
- Imag: ∂ψ_i/∂t - ½∂²ψ_r/∂x² + V·ψ_r = 0
The residual computation and sum-of-squares return is consistent with other multi-equation PDEs.
src/PhysicsInformed/PDEs/NavierStokesEquation.cs (1)
90-158: Residual computation is well-structured.The implementation correctly captures:
- Continuity: ∂u/∂x + ∂v/∂y = 0
- X-momentum with advection, pressure gradient, and viscous diffusion
- Y-momentum similarly structured
The sum-of-squares residual is consistent with the other multi-equation PDEs.
src/PhysicsInformed/PDEs/AdvectionDiffusionEquation.cs (2)
60-74: 1D constructor implementation is correct.Good validation of the diffusion coefficient and proper initialization of fields. The
_is2D = falseflag properly distinguishes the mode.
209-213: DynamicInputDimensionis a good design choice.Using the
_is2Dflag to return the appropriate dimension (3 for 2D, 2 for 1D) is cleaner than having separate classes.src/PhysicsInformed/GpuPINNTrainingOptions.cs (1)
1-136: Well-structured GPU configuration with sensible defaults and presets.The configuration class provides clear documentation, reasonable defaults, and useful presets for different GPU tiers. The design follows established patterns for options classes.
src/PhysicsInformed/PDEs/LinearElasticityEquation.cs (3)
164-188: Gradient computation may be inconsistent with squared residual return value.
ComputeResidualreturns the sum of squared residuals (R1² + R2²), butComputeResidualGradientcomputes gradients of the raw residuals (∂R₁/∂...,∂R₂/∂...) without chain-ruling through the squaring. For backpropagation to work correctly, the gradient should account for∂(R1² + R2²)/∂x = 2*R1*(∂R1/∂x) + 2*R2*(∂R2/∂x).Verify whether the caller (physics loss computation) handles this chain rule, or if the gradient method should return raw residual gradients with residual values for the caller to apply the chain rule.
60-74: Constructor validation and initialization look correct.The μ > 0 validation is appropriate for physical constraints, and the Lamé parameters are properly stored.
100-121: Factory method correctly converts engineering constants to Lamé parameters.The conversion formulas and validation bounds for Young's modulus and Poisson's ratio are physically correct.
src/PhysicsInformed/PDEs/KortewegDeVriesEquation.cs (3)
103-131: Residual and gradient computations are mathematically consistent.The KdV equation residual correctly implements
∂u/∂t + αu∂u/∂x + β∂³u/∂x³, and the gradient accounts for both explicit derivative dependencies and the implicit dependence onuthrough the nonlinear term.
60-71: Constructor properly validates dispersion coefficient.The check for
β ≠ 0is correct since zero dispersion would degenerate the KdV equation.
1-170: Well-implemented KdV equation with excellent documentation.The implementation correctly handles the nonlinear dispersive PDE with appropriate derivative requirements and consistent residual/gradient computations.
src/PhysicsInformed/Interfaces/IMultiScalePDE.cs (2)
36-134: Well-designed multi-scale PDE interface with comprehensive documentation.The interface provides a clean abstraction for multi-scale problems with appropriate methods for scale-specific residuals, inter-scale coupling, and loss weighting strategies.
177-213: Training options class provides good configurability.The
MultiScaleTrainingOptions<T>class covers essential configuration for multi-scale training including adaptive weighting, sequential training, and manual weight overrides.src/PhysicsInformed/GpuPINNTrainer.cs (2)
146-195: Train method correctly delegates to PINN's Solve method.The training orchestration properly wraps the PINN's built-in training with timing metrics and GPU-aware history tracking.
233-237: Manual verification required: GetLastLoss() loss staleness cannot be confirmed without code inspection.The concern about
GetLastLoss()potentially returning stale loss whenbatchTargetsis null depends on implementation details that require direct access to:
- The
GetLastLoss()method implementation in the PINN class- Whether the forward pass at line 211 updates or computes physics loss
- The loss state management during
Predict()callsTo verify this concern, inspect the PINN class implementation to confirm whether
GetLastLoss()is updated after the current forward pass and whether line 211 actually computes physics loss.src/PhysicsInformed/Interfaces/IPDESpecification.cs (1)
1-223: Comprehensive PDE interface design with excellent documentation.The interfaces provide a clean, well-documented API for PDE specification, boundary/initial conditions, and gradient computation. The beginner-friendly remarks are particularly helpful for users new to physics-informed methods.
src/PhysicsInformed/Interfaces/IInverseProblem.cs (2)
354-366: Result properties usedefault!appropriately for required initialization.The use of
default!forDataLoss,PhysicsLoss, andTotalLossacknowledges these will be set after construction, which is a valid pattern for result DTOs.
1-394: Comprehensive inverse problem API with excellent educational documentation.The interface design cleanly separates concerns (base interface, gradient extension, options, results) and the documentation thoroughly explains inverse problem concepts for newcomers.
src/PhysicsInformed/MultiFidelityTrainingHistory.cs (3)
6-25: Excellent documentation for multi-fidelity training history.The XML documentation and beginner-friendly remarks clearly explain the purpose of each loss component and the typical training dynamics. This will greatly help users understand multi-fidelity PINN behavior.
56-63: LGTM - Clean implementation of epoch recording.The method correctly delegates to the base class for total loss tracking before recording the multi-fidelity specific metrics. The implementation is straightforward and maintains proper synchronization between base and derived state.
26-26: Verify generic type constraints on T.The class does not specify constraints on the generic type parameter
T. Since this tracks loss values (typically numeric types likefloatordouble), verify whetherTrainingHistory<T>orIMultiFidelityTrainingHistory<T>already constrainTto numeric types. If neither the base class nor interface impose constraints, consider adding awhere T : struct, INumber<T>constraint or similar to ensure type safety.src/Helpers/LayerHelper.cs (1)
2778-2911: Physics‑informed layer factories (Hamiltonian/Lagrangian/UDE) are structurally consistent.The new helpers:
- Enforce scalar outputs for Hamiltonian/Lagrangian nets (
OutputSize == 1).- Flatten arbitrary input shapes via
Aggregate.- Use Tanh activations in hidden layers and Identity at the output, which matches the need for smooth derivatives and unbounded scalar energies/dynamics.
These look compatible with downstream autodiff/PDE solvers and follow the same parameter‑validation pattern as existing helpers.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/PhysicsInformed/PDEs/SchrodingerEquation.cs (1)
86-89: Inefficient lambda in default constructor.The lambda
x => MathHelper.GetNumericOperations<T>().ZeroinvokesGetNumericOperations<T>()on every potential evaluation. Since the potential is evaluated for every collocation point, this adds unnecessary overhead.🔎 Suggested fix - cache the zero value
public SchrodingerEquation() - : this(x => MathHelper.GetNumericOperations<T>().Zero) + : this(_ => default(T)!) { }Or initialize directly:
public SchrodingerEquation() { var zero = MathHelper.GetNumericOperations<T>().Zero; _potentialFunction = _ => zero; _halfCoeff = NumOps.FromDouble(0.5); }src/PhysicsInformed/PDEs/LinearElasticityEquation.cs (1)
177-179: Missing cross-derivative coupling in gradient computation.The gradient assignments for cross-derivatives are incorrect. Based on the residual computation (lines 142-153):
- R₁ includes the term
(λ+μ)∂²v/∂x∂y(affecting output v, which is output[1])- R₂ includes the term
(λ+μ)∂²u/∂x∂y(affecting output u, which is output[0])However, lines 178-179 assign:
gradient.SecondDerivatives[0, 0, 1]- represents ∂R₁/∂(∂²u/∂x∂y), but R₁ depends on ∂²v/∂x∂y, not ∂²u/∂x∂ygradient.SecondDerivatives[1, 0, 1]- represents ∂R₂/∂(∂²u/∂x∂y), which is correctThe correct assignments should be:
gradient.SecondDerivatives[0, 1, 1] = _lambdaPlusMu;for ∂R₁/∂(∂²v/∂x∂y)gradient.SecondDerivatives[1, 0, 1] = _lambdaPlusMu;for ∂R₂/∂(∂²u/∂x∂y)🔎 Proposed fix
// Cross derivatives -gradient.SecondDerivatives[0, 0, 1] = _lambdaPlusMu; // ∂R₁/∂(∂²u/∂x∂y) +gradient.SecondDerivatives[0, 1, 1] = _lambdaPlusMu; // ∂R₁/∂(∂²v/∂x∂y) gradient.SecondDerivatives[1, 0, 1] = _lambdaPlusMu; // ∂R₂/∂(∂²u/∂x∂y)
🧹 Nitpick comments (2)
src/PhysicsInformed/PDEs/PoissonEquation.cs (2)
61-67: Consider using the base class validation helper for consistency.The manual null check for
SecondDerivativesworks but differs from other PDE implementations that useValidateSecondDerivatives(derivatives). The base class validator also checksFirstDerivativesand provides a consistent error message format including the PDE name.🔎 Suggested refactor
public override T ComputeResidual(T[] inputs, T[] outputs, PDEDerivatives<T> derivatives) { - var secondDerivs = derivatives.SecondDerivatives; - if (secondDerivs is null) + ValidateSecondDerivatives(derivatives); + + var secondDerivs = derivatives.SecondDerivatives!; + if (secondDerivs is null) { - throw new ArgumentException("Poisson equation requires second derivatives."); + throw new InvalidOperationException("Derivatives were null after validation."); }
85-90: Same validation pattern inconsistency inComputeResidualGradient.This method also uses manual validation instead of the base class helper.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/PhysicsInformed/PDEs/AdvectionDiffusionEquation.cssrc/PhysicsInformed/PDEs/AllenCahnEquation.cssrc/PhysicsInformed/PDEs/BlackScholesEquation.cssrc/PhysicsInformed/PDEs/BurgersEquation.cssrc/PhysicsInformed/PDEs/HeatEquation.cssrc/PhysicsInformed/PDEs/KortewegDeVriesEquation.cssrc/PhysicsInformed/PDEs/LinearElasticityEquation.cssrc/PhysicsInformed/PDEs/MaxwellEquations.cssrc/PhysicsInformed/PDEs/NavierStokesEquation.cssrc/PhysicsInformed/PDEs/PDESpecificationBase.cssrc/PhysicsInformed/PDEs/PoissonEquation.cssrc/PhysicsInformed/PDEs/SchrodingerEquation.cssrc/PhysicsInformed/PDEs/WaveEquation.cs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
src/PhysicsInformed/PDEs/AllenCahnEquation.cssrc/PhysicsInformed/PDEs/PDESpecificationBase.cssrc/PhysicsInformed/PDEs/BurgersEquation.cssrc/PhysicsInformed/PDEs/BlackScholesEquation.cssrc/PhysicsInformed/PDEs/NavierStokesEquation.cssrc/PhysicsInformed/PDEs/LinearElasticityEquation.cssrc/PhysicsInformed/PDEs/MaxwellEquations.cssrc/PhysicsInformed/PDEs/SchrodingerEquation.cssrc/PhysicsInformed/PDEs/AdvectionDiffusionEquation.cssrc/PhysicsInformed/PDEs/WaveEquation.cssrc/PhysicsInformed/PDEs/KortewegDeVriesEquation.cssrc/PhysicsInformed/PDEs/PoissonEquation.cssrc/PhysicsInformed/PDEs/HeatEquation.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
src/PhysicsInformed/PDEs/AllenCahnEquation.cssrc/PhysicsInformed/PDEs/PDESpecificationBase.cssrc/PhysicsInformed/PDEs/BurgersEquation.cssrc/PhysicsInformed/PDEs/BlackScholesEquation.cssrc/PhysicsInformed/PDEs/NavierStokesEquation.cssrc/PhysicsInformed/PDEs/LinearElasticityEquation.cssrc/PhysicsInformed/PDEs/MaxwellEquations.cssrc/PhysicsInformed/PDEs/SchrodingerEquation.cssrc/PhysicsInformed/PDEs/AdvectionDiffusionEquation.cssrc/PhysicsInformed/PDEs/WaveEquation.cssrc/PhysicsInformed/PDEs/KortewegDeVriesEquation.cssrc/PhysicsInformed/PDEs/PoissonEquation.cssrc/PhysicsInformed/PDEs/HeatEquation.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL Analysis
- GitHub Check: Build (Windows)
🔇 Additional comments (10)
src/PhysicsInformed/PDEs/BlackScholesEquation.cs (1)
1-151: LGTM!The Black-Scholes equation implementation is correct. The PDE formulation (∂V/∂t + ½σ²S²∂²V/∂S² + rS∂V/∂S - rV = 0), derivative indexing, validation, and gradient computation all look proper. Pre-computing
_halfVolSquaredis a good optimization.src/PhysicsInformed/PDEs/BurgersEquation.cs (1)
1-131: LGTM!The Burgers' equation implementation correctly handles both inviscid (ν=0) and viscous cases with proper validation. The past review concern about missing second derivatives for viscous case has been addressed with explicit checks in both
ComputeResidual(lines 78-88) andComputeResidualGradient(lines 103-106).src/PhysicsInformed/PDEs/AdvectionDiffusionEquation.cs (1)
1-219: LGTM!The Advection-Diffusion equation implementation correctly handles both 1D and 2D cases. The derivative indexing is correct for both formulations, validation is proper, and the gradient computation accurately reflects the PDE structure.
src/PhysicsInformed/PDEs/AllenCahnEquation.cs (1)
1-94: LGTM!The Allen-Cahn equation implementation is correct. The PDE formulation (u_t + u³ - u - ε²·u_xx = 0), residual computation, and gradient (including the ∂R/∂u = 3u² - 1 from the reaction term) are all properly implemented.
src/PhysicsInformed/PDEs/SchrodingerEquation.cs (1)
150-156: Returning squared residual differs from other PDE implementations.Most PDE implementations in this codebase return the raw (signed) residual, but
SchrodingerEquationreturns the sum of squared residuals (R1² + R2²). This design choice is mathematically valid for loss computation but may cause integration issues if consumers expect consistent behavior across PDEs.Consider documenting this deviation clearly or aligning with the pattern used by other PDEs by returning a combined residual vector or a single representative value.
src/PhysicsInformed/PDEs/HeatEquation.cs (1)
1-92: LGTM!The Heat equation implementation is correct and clean. The PDE formulation (∂u/∂t - α·∂²u/∂x² = 0), derivative indexing, validation using base class helpers, and gradient computation are all properly implemented.
src/PhysicsInformed/PDEs/PDESpecificationBase.cs (1)
34-139: LGTM on the overall base class design.The
PDESpecificationBase<T>provides a solid foundation with consistent numeric operations, validation helpers, and gradient factory. The documentation is thorough and beginner-friendly.src/PhysicsInformed/PDEs/MaxwellEquations.cs (1)
111-213: LGTM! Residual and gradient computations are mathematically correct.The implementation correctly models the 2D TE Maxwell equations. The residual computation (lines 144-154) properly combines Faraday's and Ampere's laws, and the gradient computation (lines 188-210) correctly derives ∂R²/∂(first derivatives) with appropriate scaling factors including permittivity.
src/PhysicsInformed/PDEs/KortewegDeVriesEquation.cs (1)
100-166: LGTM! KdV equation implementation is mathematically sound.The residual computation correctly implements the nonlinear advection and third-order dispersion terms, and the gradient computation properly derives all partial derivatives including ∂R/∂u = α∂u/∂x from the nonlinear term. The validation ensuring β ≠ 0 prevents division-by-zero issues in potential solvers.
src/PhysicsInformed/PDEs/WaveEquation.cs (1)
74-120: LGTM! Wave equation implementation is correct.The residual computation properly calculates the time second derivative and spatial Laplacian across configurable dimensions (1D, 2D, 3D), and the gradient computation correctly assigns coefficients (+1 for time, -c² for spatial terms). The dimensional flexibility is well-implemented.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
src/PhysicsInformed/PDEs/NavierStokesEquation.cs (1)
221-233: Previous gradient issue has been resolved.The output gradients now correctly include all cross-equation contributions:
∂R/∂uincludes terms from both momentum-X and momentum-Y equations∂R/∂vincludes terms from both momentum-X and momentum-Y equationsThe gradient computation is mathematically correct and matches the chain rule for the sum-of-squared residuals.
🧹 Nitpick comments (6)
src/PhysicsInformed/FiniteDifferenceGradient.cs (1)
95-109: Consider adding exception safety for parameter restoration.If
lossFunction()orapplyParameters()throws an exception, theparameters[i]value at the current index remains modified because line 108 won't execute. This leaves the parameter vector in an inconsistent state.Wrapping the perturbation logic in a try-finally block would ensure parameters are always restored to their original values, even when exceptions occur.
🔎 Proposed refactor for exception safety
for (int i = 0; i < parameters.Length; i++) { var original = parameters[i]; - - parameters[i] = numOps.Add(original, epsilonT); - applyParameters(parameters); - var lossPlus = lossFunction(); - - parameters[i] = numOps.Subtract(original, epsilonT); - applyParameters(parameters); - var lossMinus = lossFunction(); - - gradient[i] = numOps.Divide(numOps.Subtract(lossPlus, lossMinus), twoEpsilon); - parameters[i] = original; + try + { + parameters[i] = numOps.Add(original, epsilonT); + applyParameters(parameters); + var lossPlus = lossFunction(); + + parameters[i] = numOps.Subtract(original, epsilonT); + applyParameters(parameters); + var lossMinus = lossFunction(); + + gradient[i] = numOps.Divide(numOps.Subtract(lossPlus, lossMinus), twoEpsilon); + } + finally + { + parameters[i] = original; + } }src/PhysicsInformed/PDEs/LinearElasticityEquation.cs (1)
59-68: Consider validating the lambda parameter for physical realism.While the code validates that
mu > 0, it does not validatelambda. For most physically realistic materials, the first Lamé parameter should satisfy certain constraints. Although negative lambda is theoretically possible for exotic auxetic materials, typical applications requirelambda > -2*mu/3(to ensure positive bulk modulus K = λ + 2μ/3 > 0).🔎 Proposed validation for lambda
public LinearElasticityEquation(T lambda, T mu, T? bodyForceX = default, T? bodyForceY = default) { ValidatePositive(mu, nameof(mu)); + + // Validate that bulk modulus K = λ + 2μ/3 is positive for physical stability + T twoMuOverThree = NumOps.Divide(NumOps.Multiply(NumOps.FromDouble(2), mu), NumOps.FromDouble(3)); + T bulkModulus = NumOps.Add(lambda, twoMuOverThree); + if (NumOps.Compare(bulkModulus, NumOps.Zero) <= 0) + { + throw new ArgumentException("Lambda must satisfy λ + 2μ/3 > 0 for physical stability.", nameof(lambda)); + } _lambda = lambda;src/PhysicsInformed/PDEs/NavierStokesEquation.cs (1)
221-233: Consider explicitly initializing gradient for pressure.While mathematically correct (pressure
pdoes not appear directly in the residual, only its derivatives do, so ∂R/∂p = 0), explicitly settinggradient.OutputGradients[2] = NumOps.Zerowould improve code clarity and self-documentation.🔎 Optional enhancement
// Gradient w.r.t. v (output 1) // From momentum-x: v appears in convection term v*∂u/∂y → ∂R/∂v += 2*momentumX*∂u/∂y // From momentum-y: v appears in convection term v*∂v/∂y → ∂R/∂v += 2*momentumY*∂v/∂y gradient.OutputGradients[1] = NumOps.Add( NumOps.Multiply(two, NumOps.Multiply(momentumX, dudy)), NumOps.Multiply(two, NumOps.Multiply(momentumY, dvdy))); + +// Gradient w.r.t. p (output 2) +// Pressure p does not appear directly in residual (only its derivatives), so ∂R/∂p = 0 +gradient.OutputGradients[2] = NumOps.Zero;src/PhysicsInformed/GpuPINNTrainer.cs (1)
263-267: Method resets state but doesn't release actual GPU resources.Currently, this method only resets flags. If actual GPU resources (buffers, contexts) are added in the future, this method would need to be updated to dispose them. Consider adding a note in the XML doc or renaming to
ResetGpuState()to clarify current behavior.src/NeuralNetworks/OctonionNeuralNetwork.cs (2)
214-240: Consider try/finally for training mode consistency.Unlike
Predict(lines 142-152),Traindoesn't wrap the training mode state changes in try/finally. If an exception occurs between lines 216 and 239,IsTrainingModeremainstrue, which could affect subsequent operations.For consistency and robustness, consider using the same exception-safety pattern as
Predict.🔎 Proposed refactor for consistency
public override void Train(Tensor<T> input, Tensor<T> expectedOutput) { - IsTrainingMode = true; + var previousTrainingMode = IsTrainingMode; + IsTrainingMode = true; - var prediction = Forward(input); - - var primaryLoss = _lossFunction.CalculateLoss(prediction.ToVector(), expectedOutput.ToVector()); - - T auxiliaryLoss = NumOps.Zero; - foreach (var auxLayer in Layers.OfType<IAuxiliaryLossLayer<T>>().Where(l => l.UseAuxiliaryLoss)) + try { - var layerAuxLoss = auxLayer.ComputeAuxiliaryLoss(); - var weightedAuxLoss = NumOps.Multiply(layerAuxLoss, auxLayer.AuxiliaryLossWeight); - auxiliaryLoss = NumOps.Add(auxiliaryLoss, weightedAuxLoss); - } + var prediction = Forward(input); - LastLoss = NumOps.Add(primaryLoss, auxiliaryLoss); + var primaryLoss = _lossFunction.CalculateLoss(prediction.ToVector(), expectedOutput.ToVector()); - var outputGradient = _lossFunction.CalculateDerivative(prediction.ToVector(), expectedOutput.ToVector()); - var outputGradientTensor = Tensor<T>.FromVector(outputGradient); + T auxiliaryLoss = NumOps.Zero; + foreach (var auxLayer in Layers.OfType<IAuxiliaryLossLayer<T>>().Where(l => l.UseAuxiliaryLoss)) + { + var layerAuxLoss = auxLayer.ComputeAuxiliaryLoss(); + var weightedAuxLoss = NumOps.Multiply(layerAuxLoss, auxLayer.AuxiliaryLossWeight); + auxiliaryLoss = NumOps.Add(auxiliaryLoss, weightedAuxLoss); + } - Backward(outputGradientTensor); + LastLoss = NumOps.Add(primaryLoss, auxiliaryLoss); - _optimizer.UpdateParameters(Layers); + var outputGradient = _lossFunction.CalculateDerivative(prediction.ToVector(), expectedOutput.ToVector()); + var outputGradientTensor = Tensor<T>.FromVector(outputGradient); - IsTrainingMode = false; + Backward(outputGradientTensor); + + _optimizer.UpdateParameters(Layers); + } + finally + { + IsTrainingMode = previousTrainingMode; + } }
302-312: Fresh optimizer instance correctly created for clones.The documentation and implementation correctly avoid sharing optimizer state by passing
nullto create a fresh optimizer instance. Good attention to stateful component isolation.Note: The loss function instance is shared (line 310), which is typically fine since loss functions are stateless, but this creates a design inconsistency with the optimizer approach. Consider whether loss function should also be freshly instantiated for complete independence, though the current approach is likely safe in practice.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/Helpers/LayerHelper.cssrc/NeuralNetworks/HyperbolicNeuralNetwork.cssrc/NeuralNetworks/OctonionNeuralNetwork.cssrc/PhysicsInformed/DomainDecompositionTrainingHistory.cssrc/PhysicsInformed/FiniteDifferenceGradient.cssrc/PhysicsInformed/GpuPINNTrainer.cssrc/PhysicsInformed/PDEs/LinearElasticityEquation.cssrc/PhysicsInformed/PDEs/NavierStokesEquation.cssrc/PhysicsInformed/PDEs/SchrodingerEquation.cstests/AiDotNet.Tests/UnitTests/PhysicsInformed/PDEs/PDEAnalyticalSolutionTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/NeuralNetworks/HyperbolicNeuralNetwork.cs
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
src/PhysicsInformed/FiniteDifferenceGradient.cssrc/PhysicsInformed/PDEs/LinearElasticityEquation.cssrc/PhysicsInformed/DomainDecompositionTrainingHistory.cssrc/NeuralNetworks/OctonionNeuralNetwork.cssrc/PhysicsInformed/GpuPINNTrainer.cssrc/PhysicsInformed/PDEs/NavierStokesEquation.cssrc/Helpers/LayerHelper.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
src/PhysicsInformed/FiniteDifferenceGradient.cssrc/PhysicsInformed/PDEs/LinearElasticityEquation.cssrc/PhysicsInformed/DomainDecompositionTrainingHistory.cssrc/NeuralNetworks/OctonionNeuralNetwork.cssrc/PhysicsInformed/GpuPINNTrainer.cssrc/PhysicsInformed/PDEs/NavierStokesEquation.cssrc/Helpers/LayerHelper.cs
📚 Learning: 2025-11-19T04:08:26.895Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 0
File: :0-0
Timestamp: 2025-11-19T04:08:26.895Z
Learning: For ILGPU GPU operations in GpuEngine.cs, use standard .NET exception types (InvalidOperationException, ArgumentException, OutOfMemoryException) instead of ILGPU-specific exception types, as ILGPU exception types may be version-specific. Combine with message-based filtering using ex.Message.Contains("device") or ex.Message.Contains("accelerator") as a fallback for GPU-specific errors.
Applied to files:
src/PhysicsInformed/GpuPINNTrainer.cs
🔇 Additional comments (28)
src/PhysicsInformed/DomainDecompositionTrainingHistory.cs (3)
1-5: LGTM!The using directives and namespace declaration are appropriate. Based on learnings, global usings cover the core types like Vector, Matrix, and Tensor.
27-47: LGTM!The class structure and properties are well-designed. All collections are properly initialized, preventing null reference issues, and the get-only properties provide appropriate encapsulation for the training history data.
54-65: LGTM!The constructor now properly validates
subdomainCountand throws a clear exception for invalid values. This addresses the previous review comment about missing validation.src/PhysicsInformed/FiniteDifferenceGradient.cs (2)
79-89: Epsilon validation successfully addresses previous review concern.The epsilon validation now correctly ensures the parameter is positive and finite, preventing division-by-zero and incorrect gradient computation. This fully resolves the issue flagged in the previous review.
52-114: Well-designed finite-difference gradient implementation.The implementation correctly applies the central difference formula with proper type-safe generic operations. The documentation is comprehensive and appropriately warns against misuse for neural network training. Input validation is thorough, and the algorithm correctly restores parameters after perturbations.
src/PhysicsInformed/PDEs/LinearElasticityEquation.cs (4)
94-115: LGTM! Correct engineering constants conversion.The validation ranges for Young's modulus and Poisson's ratio are appropriate, and the conversion formulas to Lamé parameters are mathematically correct. The bounds on Poisson's ratio properly prevent singularities and ensure physically meaningful material properties.
118-159: LGTM! Correct implementation of 2D linear elasticity residuals.The residual computation correctly implements the expanded Navier-Cauchy equations for 2D linear elasticity:
- Equation 1: (λ+2μ)∂²u/∂x² + μ∂²u/∂y² + (λ+μ)∂²v/∂x∂y + fₓ
- Equation 2: (λ+μ)∂²u/∂x∂y + μ∂²v/∂x² + (λ+2μ)∂²v/∂y² + fᵧ
The sum of squared residuals (R₁² + R₂²) is appropriate for the loss function.
171-229: LGTM! Gradient computation correctly handles cross-equation coupling.The gradient computation is mathematically correct and properly implements the cross-coupling terms noted in the previous review:
- Line 217:
∂R/∂(∂²u/∂x∂y) = 2*R₂*(λ+μ)correctly uses R₂ because ∂²u/∂x∂y appears in residual equation 2- Line 226:
∂R/∂(∂²v/∂x∂y) = 2*R₁*(λ+μ)correctly uses R₁ because ∂²v/∂x∂y appears in residual equation 1The indexing convention
[outputIndex, derivative1, derivative2]is used consistently, and the past review comment regarding cross-derivative coupling has been properly addressed.
232-238: LGTM! Properties correctly specify 2D linear elasticity problem.The dimensions are correct for 2D linear elasticity (2 spatial coordinates, 2 displacement components), and the name provides useful parameter information for debugging and logging.
src/Helpers/LayerHelper.cs (6)
2752-2809: LGTM! Well-designed Hamiltonian network architecture.The implementation correctly:
- Enforces scalar output (Hamiltonian must be single energy value)
- Uses Tanh activation for smooth gradients required by Hamilton's equations
- Applies linear output since energy is unbounded
- Provides excellent documentation explaining the physics
2811-2864: LGTM! Appropriate Lagrangian network architecture.The implementation correctly models the Lagrangian formulation with scalar output, Tanh activations for smooth second derivatives, and clear documentation of the physical principles.
2866-2911: LGTM! Suitable architecture for Universal Differential Equations.The UDE component correctly uses Tanh for smooth ODE integration and linear output for unbounded dynamics corrections. Documentation clearly explains the hybrid physics-ML approach.
3068-3092: LGTM! Appropriate architecture for Variational PINNs.The implementation correctly uses Tanh activation for smooth derivatives required in the weak formulation, with linear output for PDE solutions. Documentation clearly explains the variational approach and its advantages over strong-form PINNs.
3112-3136: LGTM! Suitable Deep Ritz Method architecture.The implementation correctly uses Tanh for smooth second derivatives needed in energy functional minimization, with linear output. Documentation clearly explains the energy-based approach from calculus of variations.
3158-3182: LGTM! Standard and appropriate PINN architecture.The implementation correctly uses Tanh activation for smooth derivatives essential to PDE residual computation, with linear output for solution values. Multiple hidden layers support complex PDE solutions, and documentation clearly explains the physics-informed training approach.
src/PhysicsInformed/PDEs/NavierStokesEquation.cs (3)
60-88: LGTM! Well-structured constructors with proper validation.The constructors correctly validate positive viscosity and density, handle the optional density parameter with a sensible default (1), and provide a convenient double-parameter overload for ease of use.
99-173: LGTM! Correct Navier-Stokes residual computation.The residual computation correctly implements the incompressible Navier-Stokes equations with proper continuity and momentum terms. The physics equations match the standard formulation with density scaling for pressure gradients and viscous diffusion terms.
264-270: LGTM! Properties correctly define dimensions and identification.The properties appropriately specify the input dimension (spatial x, y and temporal t), output dimension (velocity components u, v and pressure p), and provide a descriptive name with parameter values for runtime identification.
src/PhysicsInformed/GpuPINNTrainer.cs (8)
17-43: Well-documented class with clear beginner guidance.The class-level documentation with usage examples and auto-fallback explanation is helpful for users new to GPU-accelerated PINN training.
65-79: Constructor handles null inputs and conditional GPU initialization correctly.
85-135: GPU initialization with proper fallback and error handling.The design correctly sets
_gpuInitialized = trueeven on failure to prevent repeated failed initialization attempts. The verbose logging helps with debugging.
246-258: LGTM.The logic correctly handles GPU enable/disable transitions. Not calling
ReleaseGpuResourceswhen disabling is acceptable since re-enabling would otherwise require re-initialization.
285-302: Improved handling of unavailable GPU memory info.The use of
-1sentinel values with comprehensive documentation explaining ILGPU limitations is a reasonable approach. TheIsMemoryInfoAvailableproperty inPINNGpuMemoryInfohelps consumers determine if the data is valid.
304-314: GPU detection now properly implemented.The method correctly delegates to
engine.SupportsGpuinstead of the previous placeholder that always returned true. This addresses the prior review concern.
321-347: Clean extension of TrainingHistory with GPU metrics.The computed
AverageEpochTimeMsproperty correctly derives fromTotalTrainingTimeMsandLosses.Count. The dictionary initialization forKernelTimingsavoids null reference issues.
364-396: Well-designed data class with proper sentinel handling.The
UsagePercentagecomputation correctly guards against invalid states, andIsMemoryInfoAvailableproperly distinguishes between valid data and the-1sentinel. The extensive remarks guide users toward external profiling tools.src/NeuralNetworks/OctonionNeuralNetwork.cs (2)
9-30: Excellent documentation for octonion neural networks.The XML documentation provides both technical depth and beginner-friendly explanations, making the 8-dimensional hypercomplex algebra approachable.
137-153: Exception-safe training mode restoration implemented correctly.The try/finally block ensures
IsTrainingModeis restored even ifForwardthrows an exception. Good implementation of exception safety.
Adds Physics-Informed Neural Networks (PINNs) and Neural Operators: - PDE specifications: Heat, Wave, Burgers, Navier-Stokes, Allen-Cahn - Neural operators: FNO with spectral convolution, DeepONet - GPU-accelerated training with memory tracking - Multi-fidelity and domain decomposition PINNs - Automatic differentiation for physics constraints Adds advanced algebra primitives: - Sparse tensor operations with COO/CSR formats - Hyperbolic manifolds: Poincare ball and hyperboloid - Lie groups: SO(3), SE(3), SU(2) - Clifford algebra and multivectors - Octonion operations Adds comprehensive test coverage for all components. Closes #400 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
abf546e to
5239c6b
Compare
ooples
changed the title
Merged origin/master which added 3D layer methods (VoxelCNN, UNet3D, MeshCNN, SpiralNet) into PR branch. Also added missing Floor, Ceiling, Frac, Sin, Cos methods to OctonionOperations, MultivectorOperations, and VectorizedOperationsFallback to satisfy new interface requirements. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (35)
src/PhysicsInformed/MultiFidelityTrainingHistory.cs (2)
28-46: Consider exposing properties as IReadOnlyList for better encapsulation.The properties currently expose
List<T>directly, which allows external code to modify the collections viaAdd,Remove,Clear, etc. For a history-tracking class, consider exposing them asIReadOnlyList<T>while maintaining privateList<T>backing fields.🔎 Proposed refactor
- /// <summary> - /// Gets the low-fidelity data losses per epoch. - /// </summary> - public List<T> LowFidelityLosses { get; } = new List<T>(); + private readonly List<T> _lowFidelityLosses = new List<T>(); + + /// <summary> + /// Gets the low-fidelity data losses per epoch. + /// </summary> + public IReadOnlyList<T> LowFidelityLosses => _lowFidelityLosses; - /// <summary> - /// Gets the high-fidelity data losses per epoch. - /// </summary> - public List<T> HighFidelityLosses { get; } = new List<T>(); + private readonly List<T> _highFidelityLosses = new List<T>(); + + /// <summary> + /// Gets the high-fidelity data losses per epoch. + /// </summary> + public IReadOnlyList<T> HighFidelityLosses => _highFidelityLosses; - /// <summary> - /// Gets the correlation losses per epoch. - /// </summary> - public List<T> CorrelationLosses { get; } = new List<T>(); + private readonly List<T> _correlationLosses = new List<T>(); + + /// <summary> + /// Gets the correlation losses per epoch. + /// </summary> + public IReadOnlyList<T> CorrelationLosses => _correlationLosses; - /// <summary> - /// Gets the PDE residual losses per epoch. - /// </summary> - public List<T> PhysicsLosses { get; } = new List<T>(); + private readonly List<T> _physicsLosses = new List<T>(); + + /// <summary> + /// Gets the PDE residual losses per epoch. + /// </summary> + public IReadOnlyList<T> PhysicsLosses => _physicsLosses;And update the AddEpoch method to use the private fields:
public void AddEpoch(T totalLoss, T lowFidelityLoss, T highFidelityLoss, T correlationLoss, T physicsLoss) { AddEpoch(totalLoss); // Base class tracks total loss - LowFidelityLosses.Add(lowFidelityLoss); - HighFidelityLosses.Add(highFidelityLoss); - CorrelationLosses.Add(correlationLoss); - PhysicsLosses.Add(physicsLoss); + _lowFidelityLosses.Add(lowFidelityLoss); + _highFidelityLosses.Add(highFidelityLoss); + _correlationLosses.Add(correlationLoss); + _physicsLosses.Add(physicsLoss); }
48-63: Implementation looks good.The method correctly delegates total loss tracking to the base class and maintains the individual fidelity loss components. The logic is clear and the documentation is comprehensive.
Optional enhancements to consider:
- Parameter validation: If loss values should be non-negative or finite, consider adding validation (e.g., checking for
NaNor negative values).- Thread safety: If training history might be accessed from multiple threads during training, consider adding synchronization.
docs/PHYSICS_AI_IMPLEMENTATION_PLAN.md (2)
167-168: Expand truncated code example with complete layer creation.Lines 167–168 contain
... more layers ...which is pseudocode. For clarity, show a complete example of how multiple hidden layers are created, or at least one full hidden layer plus the output layer.🔎 Proposed improvement
var hiddenActivation = new TanhActivation<T>() as IActivationFunction<T>; yield return new DenseLayer<T>(inputSize, hiddenLayerSize, hiddenActivation); - // ... more layers ... + + for (int i = 1; i < hiddenLayerCount; i++) + { + yield return new DenseLayer<T>(hiddenLayerSize, hiddenLayerSize, hiddenActivation); + } + yield return new DenseLayer<T>(hiddenLayerSize, 1, new IdentityActivation<T>());
223-223: Consider replacing "with respect to" with more concise phrasing.Static analysis suggests the phrase "with respect to" is wordy. Consider shorter alternatives:
- Line 223: "derivative of loss of weights" or "loss gradient for weights"
- Line 244: "derivative of Hamiltonian for position" or "position gradient in Hamiltonian"
- Lines 265–266: "gradients computed for the network's weights" and "gradients computed for the inputs"
This is a style preference, but more concise phrasing improves readability for the beginner-friendly audience.
Also applies to: 244-244, 265-266
src/AiDotNet.Tensors/Topology/Simplex.cs (3)
35-57: Consider optimizing the Boundary method to skip redundant validation.The
Boundarymethod creates face vertices that are already sorted and unique (as a subset of the already-validatedVertices), but theSimplexconstructor re-sorts and re-checks uniqueness. For large simplices, this adds unnecessary overhead.🔎 Proposed optimization
Add a private constructor that bypasses validation for internal use:
private Simplex(int[] vertices, bool skipValidation) { Vertices = vertices; } public IReadOnlyList<(int Sign, Simplex Face)> Boundary() { if (Vertices.Length <= 1) return Array.Empty<(int, Simplex)>(); var faces = new List<(int, Simplex)>(Vertices.Length); for (int i = 0; i < Vertices.Length; i++) { var faceVertices = Vertices.Where((_, idx) => idx != i).ToArray(); int sign = (i % 2 == 0) ? 1 : -1; faces.Add((sign, new Simplex(faceVertices, skipValidation: true))); } return faces; }Alternatively, if you prefer the explicit array approach for performance, keep the manual loop but use the private constructor to skip validation.
59-72: Consider using SequenceEqual for cleaner equality check.The manual loop comparing vertices is correct but could be simplified using LINQ's
SequenceEqualfor better readability. The performance difference is negligible for typical simplex sizes.🔎 Proposed simplification
public bool Equals(Simplex? other) { - if (other is null) - return false; - if (Vertices.Length != other.Vertices.Length) - return false; - - for (int i = 0; i < Vertices.Length; i++) - { - if (Vertices[i] != other.Vertices[i]) - return false; - } - return true; + return other is not null && Vertices.SequenceEqual(other.Vertices); }
10-11: Consider adding equality operators for consistency.The class implements
IEquatable<Simplex>and has value-like semantics, but does not override the==and!=operators. For consistency and to meet user expectations, consider adding these operators.🔎 Proposed addition
public static bool operator ==(Simplex? left, Simplex? right) { if (left is null) return right is null; return left.Equals(right); } public static bool operator !=(Simplex? left, Simplex? right) { return !(left == right); }src/PhysicsInformed/PDEs/AdvectionDiffusionEquation.cs (1)
50-50: Optional: Rename field to follow C# naming conventions.The field
_sourcetermshould be_sourceTerm(capital T) to follow standard C# naming conventions for compound words in camelCase.src/Data/Loaders/UniformEpisodicDataLoader.cs (1)
240-254: Consider adding a brief comment explaining the distinctness criteria.The compound condition on lines 252-253 has subtle intent: reject tasks with identical class composition and the same first support index (as a proxy for sampling overlap). A short comment would clarify why
FirstSupportIndexis part of the heuristic.🔎 Suggested clarifying comment
+ // Full signature match → identical task if (candidate.Signature == _lastTaskSignature) { return false; } + // Same class set + same first support index likely indicates high overlap return !(candidate.ClassSignature == _lastClassSignature && candidate.FirstSupportIndex == _lastFirstSupportIndex);scripts/run-tests-sharded.ps1 (1)
251-253: Consider using helper functions for consistency.Lines 251-253 use string interpolation for filter construction, while other shards use the new helper functions. For consistency, consider refactoring to use
New-NamespaceFilteror at leastJoin-FilterParts.🔎 Proposed refactor for consistency
- $shards.Add((New-TestShard -Name "AiDotNet.Tests - Other - 13 InferenceOptimization" -Project "tests\AiDotNet.Tests\AiDotNetTests.csproj" -Filter "$categoryFilter&FullyQualifiedName~AiDotNet.Tests.InferenceOptimization")) - $shards.Add((New-TestShard -Name "AiDotNet.Tests - Other - 14 PromptEngineering" -Project "tests\AiDotNet.Tests\AiDotNetTests.csproj" -Filter "$categoryFilter&FullyQualifiedName~AiDotNet.Tests.PromptEngineering")) - $shards.Add((New-TestShard -Name "AiDotNet.Tests - Other - 15 Recovery/Concurrency" -Project "tests\AiDotNet.Tests\AiDotNetTests.csproj" -Filter "$categoryFilter&FullyQualifiedName~AiDotNet.Tests.Concurrency|$categoryFilter&FullyQualifiedName~AiDotNet.Tests.Recovery")) + $shards.Add((New-TestShard -Name "AiDotNet.Tests - Other - 13 InferenceOptimization" -Project "tests\AiDotNet.Tests\AiDotNetTests.csproj" -Filter (New-NamespaceFilter -CategoryFilter $categoryFilter -Namespaces @("AiDotNet.Tests.InferenceOptimization")))) + $shards.Add((New-TestShard -Name "AiDotNet.Tests - Other - 14 PromptEngineering" -Project "tests\AiDotNet.Tests\AiDotNetTests.csproj" -Filter (New-NamespaceFilter -CategoryFilter $categoryFilter -Namespaces @("AiDotNet.Tests.PromptEngineering")))) + $shards.Add((New-TestShard -Name "AiDotNet.Tests - Other - 15 Recovery/Concurrency" -Project "tests\AiDotNet.Tests\AiDotNetTests.csproj" -Filter (New-NamespaceFilter -CategoryFilter $categoryFilter -Namespaces @("AiDotNet.Tests.Concurrency", "AiDotNet.Tests.Recovery"))))src/PhysicsInformed/GpuPINNTrainer.cs (2)
193-196: Property nameKernelTimingsis misleading.The
KernelTimingsdictionary stores general training metrics (epoch count, average epoch time) rather than actual GPU kernel execution timings. Consider renaming toTrainingMetricsor documenting that this is repurposed for general statistics.🔎 Suggested refactor
// Record timing statistics if (epochs > 0) { - history.KernelTimings["TotalEpochs"] = epochs; - history.KernelTimings["AverageEpochMs"] = stopwatch.ElapsedMilliseconds / epochs; + // Store general training statistics + history.KernelTimings["TotalEpochs"] = epochs; // Note: "KernelTimings" used for general metrics + history.KernelTimings["AverageEpochMs"] = stopwatch.ElapsedMilliseconds / epochs; }Or better, rename the property in
GpuTrainingHistory<T>:-public Dictionary<string, long> KernelTimings { get; set; } = new Dictionary<string, long>(); +public Dictionary<string, long> TrainingMetrics { get; set; } = new Dictionary<string, long>();
37-350: Consider implementingIDisposablefor deterministic GPU resource cleanup.The
GpuPINNTrainer<T>class manages GPU resources but doesn't implementIDisposable. Users must remember to manually callReleaseGpuResources(), which is error-prone and could lead to resource leaks.🔎 Suggested implementation
-public class GpuPINNTrainer<T> +public class GpuPINNTrainer<T> : IDisposable { private readonly PhysicsInformedNeuralNetwork<T> _pinn; private readonly INumericOperations<T> _numOps; private bool _gpuInitialized; private bool _useGpu; private GpuPINNTrainingOptions _options; + private bool _disposed; // ... existing code ... /// <summary> /// Releases GPU resources. /// </summary> public void ReleaseGpuResources() { _useGpu = false; _gpuInitialized = false; } + + /// <summary> + /// Releases all resources used by the trainer. + /// </summary> + public void Dispose() + { + Dispose(true); + GC.SuppressFinalize(this); + } + + protected virtual void Dispose(bool disposing) + { + if (!_disposed) + { + if (disposing) + { + ReleaseGpuResources(); + } + _disposed = true; + } + } }SPRINT_PLAN_PR449_ISSUE400.md (1)
4-5: Wrap URLs in angle brackets or use markdown link syntax.The static analysis tool flags bare URLs. Consider wrapping them in angle brackets (
<URL>) or using proper markdown link syntax ([text](URL)) for better formatting and accessibility.🔎 Proposed fix
## Sources Reviewed -- PR #449: https://github.com/ooples/AiDotNet/pull/449 (body + checklist) -- Issue #400: https://github.com/ooples/AiDotNet/issues/400 (problem statement + success criteria) +- PR #449: <https://github.com/ooples/AiDotNet/pull/449> (body + checklist) +- Issue #400: <https://github.com/ooples/AiDotNet/issues/400> (problem statement + success criteria)src/Autodiff/NeuralNetworkDerivatives.cs (2)
335-423: Consider making finite-difference step size configurable.The hardcoded
step = 1e-5(Line 339) may not be appropriate for all numeric types or problem scales. For problems with very large or small magnitudes, this could lead to numerical instability or loss of precision.Consider adding an optional parameter to allow users to specify the step size when automatic differentiation is unavailable, or implement adaptive step sizing based on input magnitudes.
625-731: Document supported activation functions.The implementation supports Identity, Tanh, Sigmoid, ReLU, LeakyReLU, and GELU, but throws
NotSupportedExceptionfor others (Line 729). This is appropriate behavior, but users need to know which activations are supported for second-order derivatives.Consider adding XML documentation or a summary comment listing the supported activation functions to help users avoid runtime exceptions.
src/PhysicsInformed/Benchmarks/OperatorDataset2D.cs (1)
3-10: Consider adding XML documentation for the public DTO.This benchmark data container lacks XML documentation comments, which would help consumers understand the purpose and constraints of each property (e.g., expected dimensions of Inputs/Outputs arrays, meaning of GridSize vs SampleCount).
💡 Optional: Add XML documentation
+ /// <summary> + /// Represents a 2D operator dataset for benchmarking. + /// </summary> public sealed class OperatorDataset2D { + /// <summary> + /// Gets or sets the name of the operator being benchmarked. + /// </summary> public string OperatorName { get; set; } = string.Empty; + + /// <summary> + /// Gets or sets the spatial grid size (e.g., 64 for a 64x64 grid). + /// </summary> public int GridSize { get; set; } + + /// <summary> + /// Gets or sets the number of samples in the dataset. + /// </summary> public int SampleCount { get; set; } + + /// <summary> + /// Gets or sets the input data with shape [samples, gridSize, gridSize]. + /// </summary> public double[,,] Inputs { get; set; } = new double[0, 0, 0]; + + /// <summary> + /// Gets or sets the output data with shape [samples, gridSize, gridSize]. + /// </summary> public double[,,] Outputs { get; set; } = new double[0, 0, 0]; }src/AiDotNet.Tensors/LinearAlgebra/SparseStorageFormat.cs (1)
6-11: Consider documenting individual enum members for clarity.While the enum itself is documented, adding XML comments to each member would help developers unfamiliar with sparse matrix formats understand the differences (e.g., "COO stores (row, col, value) triplets", "CSR is efficient for row-wise operations", etc.).
💡 Optional: Add member documentation
public enum SparseStorageFormat { + /// <summary> + /// Coordinate (COO) format: stores (row, col, value) triplets. + /// Simple but less efficient for operations. + /// </summary> Coo, + + /// <summary> + /// Compressed Sparse Row (CSR) format: efficient for row-wise operations. + /// </summary> Csr, + + /// <summary> + /// Compressed Sparse Column (CSC) format: efficient for column-wise operations. + /// </summary> Csc }src/PhysicsInformed/Benchmarks/OperatorBenchmarkResult.cs (1)
3-12: Consider adding XML documentation for benchmark metrics.While the property names are fairly self-descriptive, documenting the specific calculations (e.g., how
RelativeL2Erroris normalized, whetherMseis per-sample or total) would help consumers interpret benchmark results correctly.💡 Optional: Add XML documentation
+ /// <summary> + /// Contains benchmark results for a neural operator on 2D data. + /// </summary> public sealed class OperatorBenchmarkResult { + /// <summary> + /// Gets or sets the name of the operator being benchmarked. + /// </summary> public string OperatorName { get; set; } = string.Empty; + + /// <summary> + /// Gets or sets the number of spatial grid points (e.g., 64 for a 64x64 grid). + /// </summary> public int SpatialPoints { get; set; } + + /// <summary> + /// Gets or sets the number of samples evaluated. + /// </summary> public int SampleCount { get; set; } + + /// <summary> + /// Gets or sets the mean squared error. + /// </summary> public double Mse { get; set; } + + /// <summary> + /// Gets or sets the L2 norm of the error. + /// </summary> public double L2Error { get; set; } + + /// <summary> + /// Gets or sets the relative L2 error (L2 error normalized by the L2 norm of the target). + /// </summary> public double RelativeL2Error { get; set; } + + /// <summary> + /// Gets or sets the maximum pointwise error. + /// </summary> public double MaxError { get; set; } }src/PhysicsInformed/Benchmarks/PdeBenchmarkResult.cs (1)
3-11: LGTM! Clean benchmark result container.The data structure is well-suited for capturing PDE benchmark metrics with appropriate property types for counts and error measurements.
Optional: Consider init-only properties for immutability
If benchmark results shouldn't be modified after creation, you could use
initsetters:public sealed class PdeBenchmarkResult { - public string EquationName { get; set; } = string.Empty; - public int SpatialPoints { get; set; } - public int TimeSteps { get; set; } - public double FinalTime { get; set; } - public double L2Error { get; set; } - public double MaxError { get; set; } + public string EquationName { get; init; } = string.Empty; + public int SpatialPoints { get; init; } + public int TimeSteps { get; init; } + public double FinalTime { get; init; } + public double L2Error { get; init; } + public double MaxError { get; init; } }src/AiDotNet.Tensors/Groups/Su2Group.cs (1)
65-87: Consider handling negativewin Log for numerical robustness.The current implementation handles small
normcorrectly by returning zero. However, whenw < 0(representing a rotation > π), the quaternion(-w, -x, -y, -z)represents the same rotation butatan2would give different results. Since SU(2) double-covers SO(3), this may be intentional, but consider documenting the choice or normalizing tow ≥ 0before computing the log.🔎 Optional: Normalize to positive w for consistent angle range
public Vector<T> Log(Su2<T> value) { double w = Convert.ToDouble(value.W); double x = Convert.ToDouble(value.X); double y = Convert.ToDouble(value.Y); double z = Convert.ToDouble(value.Z); + + // Normalize to w >= 0 for consistent angle in [0, π] + if (w < 0) + { + w = -w; + x = -x; + y = -y; + z = -z; + } + double norm = Math.Sqrt(x * x + y * y + z * z);src/AiDotNet.Tensors/Manifolds/PoincareBallManifold.cs (3)
54-71: ExpMap has potential numerical instability for small tangent norms.The check
if (_ops.Equals(norm, _ops.Zero))uses exact equality, but for floating-point arithmetic, very small norms could still proceed and cause numerical issues in the division at Line 67 (scale = tanh / (sqrtC * norm)). Consider using a tolerance-based check instead.🔎 Suggested fix
public Vector<T> ExpMap(Vector<T> tangent, Vector<T> basePoint) { EnsureSameLength(tangent, basePoint); T norm = Norm(tangent); - if (_ops.Equals(norm, _ops.Zero)) + if (_ops.LessThanOrEquals(norm, _epsilon)) return basePoint;
73-89: LogMap has the same zero-norm tolerance issue.Similar to ExpMap, the exact zero check could allow very small norms through, causing numerical instability in the division at Line 87.
🔎 Suggested fix
var diff = MobiusAdd(Negate(basePoint), point); T norm = Norm(diff); - if (_ops.Equals(norm, _ops.Zero)) + if (_ops.LessThanOrEquals(norm, _epsilon)) return CreateZeroVector(point.Length);
184-188: Curvature validation uses Convert.ToDouble which may lose precision for decimal type.The validation
Convert.ToDouble(Curvature) <= 0.0works forfloatanddoublebut could lose precision fordecimaltypes. Since this is a validation check, consider using the generic comparison operations instead.🔎 Suggested fix
private void ValidateCurvature() { - if (Convert.ToDouble(Curvature) <= 0.0) + if (_ops.LessThanOrEquals(Curvature, _ops.Zero)) throw new ArgumentOutOfRangeException(nameof(Curvature), "Curvature must be positive."); }src/AiDotNet.Tensors/LinearAlgebra/Octonion.cs (1)
272-287: GetHashCode implementation is functional but could use HashCode.Combine for better distribution.The manual hash computation works but modern .NET provides
HashCode.Combinewhich offers better hash distribution. This is a minor improvement opportunity.🔎 Suggested improvement
public override int GetHashCode() { - unchecked - { - int hash = 17; - hash = hash * 23 + (Scalar?.GetHashCode() ?? 0); - hash = hash * 23 + (E1?.GetHashCode() ?? 0); - hash = hash * 23 + (E2?.GetHashCode() ?? 0); - hash = hash * 23 + (E3?.GetHashCode() ?? 0); - hash = hash * 23 + (E4?.GetHashCode() ?? 0); - hash = hash * 23 + (E5?.GetHashCode() ?? 0); - hash = hash * 23 + (E6?.GetHashCode() ?? 0); - hash = hash * 23 + (E7?.GetHashCode() ?? 0); - return hash; - } + return HashCode.Combine(Scalar, E1, E2, E3, E4, E5, E6, E7); }src/NeuralNetworks/Layers/OctonionLinearLayer.cs (1)
127-141: Xavier initialization with fixed seed may cause reproducibility issues in production.Using a fixed seed (42) for initialization is good for reproducibility during development/testing, but may be problematic if multiple layers are created (they'll have identical initial weights). Consider adding an optional seed parameter or using a shared random source.
🔎 Suggested improvement
public OctonionLinearLayer( int inputFeatures, int outputFeatures, - IActivationFunction<T>? activationFunction = null) + IActivationFunction<T>? activationFunction = null, + int? seed = null) : base(...) { // ... - InitializeParameters(); + InitializeParameters(seed); } -private void InitializeParameters() +private void InitializeParameters(int? seed) { var scale = Math.Sqrt(2.0 / (InputFeatures + OutputFeatures)); - var random = RandomHelper.CreateSeededRandom(42); + var random = seed.HasValue + ? RandomHelper.CreateSeededRandom(seed.Value) + : RandomHelper.CreateRandom();src/AiDotNet.Tensors/Engines/CpuSparseEngine.cs (1)
384-395: SparseScatter silently overwrites duplicate indices.When
indicescontains duplicate (row, col) pairs, only the last value is retained. This differs fromSparseScatterAddwhich accumulates values. Consider documenting this behavior or throwing on duplicates if overwrites are unintended.+ // Note: If indices contain duplicates, only the last value for each (row, col) is retained. + // Use SparseScatterAdd for accumulation semantics. for (int i = 0; i < values.Length; i++) {src/AiDotNet.Tensors/LinearAlgebra/CliffordAlgebra.cs (1)
152-161: Consider using BitOperations.PopCount for better performance.The manual bit-counting loop works correctly, but .NET provides an optimized intrinsic that uses hardware instructions when available.
🔎 Optional optimization
+using System.Numerics; + private static int CountBits(int value) { - int count = 0; - while (value != 0) - { - count += value & 1; - value >>= 1; - } - return count; + return BitOperations.PopCount((uint)value); }src/NeuralNetworks/OctonionNeuralNetwork.cs (1)
215-241: Consider wrapping Train in try/finally for consistency with Predict.The
Trainmethod setsIsTrainingMode = trueat the start andfalseat the end, but if an exception occurs during training, the mode won't be restored. While training exceptions may warrant different handling, wrapping in try/finally would be consistent withPredict.🔎 Optional: Add exception safety
public override void Train(Tensor<T> input, Tensor<T> expectedOutput) { IsTrainingMode = true; - - var prediction = Forward(input); - // ... rest of training logic ... - _optimizer.UpdateParameters(Layers); - - IsTrainingMode = false; + try + { + var prediction = Forward(input); + // ... rest of training logic ... + _optimizer.UpdateParameters(Layers); + } + finally + { + IsTrainingMode = false; + } }src/PhysicsInformed/Interfaces/IMultiScalePDE.cs (2)
63-133: Add validation guidance for scale index parameters.The interface methods accept
scaleIndexparameters but don't specify validation requirements in their documentation. Implementers may not validate that0 <= scaleIndex < NumberOfScales, leading to index-out-of-bounds errors.Add validation guidance to the XML documentation:
/// <param name="scaleIndex">The index of the scale (0 = coarsest, increasing = finer). /// Must satisfy: 0 <= scaleIndex < NumberOfScales.</param> /// <exception cref="ArgumentOutOfRangeException">Thrown when scaleIndex is invalid.</exception>Apply similar documentation to all methods accepting
scaleIndex,coarseIndex, orfineIndexparameters (lines 66, 76-77, 107, 125, 151, 159-160).
181-213: Consider validation for ManualScaleWeights array length.
ManualScaleWeights(line 212) could have an incorrect length that doesn't matchNumberOfScales. Add documentation specifying the expected length and recommend validation when this option is used.Add to the XML documentation:
/// <summary> /// Individual weights for each scale (overrides automatic weighting). /// </summary> /// <remarks> /// If provided, the array length must equal NumberOfScales. /// Implementations should validate this constraint during training initialization. /// </remarks>src/PhysicsInformed/Interfaces/IInverseProblem.cs (2)
120-136: Specify behavior when MeasurementNoiseLevel is accessed without HasMeasurementNoiseLevel being true.The documentation advises checking
HasMeasurementNoiseLevelbefore accessingMeasurementNoiseLevel(line 133), but doesn't specify what happens if this contract is violated. Should implementations:
- Return
default(T)(but this is ambiguous with a true zero noise level)?- Throw an
InvalidOperationException?Add exception specification to the documentation:
/// <exception cref="InvalidOperationException"> /// Thrown when accessed while HasMeasurementNoiseLevel is false. /// </exception>Alternatively, consider making this property nullable (
T?) to eliminate the ambiguity and the need for theHasMeasurementNoiseLevelflag.
333-393: Avoid null-forgiving operator for properties that must be initialized.Lines 356, 361, and 366 use the null-forgiving operator (
default!) forDataLoss,PhysicsLoss, andTotalLoss. This suppresses null-safety warnings but leaves these properties potentially uninitialized, which could causeNullReferenceExceptionor unexpecteddefault(T)values if the result object is used before these properties are set.Either:
- Make these properties nullable (
T?) to accurately represent the uninitialized state, or- Require initialization via constructor parameters, or
- Initialize to a sentinel value (e.g.,
T.NaNfor floating-point types) with documentation🔎 Proposed fix option 1: Use nullable types
- public T DataLoss { get; set; } = default!; + public T? DataLoss { get; set; } - public T PhysicsLoss { get; set; } = default!; + public T? PhysicsLoss { get; set; } - public T TotalLoss { get; set; } = default!; + public T? TotalLoss { get; set; }src/AiDotNet.Tensors/Engines/IAdvancedAlgebraEngine.cs (1)
43-52: Specify array length requirements for batch operations.Batch operation methods (e.g.,
OctonionMultiplyBatch,OctonionAddBatch) accept two arrays but don't specify whether they must have the same length or what should happen if lengths differ.Add to the method documentation:
/// <param name="left">Array of left operand octonions.</param> /// <param name="right">Array of right operand octonions (must have same length as left).</param> /// <exception cref="ArgumentException">Thrown when array lengths differ.</exception>Apply similar documentation to all batch operation methods throughout the interface.
src/AiDotNet.Tensors/Engines/CpuHyperbolicManifoldEngine.cs (1)
42-101: Consider documenting numerical stability measures.The implementation includes several numerical stability safeguards (e.g., clamping at lines 76, 143, 208), but these aren't prominently documented. Consider adding a remark about numerical stability to help users understand the robustness of the implementation.
Add to the class-level documentation:
/// <para><b>Numerical Stability:</b> All operations include safeguards against: /// - Division by zero (epsilon-based clamping) /// - Overflow in hyperbolic functions (input clamping) /// - Points escaping the valid domain (projection after operations) /// </para>src/NeuralNetworks/Layers/HyperbolicLinearLayer.cs (1)
272-352: Document that backward pass uses approximated Riemannian gradients.Lines 304-305 note this is a "simplified version" using "Euclidean gradient scaled by conformal factor" that is "a first-order approximation valid for small curvature." This is an important limitation that should be more prominently documented, as it may produce incorrect gradients for hyperbolic spaces with large curvature.
Add a remark to the method documentation:
/// <remarks> /// <b>Note:</b> This implementation uses a first-order Euclidean approximation /// to the Riemannian gradient, which is accurate for small curvature values. /// For highly curved spaces, consider implementing the full Riemannian gradient. /// </remarks>Alternatively, if full Riemannian gradients are planned for the future, add a TODO comment and consider making this a known limitation in the class-level documentation.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (141)
README.mdSPRINT_PLAN_PR449_ADVANCED_ALGEBRA.mdSPRINT_PLAN_PR449_ISSUE400.mddocs/PHYSICS_AI_IMPLEMENTATION_PLAN.mddocs/PhysicsInformedBenchmarks.mdscripts/run-tests-sharded.ps1src/AiDotNet.Tensors/Engines/CpuAdvancedAlgebraEngine.cssrc/AiDotNet.Tensors/Engines/CpuHyperbolicManifoldEngine.cssrc/AiDotNet.Tensors/Engines/CpuSparseEngine.cssrc/AiDotNet.Tensors/Engines/IAdvancedAlgebraEngine.cssrc/AiDotNet.Tensors/Engines/IHyperbolicManifoldEngine.cssrc/AiDotNet.Tensors/Engines/ISparseEngine.cssrc/AiDotNet.Tensors/Groups/ILieAlgebra.cssrc/AiDotNet.Tensors/Groups/ILieGroup.cssrc/AiDotNet.Tensors/Groups/Se3.cssrc/AiDotNet.Tensors/Groups/Se3Group.cssrc/AiDotNet.Tensors/Groups/So3.cssrc/AiDotNet.Tensors/Groups/So3Group.cssrc/AiDotNet.Tensors/Groups/Su2.cssrc/AiDotNet.Tensors/Groups/Su2Group.cssrc/AiDotNet.Tensors/Helpers/MathHelper.cssrc/AiDotNet.Tensors/Helpers/VectorizedOperationsFallback.cssrc/AiDotNet.Tensors/LinearAlgebra/CliffordAlgebra.cssrc/AiDotNet.Tensors/LinearAlgebra/Multivector.cssrc/AiDotNet.Tensors/LinearAlgebra/Octonion.cssrc/AiDotNet.Tensors/LinearAlgebra/SparseStorageFormat.cssrc/AiDotNet.Tensors/LinearAlgebra/SparseTensor.cssrc/AiDotNet.Tensors/Manifolds/HyperboloidManifold.cssrc/AiDotNet.Tensors/Manifolds/PoincareBallManifold.cssrc/AiDotNet.Tensors/NumericOperations/MultivectorOperations.cssrc/AiDotNet.Tensors/NumericOperations/OctonionOperations.cssrc/AiDotNet.Tensors/Topology/Simplex.cssrc/AiDotNet.Tensors/Topology/SimplicialComplex.cssrc/Autodiff/NeuralNetworkDerivatives.cssrc/Data/Loaders/UniformEpisodicDataLoader.cssrc/Enums/OperationType.cssrc/Helpers/LayerHelper.cssrc/JitCompiler/CompilationStats.cssrc/JitCompiler/IR/IROp.cssrc/JitCompiler/IR/Operations/GeometricProductOp.cssrc/JitCompiler/IR/Operations/GradGeometricProductOp.cssrc/JitCompiler/IR/Operations/GradMobiusAddOp.cssrc/JitCompiler/IR/Operations/GradOctonionMultiplyOp.cssrc/JitCompiler/IR/Operations/GradPoincareExpMapOp.cssrc/JitCompiler/IR/Operations/GradSpMMOp.cssrc/JitCompiler/IR/Operations/GradSpMVOp.cssrc/JitCompiler/IR/Operations/MobiusAddOp.cssrc/JitCompiler/IR/Operations/OctonionMatMulOp.cssrc/JitCompiler/IR/Operations/OctonionMultiplyOp.cssrc/JitCompiler/IR/Operations/PoincareExpMapOp.cssrc/JitCompiler/IR/Operations/PoincareLogMapOp.cssrc/JitCompiler/IR/Operations/SpMMOp.cssrc/JitCompiler/IR/Operations/SpMVOp.cssrc/JitCompiler/IR/Operations/WedgeProductOp.cssrc/NeuralNetworks/HyperbolicNeuralNetwork.cssrc/NeuralNetworks/Layers/DenseLayer.cssrc/NeuralNetworks/Layers/FullyConnectedLayer.cssrc/NeuralNetworks/Layers/GraphConvolutionalLayer.cssrc/NeuralNetworks/Layers/HyperbolicLinearLayer.cssrc/NeuralNetworks/Layers/OctonionLinearLayer.cssrc/NeuralNetworks/Layers/SparseLinearLayer.cssrc/NeuralNetworks/NeuralNetworkBase.cssrc/NeuralNetworks/OctonionNeuralNetwork.cssrc/NeuralNetworks/SparseNeuralNetwork.cssrc/PhysicsInformed/Benchmarks/DarcyOperatorBenchmarkOptions.cssrc/PhysicsInformed/Benchmarks/OperatorBenchmarkOptions.cssrc/PhysicsInformed/Benchmarks/OperatorBenchmarkResult.cssrc/PhysicsInformed/Benchmarks/OperatorBenchmarkSuite.cssrc/PhysicsInformed/Benchmarks/OperatorDataset2D.cssrc/PhysicsInformed/Benchmarks/PdeBenchmarkOptions.cssrc/PhysicsInformed/Benchmarks/PdeBenchmarkResult.cssrc/PhysicsInformed/Benchmarks/PdeBenchmarkSuite.cssrc/PhysicsInformed/Benchmarks/PoissonOperatorBenchmarkOptions.cssrc/PhysicsInformed/DomainDecompositionTrainingHistory.cssrc/PhysicsInformed/FiniteDifferenceGradient.cssrc/PhysicsInformed/GpuPINNTrainer.cssrc/PhysicsInformed/GpuPINNTrainingOptions.cssrc/PhysicsInformed/Interfaces/IDomainDecompositionTrainingHistory.cssrc/PhysicsInformed/Interfaces/IGpuAcceleratedPINN.cssrc/PhysicsInformed/Interfaces/IInverseProblem.cssrc/PhysicsInformed/Interfaces/IMultiFidelityTrainingHistory.cssrc/PhysicsInformed/Interfaces/IMultiScalePDE.cssrc/PhysicsInformed/Interfaces/IPDESpecification.cssrc/PhysicsInformed/MultiFidelityTrainingHistory.cssrc/PhysicsInformed/NeuralOperators/DeepOperatorNetwork.cssrc/PhysicsInformed/NeuralOperators/FourierNeuralOperator.cssrc/PhysicsInformed/NeuralOperators/GraphNeuralOperator.cssrc/PhysicsInformed/PDEs/AdvectionDiffusionEquation.cssrc/PhysicsInformed/PDEs/AllenCahnEquation.cssrc/PhysicsInformed/PDEs/BlackScholesEquation.cssrc/PhysicsInformed/PDEs/BurgersEquation.cssrc/PhysicsInformed/PDEs/HeatEquation.cssrc/PhysicsInformed/PDEs/KortewegDeVriesEquation.cssrc/PhysicsInformed/PDEs/LinearElasticityEquation.cssrc/PhysicsInformed/PDEs/MaxwellEquations.cssrc/PhysicsInformed/PDEs/NavierStokesEquation.cssrc/PhysicsInformed/PDEs/PDESpecificationBase.cssrc/PhysicsInformed/PDEs/PoissonEquation.cssrc/PhysicsInformed/PDEs/SchrodingerEquation.cssrc/PhysicsInformed/PDEs/WaveEquation.cssrc/PhysicsInformed/PINNs/DeepRitzMethod.cssrc/PhysicsInformed/PINNs/DomainDecompositionPINN.cssrc/PhysicsInformed/PINNs/InverseProblemPINN.cssrc/PhysicsInformed/PINNs/MultiFidelityPINN.cssrc/PhysicsInformed/PINNs/MultiScalePINN.cssrc/PhysicsInformed/PINNs/PhysicsInformedNeuralNetwork.cssrc/PhysicsInformed/PINNs/VariationalPINN.cssrc/PhysicsInformed/PhysicsInformedLoss.cssrc/PhysicsInformed/ScientificML/HamiltonianNeuralNetwork.cssrc/PhysicsInformed/ScientificML/LagrangianNeuralNetwork.cssrc/PhysicsInformed/ScientificML/SymbolicPhysicsLearner.cssrc/PhysicsInformed/ScientificML/UniversalDifferentialEquations.cssrc/PhysicsInformed/TrainingHistory.cssrc/Regression/SymbolicRegression.cstests/AiDotNet.Tests/UnitTests/Diagnostics/ProfilerTests.cstests/AiDotNet.Tests/UnitTests/Engines/CpuAdvancedAlgebraEngineTests.cstests/AiDotNet.Tests/UnitTests/Engines/CpuHyperbolicManifoldEngineTests.cstests/AiDotNet.Tests/UnitTests/Engines/CpuSparseEngineTests.cstests/AiDotNet.Tests/UnitTests/Groups/LieGroupTests.cstests/AiDotNet.Tests/UnitTests/JitCompiler/AdvancedAlgebraIROpTests.cstests/AiDotNet.Tests/UnitTests/JitCompiler/JitCompilerTests.cstests/AiDotNet.Tests/UnitTests/Layers/AdvancedAlgebraLayerTests.cstests/AiDotNet.Tests/UnitTests/LinearAlgebra/MultivectorTests.cstests/AiDotNet.Tests/UnitTests/LinearAlgebra/OctonionTests.cstests/AiDotNet.Tests/UnitTests/LinearAlgebra/SparseTensorTests.cstests/AiDotNet.Tests/UnitTests/Logging/TensorBoardTests.cstests/AiDotNet.Tests/UnitTests/Manifolds/HyperbolicManifoldTests.cstests/AiDotNet.Tests/UnitTests/NeuralNetworks/AdvancedAlgebraNetworkTests.cstests/AiDotNet.Tests/UnitTests/PhysicsInformed/Benchmarks/PhysicsInformedBenchmarkTests.cstests/AiDotNet.Tests/UnitTests/PhysicsInformed/GpuAccelerationTests.cstests/AiDotNet.Tests/UnitTests/PhysicsInformed/MultiFidelityAndDomainDecompositionTests.cstests/AiDotNet.Tests/UnitTests/PhysicsInformed/MultiScaleAndInverseProblemTests.cstests/AiDotNet.Tests/UnitTests/PhysicsInformed/NeuralNetworkDerivativesTests.cstests/AiDotNet.Tests/UnitTests/PhysicsInformed/NeuralOperators/FourierLayerTests.cstests/AiDotNet.Tests/UnitTests/PhysicsInformed/NeuralOperators/FourierNeuralOperatorTests.cstests/AiDotNet.Tests/UnitTests/PhysicsInformed/NeuralOperators/NeuralOperatorTrainingTests.cstests/AiDotNet.Tests/UnitTests/PhysicsInformed/PDEs/AdvancedPDETests.cstests/AiDotNet.Tests/UnitTests/PhysicsInformed/PDEs/PDEAnalyticalSolutionTests.cstests/AiDotNet.Tests/UnitTests/PhysicsInformed/PINNs/PinnTrainingTests.cstests/AiDotNet.Tests/UnitTests/PhysicsInformed/ScientificML/ScientificMLTests.cstests/AiDotNet.Tests/UnitTests/Topology/SimplicialComplexTests.cs
🚧 Files skipped from review as they are similar to previous changes (26)
- src/JitCompiler/IR/Operations/OctonionMatMulOp.cs
- src/PhysicsInformed/FiniteDifferenceGradient.cs
- src/JitCompiler/IR/Operations/GradOctonionMultiplyOp.cs
- src/JitCompiler/IR/Operations/MobiusAddOp.cs
- src/AiDotNet.Tensors/Groups/ILieAlgebra.cs
- src/NeuralNetworks/HyperbolicNeuralNetwork.cs
- src/AiDotNet.Tensors/Groups/Su2.cs
- src/AiDotNet.Tensors/Engines/ISparseEngine.cs
- src/NeuralNetworks/Layers/GraphConvolutionalLayer.cs
- src/JitCompiler/IR/Operations/SpMVOp.cs
- src/JitCompiler/IR/Operations/SpMMOp.cs
- docs/PhysicsInformedBenchmarks.md
- src/PhysicsInformed/PDEs/HeatEquation.cs
- src/JitCompiler/IR/Operations/GradSpMVOp.cs
- src/NeuralNetworks/NeuralNetworkBase.cs
- src/PhysicsInformed/Interfaces/IPDESpecification.cs
- src/JitCompiler/IR/Operations/PoincareExpMapOp.cs
- src/PhysicsInformed/PDEs/BurgersEquation.cs
- README.md
- src/PhysicsInformed/Interfaces/IDomainDecompositionTrainingHistory.cs
- src/JitCompiler/IR/Operations/WedgeProductOp.cs
- src/PhysicsInformed/GpuPINNTrainingOptions.cs
- src/AiDotNet.Tensors/Topology/SimplicialComplex.cs
- src/JitCompiler/IR/Operations/OctonionMultiplyOp.cs
- src/PhysicsInformed/Interfaces/IMultiFidelityTrainingHistory.cs
- src/NeuralNetworks/SparseNeuralNetwork.cs
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
src/AiDotNet.Tensors/Groups/So3.cssrc/AiDotNet.Tensors/Groups/Se3.cssrc/PhysicsInformed/Interfaces/IGpuAcceleratedPINN.cssrc/AiDotNet.Tensors/Groups/Su2Group.cssrc/JitCompiler/CompilationStats.cssrc/AiDotNet.Tensors/Groups/So3Group.cssrc/PhysicsInformed/Benchmarks/PdeBenchmarkSuite.cssrc/PhysicsInformed/Benchmarks/PdeBenchmarkResult.cssrc/JitCompiler/IR/Operations/PoincareLogMapOp.cssrc/PhysicsInformed/Interfaces/IInverseProblem.cssrc/AiDotNet.Tensors/NumericOperations/MultivectorOperations.cssrc/PhysicsInformed/PDEs/LinearElasticityEquation.cssrc/AiDotNet.Tensors/Helpers/VectorizedOperationsFallback.cssrc/NeuralNetworks/Layers/DenseLayer.cssrc/AiDotNet.Tensors/Topology/Simplex.cssrc/Enums/OperationType.cssrc/PhysicsInformed/Benchmarks/DarcyOperatorBenchmarkOptions.cssrc/PhysicsInformed/Benchmarks/PdeBenchmarkOptions.cssrc/AiDotNet.Tensors/Groups/ILieGroup.cssrc/PhysicsInformed/PDEs/NavierStokesEquation.cssrc/AiDotNet.Tensors/Groups/Se3Group.cssrc/AiDotNet.Tensors/Engines/CpuHyperbolicManifoldEngine.cssrc/PhysicsInformed/PDEs/MaxwellEquations.cssrc/AiDotNet.Tensors/LinearAlgebra/SparseTensor.cssrc/PhysicsInformed/Benchmarks/OperatorDataset2D.cssrc/JitCompiler/IR/IROp.cssrc/PhysicsInformed/PDEs/KortewegDeVriesEquation.cssrc/Data/Loaders/UniformEpisodicDataLoader.cssrc/PhysicsInformed/GpuPINNTrainer.cssrc/AiDotNet.Tensors/Engines/CpuSparseEngine.cssrc/AiDotNet.Tensors/Engines/CpuAdvancedAlgebraEngine.cssrc/PhysicsInformed/MultiFidelityTrainingHistory.cssrc/JitCompiler/IR/Operations/GradSpMMOp.cssrc/PhysicsInformed/Benchmarks/OperatorBenchmarkOptions.cssrc/JitCompiler/IR/Operations/GradGeometricProductOp.cssrc/AiDotNet.Tensors/Manifolds/HyperboloidManifold.cssrc/AiDotNet.Tensors/Helpers/MathHelper.cssrc/PhysicsInformed/PDEs/BlackScholesEquation.cssrc/JitCompiler/IR/Operations/GradPoincareExpMapOp.cssrc/PhysicsInformed/DomainDecompositionTrainingHistory.cssrc/PhysicsInformed/Benchmarks/OperatorBenchmarkSuite.cssrc/AiDotNet.Tensors/LinearAlgebra/CliffordAlgebra.cssrc/AiDotNet.Tensors/LinearAlgebra/Octonion.cssrc/NeuralNetworks/Layers/FullyConnectedLayer.cssrc/NeuralNetworks/Layers/OctonionLinearLayer.cssrc/NeuralNetworks/OctonionNeuralNetwork.cssrc/PhysicsInformed/Interfaces/IMultiScalePDE.cssrc/PhysicsInformed/NeuralOperators/FourierNeuralOperator.cssrc/PhysicsInformed/Benchmarks/PoissonOperatorBenchmarkOptions.cssrc/JitCompiler/IR/Operations/GradMobiusAddOp.cssrc/AiDotNet.Tensors/Engines/IAdvancedAlgebraEngine.cssrc/JitCompiler/IR/Operations/GeometricProductOp.cssrc/AiDotNet.Tensors/LinearAlgebra/SparseStorageFormat.cssrc/PhysicsInformed/PDEs/AllenCahnEquation.cssrc/PhysicsInformed/Benchmarks/OperatorBenchmarkResult.cssrc/AiDotNet.Tensors/LinearAlgebra/Multivector.cssrc/PhysicsInformed/NeuralOperators/DeepOperatorNetwork.cssrc/NeuralNetworks/Layers/HyperbolicLinearLayer.cssrc/AiDotNet.Tensors/Manifolds/PoincareBallManifold.cssrc/PhysicsInformed/NeuralOperators/GraphNeuralOperator.cssrc/Helpers/LayerHelper.cssrc/AiDotNet.Tensors/Engines/IHyperbolicManifoldEngine.cssrc/AiDotNet.Tensors/NumericOperations/OctonionOperations.cssrc/PhysicsInformed/PDEs/AdvectionDiffusionEquation.cssrc/NeuralNetworks/Layers/SparseLinearLayer.cssrc/Autodiff/NeuralNetworkDerivatives.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
src/AiDotNet.Tensors/Groups/So3.cssrc/AiDotNet.Tensors/Groups/Se3.cssrc/PhysicsInformed/Interfaces/IGpuAcceleratedPINN.cssrc/AiDotNet.Tensors/Groups/Su2Group.cssrc/JitCompiler/CompilationStats.cssrc/AiDotNet.Tensors/Groups/So3Group.cssrc/PhysicsInformed/Benchmarks/PdeBenchmarkSuite.cssrc/PhysicsInformed/Benchmarks/PdeBenchmarkResult.cssrc/JitCompiler/IR/Operations/PoincareLogMapOp.cssrc/PhysicsInformed/Interfaces/IInverseProblem.cssrc/AiDotNet.Tensors/NumericOperations/MultivectorOperations.cssrc/PhysicsInformed/PDEs/LinearElasticityEquation.cssrc/AiDotNet.Tensors/Helpers/VectorizedOperationsFallback.cssrc/NeuralNetworks/Layers/DenseLayer.cssrc/AiDotNet.Tensors/Topology/Simplex.cssrc/Enums/OperationType.cssrc/PhysicsInformed/Benchmarks/DarcyOperatorBenchmarkOptions.cssrc/PhysicsInformed/Benchmarks/PdeBenchmarkOptions.cssrc/AiDotNet.Tensors/Groups/ILieGroup.cssrc/PhysicsInformed/PDEs/NavierStokesEquation.cssrc/AiDotNet.Tensors/Groups/Se3Group.cssrc/AiDotNet.Tensors/Engines/CpuHyperbolicManifoldEngine.cssrc/PhysicsInformed/PDEs/MaxwellEquations.cssrc/AiDotNet.Tensors/LinearAlgebra/SparseTensor.cssrc/PhysicsInformed/Benchmarks/OperatorDataset2D.cssrc/JitCompiler/IR/IROp.cssrc/PhysicsInformed/PDEs/KortewegDeVriesEquation.cssrc/Data/Loaders/UniformEpisodicDataLoader.cssrc/PhysicsInformed/GpuPINNTrainer.cssrc/AiDotNet.Tensors/Engines/CpuSparseEngine.cssrc/AiDotNet.Tensors/Engines/CpuAdvancedAlgebraEngine.cssrc/PhysicsInformed/MultiFidelityTrainingHistory.cssrc/JitCompiler/IR/Operations/GradSpMMOp.cssrc/PhysicsInformed/Benchmarks/OperatorBenchmarkOptions.cssrc/JitCompiler/IR/Operations/GradGeometricProductOp.cssrc/AiDotNet.Tensors/Manifolds/HyperboloidManifold.cssrc/AiDotNet.Tensors/Helpers/MathHelper.cssrc/PhysicsInformed/PDEs/BlackScholesEquation.cssrc/JitCompiler/IR/Operations/GradPoincareExpMapOp.cssrc/PhysicsInformed/DomainDecompositionTrainingHistory.cssrc/PhysicsInformed/Benchmarks/OperatorBenchmarkSuite.cssrc/AiDotNet.Tensors/LinearAlgebra/CliffordAlgebra.cssrc/AiDotNet.Tensors/LinearAlgebra/Octonion.cssrc/NeuralNetworks/Layers/FullyConnectedLayer.cssrc/NeuralNetworks/Layers/OctonionLinearLayer.cssrc/NeuralNetworks/OctonionNeuralNetwork.cssrc/PhysicsInformed/Interfaces/IMultiScalePDE.cssrc/PhysicsInformed/NeuralOperators/FourierNeuralOperator.cssrc/PhysicsInformed/Benchmarks/PoissonOperatorBenchmarkOptions.cssrc/JitCompiler/IR/Operations/GradMobiusAddOp.cssrc/AiDotNet.Tensors/Engines/IAdvancedAlgebraEngine.cssrc/JitCompiler/IR/Operations/GeometricProductOp.cssrc/AiDotNet.Tensors/LinearAlgebra/SparseStorageFormat.cssrc/PhysicsInformed/PDEs/AllenCahnEquation.cssrc/PhysicsInformed/Benchmarks/OperatorBenchmarkResult.cssrc/AiDotNet.Tensors/LinearAlgebra/Multivector.cssrc/PhysicsInformed/NeuralOperators/DeepOperatorNetwork.cssrc/NeuralNetworks/Layers/HyperbolicLinearLayer.cssrc/AiDotNet.Tensors/Manifolds/PoincareBallManifold.cssrc/PhysicsInformed/NeuralOperators/GraphNeuralOperator.cssrc/Helpers/LayerHelper.cssrc/AiDotNet.Tensors/Engines/IHyperbolicManifoldEngine.cssrc/AiDotNet.Tensors/NumericOperations/OctonionOperations.cssrc/PhysicsInformed/PDEs/AdvectionDiffusionEquation.cssrc/NeuralNetworks/Layers/SparseLinearLayer.cssrc/Autodiff/NeuralNetworkDerivatives.cs
📚 Learning: 2025-11-19T04:08:26.895Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 0
File: :0-0
Timestamp: 2025-11-19T04:08:26.895Z
Learning: For ILGPU GPU operations in GpuEngine.cs, use standard .NET exception types (InvalidOperationException, ArgumentException, OutOfMemoryException) instead of ILGPU-specific exception types, as ILGPU exception types may be version-specific. Combine with message-based filtering using ex.Message.Contains("device") or ex.Message.Contains("accelerator") as a fallback for GPU-specific errors.
Applied to files:
src/PhysicsInformed/GpuPINNTrainer.cs
🪛 LanguageTool
docs/PHYSICS_AI_IMPLEMENTATION_PLAN.md
[grammar] ~180-~180: Ensure spelling is correct
Context: ...rangian physics networks (NEW) ### 1.3 IEngine: Already Inherited, Never Injected **Wh...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~223-~223: ‘with respect to’ might be wordy. Consider a shorter alternative.
Context: ...l**: dLoss/dWeights (derivative of loss with respect to weights) ```csharp // This is TRAINING...
(EN_WORDINESS_PREMIUM_WITH_RESPECT_TO)
[style] ~244-~244: ‘with respect to’ might be wordy. Consider a shorter alternative.
Context: ...bol**: dH/dq (derivative of Hamiltonian with respect to position) ```csharp // This is PHYSICS...
(EN_WORDINESS_PREMIUM_WITH_RESPECT_TO)
[style] ~265-~265: ‘with respect to’ might be wordy. Consider a shorter alternative.
Context: ... - Training gradients are computed with respect to the network's weights (to make the ...
(EN_WORDINESS_PREMIUM_WITH_RESPECT_TO)
[style] ~266-~266: ‘with respect to’ might be wordy. Consider a shorter alternative.
Context: ...n) - Physics gradients are computed with respect to the inputs (to simulate the physica...
(EN_WORDINESS_PREMIUM_WITH_RESPECT_TO)
[style] ~268-~268: Consider using a different adverb to strengthen your wording.
Context: ...imulate the physical system) These are completely different operations with different pur...
(COMPLETELY_ENTIRELY)
[uncategorized] ~299-~299: Did you refer to the Austrian physicist Erwin “Schrödinger” (1887–1961)?
Context: ...| 2D TE mode electromagnetics | Yes | | Schrodinger Equation | 1D time-dependent quantum me...
(SCHROEDINGER)
[uncategorized] ~339-~339: Did you refer to the Austrian physicist Erwin “Schrödinger” (1887–1961)?
Context: ...ent Maxwell's equations - [x] Implement Schrodinger equation - [x] Add PDE unit tests with ...
(SCHROEDINGER)
SPRINT_PLAN_PR449_ADVANCED_ALGEBRA.md
[grammar] ~19-~19: Ensure spelling is correct
Context: ...Every new operation must have IEngine + GpuEngine implementations (no CPU-only paths). - ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
SPRINT_PLAN_PR449_ISSUE400.md
4-4: Bare URL used
(MD034, no-bare-urls)
5-5: Bare URL used
(MD034, no-bare-urls)
- Fix critical HyperbolicLinearLayer bias update loop structure - Add atanh domain protection in PoincareBallManifold - Prevent vertices mutation in Simplex by returning defensive copy - Add CFL stability checks for Burgers and Allen-Cahn equations - Add tensor dimension validation in GpuPINNTrainer - Fix So3Group Log numerical instability near θ ≈ π - Fix IHyperbolicManifoldEngine epsilon documentation - Fix IAdvancedAlgebraEngine interface documentation - Fix Schrödinger spelling in implementation plan - Use permeability in MaxwellEquations computations 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
src/AiDotNet.Tensors/Engines/IAdvancedAlgebraEngine.cs (1)
27-43: Non-associativity warning is now correctly phrased for an interface.The documentation properly states "Implementations should use left-to-right evaluation order for consistency" rather than claiming "this implementation uses."
src/AiDotNet.Tensors/Engines/IHyperbolicManifoldEngine.cs (1)
121-135: Epsilon documentation is now accurate.The parameter description correctly states "recommended: 1e-5" rather than implying a default value that doesn't exist in the signature.
src/AiDotNet.Tensors/Manifolds/PoincareBallManifold.cs (1)
95-108: The atanh domain clamping has been addressed.The clamping at lines 101-104 properly handles boundary points where floating-point errors could push the argument outside the valid domain. The distance formula is correctly implemented.
src/PhysicsInformed/Benchmarks/PdeBenchmarkSuite.cs (1)
83-133: Advection CFL condition still missing.The diffusion CFL check (lines 93-102) is good, but Burgers' equation also has an advection term
u*u_xthat requiresdt * max(|u|) / dx ≤ 1for stability. Without this check, large solution values can trigger advection-driven instability even when diffusion is stable.🔎 Suggested conservative pre-check
Add after line 102:
+ // Conservative advection CFL estimate using initial condition + double maxU = 0.0; + for (int i = 0; i < n; i++) + { + double ic = options.InitialCondition(grid[i]); + if (Math.Abs(ic) > maxU) maxU = Math.Abs(ic); + } + + double advectionCfl = maxU * dt * invDx; + if (advectionCfl > 1.0) + { + throw new InvalidOperationException( + $"CFL condition violated for Burgers equation. " + + $"Advection stability requires max(|u|) * dt / dx ≤ 1.0, " + + $"but initial condition gives {advectionCfl:F4}. " + + $"Increase TimeSteps or decrease domain resolution."); + }Note: This checks against the initial condition as a conservative estimate. The true maximum of |u| may grow during time evolution, but this catches obviously unstable parameter sets before the solve begins.
🧹 Nitpick comments (14)
src/AiDotNet.Tensors/Groups/So3Group.cs (1)
103-122: Sign ambiguity in near-π axis extraction.The axis direction for 180° rotations has a sign ambiguity (both
+nand-nrepresent the same rotation). The current logic doesn't establish a consistent reference sign forrxbefore using off-diagonals to determineryandrzsigns.Standard approach: choose the largest diagonal element, set that axis component positive, then derive other signs.
🔎 Proposed fix for robust sign determination
// Handle θ near π where sin(θ) approaches 0 if (theta > Math.PI - 1e-6) { // For θ ≈ π, extract axis from diagonal of (R + I)/2 // The axis is the eigenvector with eigenvalue 1 double rx = Math.Sqrt(Math.Max(0.0, (r00 + 1.0) / 2.0)); double ry = Math.Sqrt(Math.Max(0.0, (r11 + 1.0) / 2.0)); double rz = Math.Sqrt(Math.Max(0.0, (r22 + 1.0) / 2.0)); - // Determine signs from off-diagonal elements - if (Convert.ToDouble(value.Matrix[0, 1]) + Convert.ToDouble(value.Matrix[1, 0]) < 0) ry = -ry; - if (Convert.ToDouble(value.Matrix[0, 2]) + Convert.ToDouble(value.Matrix[2, 0]) < 0) rz = -rz; + // Determine signs: pick largest component as positive reference + double r01 = Convert.ToDouble(value.Matrix[0, 1]); + double r02 = Convert.ToDouble(value.Matrix[0, 2]); + double r12 = Convert.ToDouble(value.Matrix[1, 2]); + + if (rx >= ry && rx >= rz) + { + // rx is reference (positive) + if (r01 + Convert.ToDouble(value.Matrix[1, 0]) < 0) ry = -ry; + if (r02 + Convert.ToDouble(value.Matrix[2, 0]) < 0) rz = -rz; + } + else if (ry >= rx && ry >= rz) + { + // ry is reference (positive) + if (r01 + Convert.ToDouble(value.Matrix[1, 0]) < 0) rx = -rx; + if (r12 + Convert.ToDouble(value.Matrix[2, 1]) < 0) rz = -rz; + } + else + { + // rz is reference (positive) + if (r02 + Convert.ToDouble(value.Matrix[2, 0]) < 0) rx = -rx; + if (r12 + Convert.ToDouble(value.Matrix[2, 1]) < 0) ry = -ry; + } return new Vector<T>(new[]src/AiDotNet.Tensors/Engines/IAdvancedAlgebraEngine.cs (1)
43-89: Consider documenting array length mismatch behavior.Batch operations like
OctonionMultiplyBatchtake two arrays but don't specify the expected behavior whenleft.Length != right.Length. Consider adding a remark about whether implementations should throwArgumentExceptionor handle mismatches differently.src/AiDotNet.Tensors/Engines/IHyperbolicManifoldEngine.cs (2)
30-48: Clarify the curvature parameter convention.The documentation states curvature is "typically -1" but the formula uses
c = -curvature. This naming can cause confusion. Consider clarifying whether the parameter represents the actual curvature (negative for hyperbolic space) or a positive curvature magnitude.🔎 Suggested documentation clarification
- /// <param name="curvature">The curvature parameter (typically -1).</param> + /// <param name="curvature">The curvature parameter K (typically -1 for unit hyperbolic space). + /// Formulas use c = |K| = -curvature internally.</param>
222-244: Batch operations only cover Poincare model.The batch operations section includes
PoincareExpMapBatchandPoincareDistanceBatchbut no hyperboloid equivalents. If this asymmetry is intentional (e.g., Poincare is more commonly used in hyperbolic NNs), consider adding a remark. Otherwise, consider addingHyperboloidExpMapBatchandHyperboloidDistanceBatchfor API consistency.src/AiDotNet.Tensors/Topology/Simplex.cs (3)
17-17: ConsiderIReadOnlyList<int>to avoid repeated allocations.While
Clone()correctly prevents mutation of the internal_vertices, it allocates a new array on every property access. ReturningIReadOnlyList<int>would eliminate allocations while expressing immutability in the type system.As noted in the previous review, arrays already implement
IReadOnlyList<T>, so the constructor assignment remains compatible.🔎 Alternative implementation
- public int[] Vertices => (int[])_vertices.Clone(); + public IReadOnlyList<int> Vertices => _vertices;
46-59: Consider optimizing face creation to skip redundant validation.Each face simplex is created by removing one vertex from an already-sorted, unique vertex list. However, the
Simplexconstructor re-sorts and re-validates these vertices. For an n-simplex, this adds O(n log n) overhead per face.A package-internal constructor overload that accepts pre-validated vertices could eliminate this redundancy for performance-critical applications.
🔎 Example optimization approach
Add an internal constructor:
// Internal constructor assuming pre-validated, sorted vertices internal Simplex(int[] vertices, bool skipValidation) { if (!skipValidation) throw new ArgumentException("This constructor requires skipValidation=true"); _vertices = vertices; }Then in
Boundary():int sign = (i % 2 == 0) ? 1 : -1; - faces.Add((sign, new Simplex(faceVertices))); + faces.Add((sign, new Simplex(faceVertices, skipValidation: true)));
94-94: Avoid unnecessary allocation inToString().The
Verticesproperty creates a defensive copy viaClone(), butToString()can safely access_verticesdirectly since it's within the class.🔎 Proposed change
- return $"Simplex(dim={Dimension}, vertices=[{string.Join(",", Vertices)}])"; + return $"Simplex(dim={Dimension}, vertices=[{string.Join(",", _vertices)}])";src/PhysicsInformed/Benchmarks/PdeBenchmarkSuite.cs (1)
135-183: Consider reaction term stability for robustness.The diffusion CFL is checked, but the reaction term
u³ - uhas derivative3u² - 1that can be large when |u| > 1, potentially requiring smaller timesteps for explicit Euler stability. For most benchmark cases with |u| ≈ 1 this is manageable, but for robustness you could optionally warn if the initial condition has |u| significantly exceeding 1.src/PhysicsInformed/GpuPINNTrainer.cs (2)
193-197: "KernelTimings" is a misleading name for epoch-level metrics.The dictionary is populated with epoch-level statistics ("TotalEpochs", "AverageEpochMs"), but "KernelTimings" suggests GPU kernel execution profiling. Consider renaming to
TrainingMetrics,EpochMetrics, orPerformanceMetricsfor clarity.🔎 Suggested renaming
In
GpuTrainingHistory<T>class (line 417):- public Dictionary<string, long> KernelTimings { get; set; } = new Dictionary<string, long>(); + public Dictionary<string, long> TrainingMetrics { get; set; } = new Dictionary<string, long>();And update usage in Train method:
- history.KernelTimings["TotalEpochs"] = epochs; - history.KernelTimings["AverageEpochMs"] = stopwatch.ElapsedMilliseconds / epochs; + history.TrainingMetrics["TotalEpochs"] = epochs; + history.TrainingMetrics["AverageEpochMs"] = stopwatch.ElapsedMilliseconds / epochs;
266-280: Consider using tensor operations for MSE computation.The current nested-loop implementation manually computes MSE element by element. For better performance and idiomatic tensor code, consider leveraging tensor operations (subtraction, element-wise multiply, sum, mean).
🔎 Example using tensor operations
- // Simple MSE loss for targets (physics loss computed separately) - T sum = _numOps.Zero; - int count = 0; - - for (int i = 0; i < outputs.Shape[0]; i++) - { - for (int j = 0; j < outputs.Shape[1]; j++) - { - T diff = _numOps.Subtract(outputs[i, j], batchTargets[i, j]); - sum = _numOps.Add(sum, _numOps.Multiply(diff, diff)); - count++; - } - } - - loss = count > 0 ? _numOps.Divide(sum, _numOps.FromDouble(count)) : _numOps.Zero; + // Compute MSE using tensor operations (if supported by the tensor API) + // Example: loss = ((outputs - batchTargets)^2).Mean() + // Note: Adjust based on available Tensor<T> operations in your API + var diff = outputs.Subtract(batchTargets); + var squared = diff.MultiplyElementwise(diff); + loss = squared.Mean();Note: Actual implementation depends on the available tensor operations in your
Tensor<T>API. If such operations aren't available, the current implementation is acceptable.src/PhysicsInformed/PDEs/MaxwellEquations.cs (1)
115-142: Consider validating derivative matrix dimensions.The code indexes
firstDerivs[output_idx, input_idx](lines 132-142) without verifying that the matrix has the expected[3, 3]dimensions. WhileValidateFirstDerivativesmay perform some checks, explicit dimension validation would make the code more robust and provide clearer error messages if the derivative structure is malformed.🔎 Proposed validation
After line 119, add:
+if (firstDerivs.GetLength(0) != 3 || firstDerivs.GetLength(1) != 3) +{ + throw new ArgumentException($"Expected first derivatives with shape [3, 3], got [{firstDerivs.GetLength(0)}, {firstDerivs.GetLength(1)}]."); +}src/NeuralNetworks/Layers/HyperbolicLinearLayer.cs (3)
139-154: Consider making the random seed configurable.The initialization uses a fixed seed (42) which ensures reproducibility but may not be ideal for all use cases. Consider adding an optional seed parameter to the constructor.
🔎 Proposed enhancement
public HyperbolicLinearLayer( int inputFeatures, int outputFeatures, double curvature = -1.0, - IActivationFunction<T>? activationFunction = null) + IActivationFunction<T>? activationFunction = null, + int? randomSeed = null) : base( [inputFeatures], [outputFeatures], activationFunction ?? new IdentityActivation<T>()) { // ... existing validation ... _weights = new Matrix<T>(outputFeatures, inputFeatures); _biases = new Matrix<T>(outputFeatures, inputFeatures); - InitializeParameters(); + InitializeParameters(randomSeed); } -private void InitializeParameters() +private void InitializeParameters(int? seed = null) { var scale = Math.Sqrt(2.0 / (InputFeatures + OutputFeatures)); - var random = RandomHelper.CreateSeededRandom(42); + var random = seed.HasValue + ? RandomHelper.CreateSeededRandom(seed.Value) + : RandomHelper.CreateSeededRandom(42);
368-376: Inconsistent update strategies for weights and biases.Weights are updated using standard gradient descent in tangent space (lines 371-376), while biases use proper Riemannian gradient descent with exponential maps (lines 378-396). This asymmetry may cause imbalanced learning dynamics.
For consistency, consider applying exponential map updates to weights as well, or document why the simpler tangent-space update is sufficient for weights but not biases.
489-497: Consider simplifying origin vector creation.The manual loop to create a zero vector could potentially be simplified if
Vector<T>provides a constructor or method to fill with a default value.🔎 Potential simplification
If
Vector<T>supports it:private Vector<T> CreateOriginVector(int dimension) { - var origin = new Vector<T>(dimension); - for (int i = 0; i < dimension; i++) - { - origin[i] = _numOps.Zero; - } - return origin; + // Assuming Vector<T> initializes to zero by default, or has a Fill method + return new Vector<T>(dimension); // if zero-initialized + // or + // return Vector<T>.Fill(dimension, _numOps.Zero); }Verify whether
Vector<T>zero-initializes elements by default.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
docs/PHYSICS_AI_IMPLEMENTATION_PLAN.mdsrc/AiDotNet.Tensors/Engines/IAdvancedAlgebraEngine.cssrc/AiDotNet.Tensors/Engines/IHyperbolicManifoldEngine.cssrc/AiDotNet.Tensors/Groups/So3Group.cssrc/AiDotNet.Tensors/Manifolds/PoincareBallManifold.cssrc/AiDotNet.Tensors/Topology/Simplex.cssrc/NeuralNetworks/Layers/HyperbolicLinearLayer.cssrc/PhysicsInformed/Benchmarks/PdeBenchmarkSuite.cssrc/PhysicsInformed/GpuPINNTrainer.cssrc/PhysicsInformed/PDEs/MaxwellEquations.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/PHYSICS_AI_IMPLEMENTATION_PLAN.md
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
src/AiDotNet.Tensors/Groups/So3Group.cssrc/NeuralNetworks/Layers/HyperbolicLinearLayer.cssrc/AiDotNet.Tensors/Manifolds/PoincareBallManifold.cssrc/PhysicsInformed/Benchmarks/PdeBenchmarkSuite.cssrc/PhysicsInformed/PDEs/MaxwellEquations.cssrc/AiDotNet.Tensors/Engines/IAdvancedAlgebraEngine.cssrc/AiDotNet.Tensors/Topology/Simplex.cssrc/PhysicsInformed/GpuPINNTrainer.cssrc/AiDotNet.Tensors/Engines/IHyperbolicManifoldEngine.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
src/AiDotNet.Tensors/Groups/So3Group.cssrc/NeuralNetworks/Layers/HyperbolicLinearLayer.cssrc/AiDotNet.Tensors/Manifolds/PoincareBallManifold.cssrc/PhysicsInformed/Benchmarks/PdeBenchmarkSuite.cssrc/PhysicsInformed/PDEs/MaxwellEquations.cssrc/AiDotNet.Tensors/Engines/IAdvancedAlgebraEngine.cssrc/AiDotNet.Tensors/Topology/Simplex.cssrc/PhysicsInformed/GpuPINNTrainer.cssrc/AiDotNet.Tensors/Engines/IHyperbolicManifoldEngine.cs
📚 Learning: 2025-11-19T04:08:26.895Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 0
File: :0-0
Timestamp: 2025-11-19T04:08:26.895Z
Learning: For ILGPU GPU operations in GpuEngine.cs, use standard .NET exception types (InvalidOperationException, ArgumentException, OutOfMemoryException) instead of ILGPU-specific exception types, as ILGPU exception types may be version-specific. Combine with message-based filtering using ex.Message.Contains("device") or ex.Message.Contains("accelerator") as a fallback for GPU-specific errors.
Applied to files:
src/PhysicsInformed/GpuPINNTrainer.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL Analysis
- GitHub Check: Build (Windows)
🔇 Additional comments (40)
src/AiDotNet.Tensors/Groups/So3Group.cs (4)
1-18: LGTM!Class setup and constructor are clean. The use of
MathHelper.GetNumericOperations<T>()follows the repository's pattern for generic numeric operations.
20-26: LGTM!
Identity,Compose, andInverseimplementations are mathematically correct for SO(3):
- Identity returns the identity rotation
- Compose multiplies rotation matrices (correct order)
- Inverse transposes the orthogonal matrix
28-81: LGTM!The
Expmethod correctly implements the Rodrigues formula with proper small-angle handling using Taylor expansion (coefficients1 - θ²/6and0.5 - θ²/24are correct).
137-154: LGTM!
Adjointcorrectly returns the rotation matrix itself (Ad_R = R for SO(3)), and theMultiplyhelper is a straightforward 3×3 matrix multiplication.src/AiDotNet.Tensors/Engines/IAdvancedAlgebraEngine.cs (3)
6-22: LGTM!Excellent interface documentation with clear explanations of the three algebraic domains (Octonions, Multivectors, Lie Groups) and beginner-friendly context.
93-174: LGTM!The Clifford algebra batch operations are well-documented. The geometric/wedge/inner product explanations and the grade projection API are clear.
177-261: LGTM!Lie group batch operations for SO(3) and SE(3) are comprehensive, covering Exp, Log, Compose, and Adjoint. The beginner documentation explaining axis-angle representations and rigid body transformations is helpful.
src/AiDotNet.Tensors/Engines/IHyperbolicManifoldEngine.cs (2)
5-26: LGTM!Excellent interface-level documentation explaining hyperbolic space geometry, its applications for hierarchical data, and the two common models (Poincare Ball and Hyperboloid).
139-197: LGTM!Hyperboloid model operations are well-documented with correct formulas for the Lorentzian/Minkowski geometry. The projection constraint explanation is clear.
src/AiDotNet.Tensors/Manifolds/PoincareBallManifold.cs (8)
1-15: LGTM!Clean class structure with immutable fields. The sealed class with readonly state ensures thread-safety.
17-33: LGTM!Both constructors properly initialize the numeric operations and validate curvature. The validation-on-construction pattern prevents invalid manifold states.
35-52: LGTM!The Möbius addition formula is correctly implemented with proper denominator protection via
SafeDenominator.
54-71: LGTM!The exponential map correctly handles the zero-tangent edge case. The tanh function is naturally bounded, avoiding domain issues, and the division by
normis protected by the early return.
73-93: LGTM!Good implementation with proper atanh domain clamping (lines 85-89) and zero-norm edge case handling.
110-128: LGTM!The parallel transport correctly uses the gyration operator with conformal factor scaling. The gyration formula follows the standard gyrovector space definition.
130-176: LGTM!The conformal factor (lambda) and vector helpers are correctly implemented. The immutable approach with new allocations prioritizes clarity and thread-safety.
178-196: LGTM!The safety helpers provide appropriate guards:
SafeDenominatorprevents division-by-zero while preserving sign, andValidateCurvatureensures the manifold is properly configured.src/AiDotNet.Tensors/Topology/Simplex.cs (2)
21-38: LGTM!The constructor validation is thorough and efficient: null check, empty check, and duplicate detection on sorted vertices. The sorting step ensures O(n) duplicate checking rather than O(n²).
64-90: LGTM!The equality and hash code implementations are correct and consistent. The order-dependent comparison properly reflects the oriented nature of simplices.
src/PhysicsInformed/Benchmarks/PdeBenchmarkSuite.cs (6)
7-37: LGTM! Clean public API design.Both benchmark methods follow a consistent pattern with proper null checking and validation. The separation of concerns (validation, solving, error computation) makes the code maintainable.
39-45: LGTM! Proper validation.The null check is appropriate for the predictor function.
47-78: LGTM! Correct error metric computation.The L2 and max error calculations are mathematically sound. The evaluation at
FinalTimeonly (line 59) is a standard approach for PDE benchmarks comparing final steady states.
93-102: Good addition of diffusion CFL check.The diffusion stability constraint is now properly enforced. This addresses the stability concern for the viscous term.
145-154: Good addition of diffusion CFL check.The diffusion stability constraint
epsilon² * dt / dx² ≤ 0.5is properly enforced, addressing the main stability concern for this PDE.
185-195: LGTM! Clean grid generation.The uniform spatial grid is correctly constructed with proper endpoint inclusion.
src/PhysicsInformed/GpuPINNTrainer.cs (6)
65-79: LGTM!The constructor properly validates inputs, initializes fields with safe defaults, and conditionally attempts GPU initialization based on options. The logic is clear and correct.
85-135: LGTM!The GPU initialization logic is well-structured with proper error handling, idempotency checks, and informative logging. The method gracefully falls back to CPU if GPU is unavailable.
309-316: ReleaseGpuResources only resets flags.This method sets
_useGpuand_gpuInitializedto false but doesn't dispose of actual GPU resources (accelerators, buffers, etc.). If resource cleanup is handled by the underlying engine or PINN, this is acceptable. Otherwise, consider adding explicit disposal if resources are allocated.
334-351: LGTM!The method now correctly returns -1 sentinel values with comprehensive documentation explaining ILGPU's lack of memory query APIs. The approach of recommending external profiling tools is appropriate. This addresses the previous review concerns about misleading zero placeholders.
353-363: LGTM!The method now correctly checks
engine.SupportsGpurather than just checking for null. This properly validates GPU availability and addresses previous review concerns.
376-467: Supporting classes are well-documented.Both
GpuTrainingHistory<T>andPINNGpuMemoryInfohave comprehensive XML documentation. Notable:
PeakMemoryBytesdocumentation (lines 396-402) correctly clarifies it measures managed memory growth, not GPU memoryPINNGpuMemoryInfoproperly uses -1 sentinel for unavailable metrics with clear documentationThe earlier comments about renaming
PeakMemoryBytesandKernelTimingsfor clarity also apply to the property definitions here.src/PhysicsInformed/PDEs/MaxwellEquations.cs (6)
52-54: Permeability is now correctly used in computations.The
_permeabilityfield is now properly incorporated into the residual calculations at lines 150-155 and gradient computation at line 191, where it's multiplied with_permittivityto form the εμ product required by Maxwell's equations. This addresses the concern raised in the previous review.
66-100: LGTM!The constructors properly handle nullable parameters with sensible defaults (vacuum/normalized units), validate positivity of electromagnetic properties, and the double-parameter constructor correctly delegates to the generic version using the appropriate conversion helpers.
144-164: LGTM!The Maxwell equation residual computations are mathematically correct:
- Faraday's law properly combines time and spatial derivatives
- Ampere's law correctly uses the εμ product (addressing the physics)
- Sum of squared residuals provides a scalar loss for optimization
189-213: LGTM!The gradient computation correctly implements the chain rule for the sum of squared residuals. Each partial derivative with respect to the first derivatives properly scales by 2 and the appropriate residual component (faraday, ampereX, or ampereY), and correctly incorporates the εμ factor where needed.
218-224: LGTM!The properties correctly specify the dimensionality of the PDE (3 inputs: x, y, t; 3 outputs: Ex, Ey, Bz) and provide a descriptive name that includes the electromagnetic parameters for easy identification.
168-177: Apply the same input/dimension validation as inComputeResidual.This method has the same validation gaps as
ComputeResidual:
- Missing validation of
inputs.Length == 3- Missing validation of derivative matrix dimensions
For consistency and robustness, apply the same validation logic suggested for
ComputeResidualat lines 111-124.src/NeuralNetworks/Layers/HyperbolicLinearLayer.cs (3)
161-265: Forward pass implementation looks sound.The 1D/2D input handling is properly implemented with explicit conversions, and the hyperbolic operations (projection, exponential map, Mobius addition, distance) follow the expected sequence for a hyperbolic linear layer.
378-396: Bias update structure correctly implemented.The bias update now properly builds the full tangent vector from all gradient components and applies a single exponential map per output neuron, addressing the previous nested-loop issue.
404-460: Parameter management implementation is correct.The
GetParametersandSetParametersmethods properly flatten and restore the weight and bias matrices with appropriate validation. The parameter ordering is consistent (weights first, then biases).
…hyperbolic layer Apply the correct Poincare ball Riemannian gradient scaling ((1-c||x||^2)^2/4) to weight, bias, and input gradients during backward pass. This ensures mathematically correct gradient descent in hyperbolic space at all curvatures. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Renamed property to clearly indicate it tracks .NET managed heap memory via GC.GetTotalMemory(), not GPU device memory. Updated documentation to explicitly state this limitation and point to external profiling tools. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Fixed Ampere's law formulas to use 1/(εμ) instead of 1/ε, matching the actual implementation. Since B = μH, the curl of B gives ∇×B = εμ ∂E/∂t, not ∇×B = ε ∂E/∂t. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add validation for inputs.Length == 3 to match existing outputs validation. This prevents potential IndexOutOfRangeException when derivatives are accessed later in the method. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The GpuTrainingHistory property was renamed from PeakMemoryBytes to PeakManagedMemoryBytes in c30272d, but the test file was not updated. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
This PR implements comprehensive physics-informed machine learning and advanced algebraic capabilities for scientific computing.
Part 1: Physics-Informed Neural Networks (Fixes #400)
Physics-Informed Neural Networks (PINNs)
Neural Operators
Scientific Machine Learning
Directory Structure
Part 2: Advanced Algebra + Geometry (Sprint A)
Algebraic Types
Lie Groups and Algebras
Hyperbolic Manifolds
Simplicial Complexes (Topology)
Numeric Operations
Directory Structure
Remaining Sprints (Advanced Algebra)
Per the implementation plan in
SPRINT_PLAN_PR449_ADVANCED_ALGEBRA.md:Key Features
Verification
Files Modified (Sprint A)
Fixes #400