fix: fix #700: diffusion conv GPU training + auto eigenbasis

[ooples]

Summary\n- enable GPU training for DiffusionConv with cached tensors and GPU backprop\n- auto-compute eigenbasis from Laplacian for GPU/CPU paths with optional direct CPU mode\n- mark ConvLSTM GPU training support and update GPU status docs\n\n## Testing\n- dotnet build AiDotNet.sln -c Release -v minimal (warnings: NU1608 Pomelo EFCore, CS8618 SimdBenchmark, xUnit1031 and existing nullable warnings in tests)

[github-actions]

Commit Messages Auto-Fixed

The commitlint check failed because one or more commit messages did not follow Conventional Commits format.

Action taken - All non-compliant commits have been fixed to follow the conventional commits format.

Changes made:

• Subject lines are now lowercase (all types including deps)
• Types are now one of: feat, fix, docs, refactor, perf, test, chore, ci, style, or deps

The PR branch has been force-pushed with the fixed commits. If you had local changes, you may need to git pull --rebase.

[github-actions]

🤖 PR Title Auto-Fixed

Your PR title was automatically updated to follow Conventional Commits format.

Original title:
Fix #700: diffusion conv GPU training + auto eigenbasis

New title:
fix: fix #700: diffusion conv GPU training + auto eigenbasis

Detected type: fix: (title starts with fix/correct/resolve/patch)
Version impact: MINOR version bump (0.1.0 → 0.2.0)

---

Valid types and their effects:

• feat: - New feature (MINOR bump: 0.1.0 → 0.2.0)
• fix: - Bug fix (MINOR bump)
• docs: - Documentation (MINOR bump)
• refactor: - Code refactoring (MINOR bump)
• perf: - Performance improvement (MINOR bump)
• test: - Tests only (no release)
• chore: - Build/tooling (no release)
• ci: - CI/CD changes (no release)
• style: - Code formatting (no release)
• deps: - Dependency update (no release)

If the detected type is incorrect, you can manually edit the PR title.

[coderabbitai]

Warning

Rate limit exceeded

@ooples has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 16 minutes and 41 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

📥 Commits

Reviewing files that changed from the base of the PR and between 701603e and 7954417.

📒 Files selected for processing (2)

• docs/GPU_TRAINING_STATUS.md
• src/NeuralNetworks/Layers/DiffusionConvLayer.cs

Note

Other AI code review bot(s) detected

CodeRabbit 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.

Walkthrough

This PR adds GPU training support to ConvLSTMLayer and DiffusionConvLayer by introducing SupportsGpuTraining properties, implementing GPU-based backward pass and parameter updates, adding tensor caching mechanisms, and auto-generating eigenbasis from Laplacian matrices as needed. Documentation is updated to reflect these GPU implementations.

Changes

Cohort / File(s)	Summary
Documentation Status Updates docs/GPU_KERNEL_STATUS.md, docs/GPU_TRAINING_STATUS.md	Updated GPU kernel and training status tables: ConvLSTM and DiffusionConv kernels now marked as ⚠️ (composed/spectral implementations), and ConvLSTMLayer training marked as ✅ (implemented).
ConvLSTM Layer GPU Training src/NeuralNetworks/Layers/ConvLSTMLayer.cs	Added SupportsGpuTraining property returning true; updated forward GPU documentation to reflect that gate values and state buffers are cached for full GPU BPTT during backward pass.
DiffusionConv Layer GPU Pipeline src/NeuralNetworks/Layers/DiffusionConvLayer.cs	Comprehensive GPU training support: added SupportsGpuTraining property, GPU tensor fields for caching (input, diffused features, output, weights, biases, gradients), eigenbasis auto-computation from Laplacian, GPU forward/backward/parameter-update methods, optimizer state management, and serialization support.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Layer as DiffusionConvLayer
    participant GPU as GPU Backend
    participant Cache as Tensor Cache
    participant Optim as Optimizer

    User->>Layer: ForwardGpu(input, training=true)
    Layer->>Layer: EnsureEigenbasisForExecution()
    Layer->>GPU: Allocate GPU tensors
    GPU->>Cache: Store input, diffused_features, output
    Layer-->>User: output

    User->>Layer: BackwardGpu(loss_gradient)
    Layer->>Cache: Retrieve cached forward tensors
    Cache-->>Layer: input, diffused_features, output
    Layer->>GPU: Compute weight/bias/diffusion gradients
    GPU->>GPU: Spectral or direct diffusion path
    GPU->>Cache: Store gradients
    Layer->>GPU: Compute input gradients
    Layer-->>User: input_gradient

    User->>Layer: UpdateParametersGpu(optimizer)
    Layer->>Optim: Get optimizer state (velocity, moments)
    Layer->>GPU: Apply updates with optimizer
    Layer->>Layer: Clamp diffusion times
    Layer->>Cache: Clear GPU cache post-update

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

• Implement GPU backward pass for ConvLSTMLayer and DiffusionConvLayer #700: Directly addresses the request for GPU BackwardGpu, full GPU training support, and SupportsGpuTraining properties for both ConvLSTMLayer and DiffusionConvLayer with GPU state caching and eigenbasis fallback logic.

Possibly related PRs

• chore: feat/gpu resident training #702: Overlaps with GPU-resident training support, ConvLSTM layer changes, and GPU kernel/optimizer infrastructure.
• chore: continue GPU architecture cleanup work #497: Shares GPU training implementation and ConvLSTMLayer modifications with overlapping GPU documentation and infrastructure updates.

Suggested labels

feature, roadmap

Poem

🐰 Hops of GPU delight!
Through spectral paths and eigenbasis bright,
ConvLSTM and DiffusionConv now train,
On GPUs swift, with gradients—no strain!
Backwards we flow, parameters align,
A computational garden designed to shine! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 73.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically summarizes the main changes: adding GPU training support for DiffusionConv with auto eigenbasis computation, and marking ConvLSTM GPU support.
Description check	✅ Passed	The description directly relates to the changeset, outlining the three main objectives: GPU training for DiffusionConv, auto eigenbasis computation, and ConvLSTM GPU training documentation updates.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/issue-700-gpu-backward

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Copilot]

Pull request overview

This PR enables GPU training for DiffusionConvLayer and marks ConvLSTMLayer as GPU-training-capable by implementing GPU-resident backward passes and parameter updates.

Changes:

• Implemented full GPU backward pass for DiffusionConvLayer with cached tensors and gradient computation
• Added automatic eigenbasis computation from Laplacian for both GPU and CPU execution paths
• Updated documentation to reflect GPU training support for both layers

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

File	Description
src/NeuralNetworks/Layers/DiffusionConvLayer.cs	Implements GPU backward pass, automatic eigenbasis computation, and GPU parameter updates
src/NeuralNetworks/Layers/ConvLSTMLayer.cs	Updates comments to reflect existing GPU training support
docs/GPU_TRAINING_STATUS.md	Marks ConvLSTMLayer as fully GPU-capable
docs/GPU_KERNEL_STATUS.md	Updates kernel status for ConvLSTM and DiffusionConv layers

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/NeuralNetworks/Layers/DiffusionConvLayer.cs (1)

788-1060: ForwardGpu performs repeated GPU→CPU→GPU transfers per time scale, severely limiting performance.

The inner loop (lines 984-1005) downloads both timeOutputBuffer and the entire diffusedBuffer, scatters on CPU, then re-uploads—repeated for each time scale. This causes O(NumTimeScales) roundtrips and defeats GPU-resident training efficiency.

Additionally, two buffers are allocated but unused:

• decayBuffer (line 901) is never referenced
• decayTiledBuffer (line 929) is never used (code uses decayTiledUploadBuffer instead)

Replace the CPU-side scatter with a device-side strided copy operation and remove the unused allocations.

🤖 Fix all issues with AI agents

In `@docs/GPU_TRAINING_STATUS.md`:
- Around line 324-325: Add a short explanatory note near the
ConvLSTMLayer/GRULayer table clarifying that GPU training support depends on
using the DirectGpu backend and on having ForwardGpu run in training mode to
cache activations (since ConvLSTMLayer uses composed kernels + cached forward
state) before invoking BackwardGpu; mention these prerequisites explicitly and
link them to the symbols DirectGpu, ForwardGpu, BackwardGpu and ConvLSTMLayer so
readers know the required execution order and backend for GPU training to work.

In `@src/NeuralNetworks/Layers/DiffusionConvLayer.cs`:
- Around line 462-472: Compute the eigenbasis before entering any Parallel.For
by calling EnsureEigenbasisForExecution (or extracting its eigen-computation
logic) from ProcessBatched and only after SetLaplacian/_massMatrix changes, or
else guard the mutation of _eigenvalues/_eigenvectors in
EnsureEigenbasisForExecution with a thread-safe lock/double-checked pattern to
avoid races from ComputeDiffusedFeatures; additionally, if a non-null
_massMatrix is meaningful for the Laplacian, replace the plain L
eigen-decomposition with a generalized eigenproblem solver for L φ = λ M φ (or
explicitly document that only standard L decomposition is supported), updating
EnsureEigenbasisForExecution and any helpers used by ComputeDiffusedFeatures to
use the generalized solution when _massMatrix != null.

🧹 Nitpick comments (3)

src/NeuralNetworks/Layers/DiffusionConvLayer.cs (3)

2-4: New API/behavior knobs look reasonable; please document semantics and persistence.

• preferSpectralDiffusion: bool? is subtle (null/true/false). Consider an enum for readability, or at least add a brief note in class-level docs + XML param docs about when eigenbasis computation happens and its cost.
• SupportsGpuTraining => true is fine, but the runtime requirement is effectively “GPU training supported when eigenbasis is available or Laplacian is provided and we can compute eigenbasis.”

Also applies to: 98-102, 178-187

---

1064-1399: BackwardGpu structure is solid, but it’s very allocation-heavy; consider caching & avoiding CPU materializations.

• Many per-time-scale allocations (weightsSlice, decayTiledData, derivTiledData) and repeated GPU uploads will add overhead; consider keeping weights/eigenbasis and per-t buffers on GPU once (especially since the PR goal is GPU-resident training).
• Fallback path downloads to CPU for activation derivative (Line 1165-1172). That’s acceptable as a fallback, but it’d be good to explicitly document that custom/non-fused activations will incur host transfers.

---

2010-2016: Clone/ResetState updates are consistent; consider whether preferSpectralDiffusion should serialize.

• Good: Clone() propagates _preferSpectralDiffusion (Line 2010-2016) and ResetState() clears GPU caches (Line 2040-2059).
• If _preferSpectralDiffusion materially changes runtime behavior, consider serializing it too (otherwise deserialization will silently revert to the constructor default).

Also applies to: 2040-2059

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 85150b8 and 701603e.

📒 Files selected for processing (4)

• docs/GPU_KERNEL_STATUS.md
• docs/GPU_TRAINING_STATUS.md
• src/NeuralNetworks/Layers/ConvLSTMLayer.cs
• src/NeuralNetworks/Layers/DiffusionConvLayer.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/NeuralNetworks/Layers/ConvLSTMLayer.cs
• src/NeuralNetworks/Layers/DiffusionConvLayer.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/Layers/ConvLSTMLayer.cs
• src/NeuralNetworks/Layers/DiffusionConvLayer.cs

📚 Learning: 2025-12-19T19:05:13.598Z

Learnt from: ooples
Repo: ooples/AiDotNet PR: 445
File: src/Interfaces/IPredictionModelBuilder.cs:7-8
Timestamp: 2025-12-19T19:05:13.598Z
Learning: In ooples/AiDotNet, IAggregationStrategy<TModel> is declared in AiDotNet.Interfaces (src/Interfaces/IAggregationStrategy.cs). For src/Interfaces/IPredictionModelBuilder.cs (also in AiDotNet.Interfaces), referencing IAggregationStrategy does not require an additional using. Avoid flagging missing using directives for this case in future reviews.

Applied to files:

• src/NeuralNetworks/Layers/DiffusionConvLayer.cs

🔇 Additional comments (4)

docs/GPU_KERNEL_STATUS.md (1)

183-184: Good status change; ensure ⚠️ meaning is consistently “no dedicated kernel, composed ops.”

The ⚠️ entries read correctly as “composed from Conv2D/GEMM/elementwise ops” (not a standalone kernel). Consider adding the same “composed” qualifier anywhere these appear in the doc (including the priority list) to prevent readers assuming partial backend parity issues vs composition.

Also applies to: 231-232, 286-287

src/NeuralNetworks/Layers/ConvLSTMLayer.cs (2)

246-250: SupportsGpuTraining=>true is a behavior contract: please gate it behind validated BackwardGpu/UpdateParametersGpu.

With this flip, training will route through GPU BPTT and GPU updates; any subtle shape/lifetime bugs in those paths become release-blocking. I’d strongly recommend adding at least one integration test that runs ForwardGpu(train) → BackwardGpu → UpdateParametersGpu and asserts loss decreases / gradients finite.

---

649-650: ForwardGpu doc update matches the new training path.

Nice to explicitly document the cache dependency for GPU BPTT; that helps prevent misuse (ForwardGpu inference-only → BackwardGpu).

src/NeuralNetworks/Layers/DiffusionConvLayer.cs (1)

1405-1455: UpdateParametersGpu: diffusion-times update/clamp is good; verify optimizer-state mapping matches config expectations.

You reuse Velocity buffers as SquaredAvg/AccumulatedGrad. If IGpuOptimizerConfig.ApplyUpdate interprets these fields differently per optimizer, double-check that RMSProp/Adagrad aren't accidentally sharing a buffer with momentum/velocity semantics.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

In general, to fix this pattern you merge the outer and inner if conditions into a single if statement using &&, since both must be true to execute the body. This removes unnecessary nesting and improves readability while preserving semantics.

Concretely, in ComputeDiffusedFeatures in src/NeuralNetworks/Layers/DiffusionConvLayer.cs, replace the nested structure:

if (_eigenvalues == null || _eigenvectors == null)
{
    if (_preferSpectralDiffusion != false)
    {
        EnsureEigenbasisForExecution();
    }
}

with a single combined condition:

if ((_eigenvalues == null || _eigenvectors == null) && _preferSpectralDiffusion != false)
{
    EnsureEigenbasisForExecution();
}

This preserves operator precedence clearly via parentheses, maintains identical short‑circuiting (the second part is only evaluated if the first is true), and does not change any subsequent logic. No new methods, imports, or auxiliary definitions are required.

In general, the fix is to replace the manual “declare resource, assign in try, dispose in finally” pattern with C# using resource management. That can be done either as using statements with blocks or as using declarations, which dispose the resource at the end of the containing scope.

In this method, the cleanest change without altering functionality is:

• Keep the existing try/catch/finally structure for any non‑disposal logic.
• Convert weightGradBuffer, biasGradBuffer, and inputGradBuffer to using declarations so they are automatically disposed at the end of the try block’s scope.
• Remove the explicit finally block that calls Dispose() on each buffer, as it becomes redundant.

Concretely:

• Around lines 1166–1168, change the three nullable variables to using IGpuBuffer? ... = null;.
• Around lines 1437–1442, remove the finally block that disposes them, leaving the closing brace of the method directly after the return inputGradTensor;.

No new imports, methods, or definitions are needed; this relies only on standard C# using semantics.

In general, to fix this kind of issue you replace a try/finally pattern that manually calls Dispose with a using statement that automatically disposes the resource when control leaves the scope, including when exceptions are thrown. When ownership of the resource may be transferred (so that it should not be disposed at the end of the method), you introduce a separate local variable managed by using and clear it when ownership is transferred. The disposable field or outer variable itself does not need to be in a using; only the temporary handle does.

For this specific method, weightGradBuffer, biasGradBuffer, and inputGradBuffer are declared before the try and disposed in the finally. CodeQL flags inputGradBuffer, but the same reasoning applies to all three. The best fix without changing functionality is to:

1. Introduce separate “owned” local variables that are actually created in the try block (for example, weightGradOwned, biasGradOwned, inputGradOwned) and wrap each of those in its own using statement.
2. Keep weightGradBuffer, biasGradBuffer, and inputGradBuffer as the variables that are passed into the created tensors, but have them just reference the owned buffers.
3. When ownership is transferred to a GpuTensor<T> (via ownsBuffer: true), set the corresponding owned variable to null (or otherwise clear it) so that the using does not attempt to dispose it again. This emulates the existing “set to null before finally” behavior.
4. Remove the finally block for these three variables, since disposal is now handled by the using statements, while preserving all other logic.

Concretely, inside BackwardGpu, in the region where weightGradBuffer, biasGradBuffer, and inputGradBuffer are used, we’ll:

• Replace the single big try/finally that disposes them with nested using blocks for newly introduced owned locals.
• Update buffer allocations to assign to the owned locals, then to the original buffer variables.
• Before constructing GpuTensor<T> instances with ownsBuffer: true, set the owned local to null and keep returning as before.
• Remove the explicit .Dispose() calls in the finally block.

This change is fully contained within BackwardGpu in src/NeuralNetworks/Layers/DiffusionConvLayer.cs and requires no new imports or additional methods.

In general, the fix is to replace the manual try/finally-based disposal of IGpuBuffer instances with using statements (or using var declarations), so that the C# compiler automatically generates the correct try/finally disposal logic. This makes the intent clearer and reduces boilerplate.

Concretely here, the best fix is:

• Change tiledEigBuffer and tiledEigTBuffer from nullable fields initialized to null and disposed in a finally block, to using var local variables that are conditionally assigned when batchSize > 1.
• Remove the explicit try/finally block and the manual .Dispose() calls.
• Keep the rest of the logic (conditional allocation, usage, and the null-coalescing/exception check before BatchedGemm) unchanged, so functional behavior remains the same.
• No additional imports or helper methods are required; using var works directly on IGpuBuffer.

The scope of these using var declarations will be the current method’s body (or the surrounding block where we declare them), which is effectively the same as the former try scope plus finally, so resources will still be disposed after all their uses, including when exceptions are thrown.

General fix: Replace the manual try/finally with explicit using-based lifetime management for the disposable GPU buffers. The goal is to keep semantics identical: only allocate the batched buffers when batchSize > 1, use them where needed, and ensure they are disposed afterwards, even on exceptions.

Best concrete fix in this snippet:

• Keep tiledEigBuffer and tiledEigTBuffer as locals inside the batchSize > 1 branch, but wrap them in using blocks so they are always disposed when leaving that branch.
• For batchSize == 1, we don’t allocate these buffers at all, so we don’t need nullable fields or a finally.
• Because the code after the if needs access to tiledEigTBuffer (in the else branch at line 1250), we can:
  • Move the if (batchSize > 1) block up into its own if/else around the GEMM call so that the using-scoped buffer is used entirely inside that block; or
  • Use C# using declarations for tiledEigBuffer and tiledEigTBuffer that live from their declaration to the end of the containing scope.
• The smallest change within the shown region is to turn the existing try/finally into a using-declaration style: declare using IGpuBuffer? tiledEigBuffer = null; using IGpuBuffer? tiledEigTBuffer = null; and remove the finally disposal, since using declarations will automatically call Dispose at scope end. However, using declarations cannot be nullable where you want to skip allocation; they can, but Dispose will be called on null and must be safe. To avoid relying on that, a better adjustment is:
  • Keep them nullable but declare them with using declarations and rely on the runtime not calling Dispose on null. In C#, the compiler emits a null check before Dispose, so this is safe.
• Thus:
  • Change IGpuBuffer? tiledEigBuffer = null; to using IGpuBuffer? tiledEigBuffer = null;
  • Change IGpuBuffer? tiledEigTBuffer = null; to using IGpuBuffer? tiledEigTBuffer = null;
  • Remove the try/finally wrapper and its manual Dispose calls, just leaving the body of the old try block in place.
• No new imports or additional methods are required.

Line‑level changes:

• In DiffusionConvLayer<T> method containing lines 1230–1382 in src/NeuralNetworks/Layers/DiffusionConvLayer.cs:
  • Replace the declarations of tiledEigBuffer and tiledEigTBuffer with using declarations.
  • Remove the try/finally structure, keeping only the former try block contents in place.