feat: add learning rate scheduler integration and refactor diffusion architecture

[ooples]

Summary

This PR implements Issue #388 - AdamW, Learning Rate Schedulers, and Scheduler Architecture Refactor.

Changes:

• Add SchedulerStepMode enum with StepPerBatch, StepPerEpoch, and WarmupThenEpoch modes
• Add LR scheduler support to GradientBasedOptimizerOptions with LearningRateScheduler and SchedulerStepMode properties
• Add LR scheduler integration to GradientBasedOptimizerBase with:
  • Step tracking fields (_currentStep, _currentEpoch)
  • OnEpochEnd(), OnBatchEnd(), StepScheduler() methods
  • IsInWarmupPhase() heuristic for warmup detection
  • Proper scheduler reset in Reset() method
• Move Diffusion to NeuralNetworks (src/Diffusion/ → src/NeuralNetworks/Diffusion/)
• Rename IStepScheduler to INoiseScheduler to avoid confusion with learning rate schedulers
• Rename StepSchedulerBase to NoiseSchedulerBase for consistency

All 25 gradient-based optimizers automatically inherit scheduler support via the base class.

AdamW already has proper decoupled weight decay implementation.

Closes #388

Test plan

• Verify build passes on all target frameworks
• Test scheduler integration with AdamW optimizer
• Verify existing diffusion model tests still pass with new namespace

🤖 Generated with Claude Code

[coderabbitai]

Warning

Rate limit exceeded

@ooples has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 20 minutes and 23 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 6b3c3cc and 7eb2c25.

📒 Files selected for processing (4)

• testconsole/Examples/EnhancedRegressionExample.cs
• testconsole/Examples/EnhancedTimeSeriesExample.cs
• testconsole/Examples/RegressionExample.cs
• testconsole/Examples/TimeSeriesExample.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

Renames diffusion scheduler types and moves diffusion code into a NeuralNetworks namespace, integrates learning-rate scheduler support into gradient-based optimizers (new enum and lifecycle hooks), renames optimizer learning-rate properties to InitialLearningRate, and adds integration tests for optimizer + scheduler behaviors.

Changes

Cohort / File(s)	Summary
Diffusion scheduler rename & move src/Interfaces/INoiseScheduler.cs, src/Interfaces/IDiffusionModel.cs, src/NeuralNetworks/Diffusion/... (e.g., DiffusionModelBase.cs, DDPMModel.cs, Schedulers/SchedulerConfig.cs, Schedulers/NoiseSchedulerBase.cs, Schedulers/DDIMScheduler.cs, Schedulers/PNDMScheduler.cs)	Moved diffusion types into AiDotNet.NeuralNetworks.Diffusion and renamed scheduler interface/type from IStepScheduler<T>/StepSchedulerBase<T> → INoiseScheduler<T>/NoiseSchedulerBase<T>; updated constructors, properties, inheritance, namespaces and XML docs to use the new names.
Learning-rate scheduler step mode src/LearningRateSchedulers/SchedulerStepMode.cs	Added public enum SchedulerStepMode with StepPerBatch, StepPerEpoch, WarmupThenEpoch.
Optimizer base: LR scheduler integration src/Optimizers/GradientBasedOptimizerBase.cs	Added nullable ILearningRateScheduler fields, scheduler step-mode/state (_schedulerStepMode, _currentStep, _currentEpoch), public accessors/mutators, StepScheduler(), OnBatchEnd(), OnEpochEnd(), GetCurrentLearningRate(), warmup-aware logic, and reset integration.
Optimizer options updated src/Models/Options/GradientBasedOptimizerOptions.cs, src/Models/Options/*OptimizerOptions.cs (multiple files)	Added ILearningRateScheduler? LearningRateScheduler and SchedulerStepMode SchedulerStepMode to gradient-based options; many optimizer option classes renamed LearningRate → new InitialLearningRate (hidden member) and tests/usage updated accordingly. Note: duplicate insertion observed in GradientBasedOptimizerOptions.cs.
Optimizers adapted to base scheduler model src/Optimizers/* (e.g., AdamOptimizer.cs, AdamWOptimizer.cs usages, AMSGradOptimizer.cs, AdaMaxOptimizer.cs, LionOptimizer.cs, NadamOptimizer.cs)	Removed per-optimizer private learning-rate fields; use CurrentLearningRate from base class and sync from scheduler/options; calls to base.Reset() added; internal updates and gradient cache keys changed to reference InitialLearningRate.
Optimizer-related wiring in higher-level code src/Regression/*, src/ReinforcementLearning/Agents/*, src/Models/Options/MultilayerPerceptronRegressionOptions.cs	Replaced LearningRate usages with InitialLearningRate when constructing default optimizers across regressors and RL agents.
New integration tests tests/AiDotNet.Tests/IntegrationTests/Optimizers/OptimizerSchedulerIntegrationTests.cs	Added comprehensive integration tests validating optimizer + learning-rate scheduler interactions, warmup, stepping modes, reset behavior, and cross-optimizer compatibility.
Test namespace updates & unit test changes tests/.../Diffusion/*, tests/.../Optimizers/* (multiple files)	Updated using directives to new diffusion namespace and updated unit tests to use InitialLearningRate where option property was renamed; no logic changes beyond test expectations.

Sequence Diagram(s)

mermaid
sequenceDiagram
autonumber
participant Trainer
participant Optimizer
participant LR_Scheduler as LearningRateScheduler
participant Model
Note over Trainer,Optimizer: Training loop invokes batch/epoch lifecycle
Trainer->>Optimizer: UpdateParameters(batch)
Optimizer->>Model: ApplyGradients()
alt Scheduler mode = StepPerBatch or warmup active
Optimizer->>LR_Scheduler: Step(currentStep)
LR_Scheduler-->>Optimizer: NewLearningRate
Optimizer->>Optimizer: CurrentLearningRate = NewLearningRate
Optimizer->>Optimizer: _currentStep++
end
Trainer->>Optimizer: OnEpochEnd()
alt Scheduler mode = StepPerEpoch or warmup completed
Optimizer->>LR_Scheduler: Step(currentEpoch)
LR_Scheduler-->>Optimizer: NewLearningRate
Optimizer->>Optimizer: CurrentLearningRate = NewLearningRate
Optimizer->>Optimizer: _currentEpoch++
end

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• feat(diffusion): schedulers + integrate into diffusion model (Part of #261, Fixes #263) #294: Renames/moves diffusion scheduler types (IStepScheduler → INoiseScheduler) and updates affected models — strong overlap with diffusion namespace and type rename changes.
• Fix Issue for Lion Optimizer #381: Changes to optimizers/options and learning-rate scheduler integration (InitialLearningRate and scheduler wiring) — closely related to optimizer and options edits.

Poem

🐰 Hop-hop, the namespaces leap today,

Noise schedulers hop where step once lay,
Optimizers learn to time their pace,
Epochs and batches waltz in place,
I nibble code carrots — celebrate the race!

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	The PR contains significant duplicate property declarations in GradientBasedOptimizerOptions.cs (LearningRateScheduler and SchedulerStepMode appear twice), indicating incomplete merge/refactoring that should have been resolved before submission.	Remove duplicate property declarations (LearningRateScheduler and SchedulerStepMode) from GradientBasedOptimizerOptions.cs to ensure a single, clean public API definition.
Docstring Coverage	⚠️ Warning	Docstring coverage is 43.43% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main changes: adding learning rate scheduler integration and refactoring the diffusion architecture with namespace reorganization.
Description check	✅ Passed	The description is directly related to the changeset, providing a comprehensive summary of all major modifications including scheduler integration, optimizer updates, and diffusion refactoring.
Linked Issues check	✅ Passed	The PR implements all major objectives from Issue #388: SchedulerStepMode enum, LR scheduler integration in base optimizer class, namespace/naming refactor for diffusion components (IStepScheduler→INoiseScheduler, move to NeuralNetworks), and optimizer property updates (LearningRate→InitialLearningRate).

---

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

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/Optimizers/GradientBasedOptimizerBase.cs (1)

1031-1046: Warmup heuristic may misfire after learning rate decay.

The heuristic currentLr < baseLr will return true for any scheduler that has decayed below the base rate (e.g., step decay, cosine annealing, reduce-on-plateau), not just during warmup. This could cause WarmupThenEpoch mode to unexpectedly switch back to per-batch stepping after the learning rate decays.

Consider querying the scheduler directly for warmup state:

🔎 Proposed improvement

 protected virtual bool IsInWarmupPhase()
 {
     if (_learningRateScheduler == null)
     {
         return false;
     }
 
-    // Check if the scheduler supports warmup detection
-    // A scheduler is in warmup if current LR is below base LR and still increasing
-    var currentLr = _learningRateScheduler.CurrentLearningRate;
-    var baseLr = _learningRateScheduler.BaseLearningRate;
-
-    // If current LR is less than base LR, we're likely still in warmup
-    // This is a heuristic - specific schedulers may override this behavior
-    return currentLr < baseLr;
+    // Query the scheduler directly if it supports warmup detection
+    if (_learningRateScheduler is IWarmupAwareScheduler warmupScheduler)
+    {
+        return warmupScheduler.IsInWarmupPhase;
+    }
+
+    // Fallback: check if current LR is below base LR AND we're in early steps
+    // This adds a step threshold to avoid false positives after decay
+    var currentLr = _learningRateScheduler.CurrentLearningRate;
+    var baseLr = _learningRateScheduler.BaseLearningRate;
+    return currentLr < baseLr && _currentStep < 1000; // Reasonable warmup threshold
 }

Alternatively, introduce an IWarmupAwareScheduler interface that warmup-capable schedulers can implement to provide accurate warmup state detection.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 96ca241 and 763318c.

📒 Files selected for processing (11)

• src/Interfaces/IDiffusionModel.cs
• src/Interfaces/INoiseScheduler.cs
• src/LearningRateSchedulers/SchedulerStepMode.cs
• src/Models/Options/GradientBasedOptimizerOptions.cs
• src/NeuralNetworks/Diffusion/DDPMModel.cs
• src/NeuralNetworks/Diffusion/DiffusionModelBase.cs
• src/NeuralNetworks/Diffusion/Schedulers/DDIMScheduler.cs
• src/NeuralNetworks/Diffusion/Schedulers/NoiseSchedulerBase.cs
• src/NeuralNetworks/Diffusion/Schedulers/PNDMScheduler.cs
• src/NeuralNetworks/Diffusion/Schedulers/SchedulerConfig.cs
• src/Optimizers/GradientBasedOptimizerBase.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/Diffusion/Schedulers/DDIMScheduler.cs
• src/Interfaces/IDiffusionModel.cs
• src/LearningRateSchedulers/SchedulerStepMode.cs
• src/Interfaces/INoiseScheduler.cs
• src/NeuralNetworks/Diffusion/Schedulers/NoiseSchedulerBase.cs
• src/NeuralNetworks/Diffusion/Schedulers/PNDMScheduler.cs
• src/NeuralNetworks/Diffusion/DiffusionModelBase.cs
• src/NeuralNetworks/Diffusion/Schedulers/SchedulerConfig.cs
• src/Models/Options/GradientBasedOptimizerOptions.cs
• src/NeuralNetworks/Diffusion/DDPMModel.cs
• src/Optimizers/GradientBasedOptimizerBase.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/Diffusion/Schedulers/DDIMScheduler.cs
• src/Interfaces/IDiffusionModel.cs
• src/LearningRateSchedulers/SchedulerStepMode.cs
• src/Interfaces/INoiseScheduler.cs
• src/NeuralNetworks/Diffusion/Schedulers/NoiseSchedulerBase.cs
• src/NeuralNetworks/Diffusion/Schedulers/PNDMScheduler.cs
• src/NeuralNetworks/Diffusion/DiffusionModelBase.cs
• src/NeuralNetworks/Diffusion/Schedulers/SchedulerConfig.cs
• src/Models/Options/GradientBasedOptimizerOptions.cs
• src/NeuralNetworks/Diffusion/DDPMModel.cs
• src/Optimizers/GradientBasedOptimizerBase.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/Diffusion/DiffusionModelBase.cs
• src/Optimizers/GradientBasedOptimizerBase.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). (4)

• GitHub Check: CodeQL analysis (csharp)
• GitHub Check: Agent
• GitHub Check: CodeQL Analysis
• GitHub Check: Build (Windows)

🔇 Additional comments (15)

src/Models/Options/GradientBasedOptimizerOptions.cs (1)

131-164: LGTM! Clean integration of scheduler options.

The nullable ILearningRateScheduler? property with null defaulting to constant learning rate maintains backward compatibility. The StepPerEpoch default aligns with the documented best practice for most schedulers.

src/LearningRateSchedulers/SchedulerStepMode.cs (1)

1-45: Well-designed enum with clear semantics.

The three modes cover the common scheduling patterns. The documentation clearly explains when to use each mode, with the WarmupThenEpoch mode being particularly useful for transformer-style training workflows.

src/Optimizers/GradientBasedOptimizerBase.cs (5)

91-122: LGTM!

The protected fields appropriately allow derived optimizer classes to access scheduler state if needed for specialized behavior.

---

163-172: LGTM!

Correctly synchronizes the initial learning rate from the scheduler when provided, ensuring consistency between the optimizer and scheduler states from the start.

---

913-928: LGTM!

The reset logic properly mirrors the constructor initialization, ensuring a clean slate for scheduler state when the optimizer is reset.

---

966-1018: LGTM!

The lifecycle hooks correctly implement all three stepping modes. The WarmupThenEpoch logic properly transitions from per-batch stepping during warmup to per-epoch stepping afterward.

---

1048-1070: LGTM!

Clean public API for monitoring learning rate and training progress. Useful for logging, debugging, and implementing custom training loops.

src/Interfaces/INoiseScheduler.cs (1)

1-188: LGTM! Clean interface rename with excellent documentation.

The interface rename from IStepScheduler<T> to INoiseScheduler<T> effectively distinguishes diffusion noise schedulers from learning rate schedulers. The updated documentation (lines 16-19) clearly explains the rationale. Namespace migration to AiDotNet.NeuralNetworks.Diffusion.Schedulers aligns with the broader reorganization.

src/NeuralNetworks/Diffusion/Schedulers/SchedulerConfig.cs (1)

3-3: LGTM! Namespace updated to match new directory structure.

The namespace change aligns with the relocation of diffusion schedulers to AiDotNet.NeuralNetworks.Diffusion.Schedulers.

src/NeuralNetworks/Diffusion/Schedulers/DDIMScheduler.cs (1)

1-1: LGTM! Base class and namespace updated consistently.

The namespace migration and base class rename from StepSchedulerBase<T> to NoiseSchedulerBase<T> are consistent with the broader refactoring. All implementation logic remains unchanged.

Also applies to: 34-34

src/NeuralNetworks/Diffusion/Schedulers/PNDMScheduler.cs (1)

1-1: LGTM! Consistent refactoring applied to PNDM scheduler.

The namespace and base class updates match the pattern across all scheduler implementations. The complex PRK/PLMS logic remains intact.

Also applies to: 38-38

src/Interfaces/IDiffusionModel.cs (1)

36-36: LGTM! Interface updated to use renamed scheduler type.

The Scheduler property type updated from IStepScheduler<T> to INoiseScheduler<T>, with corresponding documentation reference updated on line 36.

Also applies to: 59-59

src/NeuralNetworks/Diffusion/DiffusionModelBase.cs (1)

3-3: LGTM! Comprehensive scheduler type updates applied consistently.

All references to IStepScheduler<T> updated to INoiseScheduler<T> throughout the base class (private field line 49, public property line 72, constructor parameter line 88). Namespace migration to AiDotNet.NeuralNetworks.Diffusion aligns with the reorganization.

Also applies to: 12-12, 49-49, 72-72, 88-88

src/NeuralNetworks/Diffusion/DDPMModel.cs (1)

1-1: LGTM! Complete scheduler type migration in DDPM model.

All constructor signatures (lines 104, 119) and the DeepCopy method (line 204) updated to use INoiseScheduler<T>. Namespace migration to AiDotNet.NeuralNetworks.Diffusion is consistent.

Also applies to: 5-5, 104-104, 119-119, 204-204

src/NeuralNetworks/Diffusion/Schedulers/NoiseSchedulerBase.cs (1)

4-4: LGTM! Base class rename executed comprehensively.

The class rename from StepSchedulerBase<T> to NoiseSchedulerBase<T> is complete and well-documented. The explanation in lines 17-19 clarifies the distinction from learning rate schedulers. All implementation logic remains unchanged - this is a pure naming refactor.

Also applies to: 7-31, 89-94

[Copilot]

Pull request overview

This PR implements learning rate scheduler integration into gradient-based optimizers and performs a major refactoring of the diffusion model architecture to improve code organization and naming clarity.

Key Changes:

• Adds comprehensive learning rate scheduler support to all 25 gradient-based optimizers via GradientBasedOptimizerBase, including configurable step modes (per-batch, per-epoch, warmup-then-epoch)
• Moves diffusion model code from src/Diffusion/ to src/NeuralNetworks/Diffusion/ for better architectural organization
• Renames diffusion-related interfaces and classes (IStepScheduler → INoiseScheduler, StepSchedulerBase → NoiseSchedulerBase) to avoid confusion with learning rate schedulers

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/LearningRateSchedulers/SchedulerStepMode.cs	New enum defining when schedulers should step (per-batch, per-epoch, or warmup-then-epoch modes)
src/Models/Options/GradientBasedOptimizerOptions.cs	Adds LearningRateScheduler and SchedulerStepMode properties to optimizer configuration
src/Optimizers/GradientBasedOptimizerBase.cs	Integrates scheduler support with step tracking, lifecycle methods (OnEpochEnd, OnBatchEnd), and warmup detection logic
src/NeuralNetworks/Diffusion/Schedulers/SchedulerConfig.cs	Updates namespace from AiDotNet.Diffusion.Schedulers to AiDotNet.NeuralNetworks.Diffusion.Schedulers
src/NeuralNetworks/Diffusion/Schedulers/NoiseSchedulerBase.cs	Renames from StepSchedulerBase and updates namespace; adds documentation explaining the rename
src/NeuralNetworks/Diffusion/Schedulers/PNDMScheduler.cs	Updates base class reference and namespace
src/NeuralNetworks/Diffusion/Schedulers/DDIMScheduler.cs	Updates base class reference and namespace
src/NeuralNetworks/Diffusion/DiffusionModelBase.cs	Updates to use INoiseScheduler interface and new namespace
src/NeuralNetworks/Diffusion/DDPMModel.cs	Updates to use INoiseScheduler interface and new namespace
src/Interfaces/INoiseScheduler.cs	Renames from IStepScheduler and updates namespace; adds documentation explaining the rename
src/Interfaces/IDiffusionModel.cs	Updates interface reference from IStepScheduler to INoiseScheduler

Comments suppressed due to low confidence (1)

src/NeuralNetworks/Diffusion/Schedulers/SchedulerConfig.cs:3

• The namespace change from AiDotNet.Diffusion to AiDotNet.NeuralNetworks.Diffusion and the interface rename from IStepScheduler to INoiseScheduler are breaking changes that will cause compilation errors in existing test files. Multiple test files in the repository still reference the old namespaces and interface names (e.g., tests/AiDotNet.Tests/UnitTests/Diffusion/Models/DDPMModelTests.cs, tests/AiDotNet.Tests/IntegrationTests/Diffusion/DiffusionSchedulersIntegrationTests.cs, and others). These test files must be updated as part of this PR to use the new namespaces and interface names to ensure the build passes.

---

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

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/Optimizers/GradientBasedOptimizerBase.cs (1)

1032-1050: Improved warmup detection logic.

The explicit LinearWarmupScheduler type check with WarmupSteps property is a robust solution that addresses the previous review concern about the flawed heuristic. Returning false for other schedulers is a safe default that prevents incorrect behavior with WarmupThenEpoch mode.

For future extensibility, consider defining an IWarmupScheduler interface with WarmupSteps property if more scheduler types need warmup support.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 763318c and dcd6aa4.

📒 Files selected for processing (6)

• src/Optimizers/GradientBasedOptimizerBase.cs
• tests/AiDotNet.Tests/IntegrationTests/Diffusion/DiffusionSchedulersIntegrationTests.cs
• tests/AiDotNet.Tests/UnitTests/Diffusion/Models/DDPMModelTests.cs
• tests/AiDotNet.Tests/UnitTests/Diffusion/Schedulers/DDIMSchedulerTests.cs
• tests/AiDotNet.Tests/UnitTests/Diffusion/Schedulers/PNDMSchedulerTests.cs
• tests/AiDotNet.Tests/UnitTests/Diffusion/Schedulers/SchedulerConfigTests.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:

• tests/AiDotNet.Tests/UnitTests/Diffusion/Schedulers/PNDMSchedulerTests.cs
• tests/AiDotNet.Tests/UnitTests/Diffusion/Schedulers/SchedulerConfigTests.cs
• tests/AiDotNet.Tests/UnitTests/Diffusion/Models/DDPMModelTests.cs
• tests/AiDotNet.Tests/IntegrationTests/Diffusion/DiffusionSchedulersIntegrationTests.cs
• tests/AiDotNet.Tests/UnitTests/Diffusion/Schedulers/DDIMSchedulerTests.cs
• src/Optimizers/GradientBasedOptimizerBase.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/Diffusion/Schedulers/PNDMSchedulerTests.cs
• tests/AiDotNet.Tests/UnitTests/Diffusion/Schedulers/SchedulerConfigTests.cs
• tests/AiDotNet.Tests/UnitTests/Diffusion/Models/DDPMModelTests.cs
• tests/AiDotNet.Tests/IntegrationTests/Diffusion/DiffusionSchedulersIntegrationTests.cs
• tests/AiDotNet.Tests/UnitTests/Diffusion/Schedulers/DDIMSchedulerTests.cs
• src/Optimizers/GradientBasedOptimizerBase.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:

• tests/AiDotNet.Tests/UnitTests/Diffusion/Schedulers/SchedulerConfigTests.cs
• tests/AiDotNet.Tests/UnitTests/Diffusion/Models/DDPMModelTests.cs
• tests/AiDotNet.Tests/IntegrationTests/Diffusion/DiffusionSchedulersIntegrationTests.cs
• tests/AiDotNet.Tests/UnitTests/Diffusion/Schedulers/DDIMSchedulerTests.cs
• src/Optimizers/GradientBasedOptimizerBase.cs

🔇 Additional comments (11)

src/Optimizers/GradientBasedOptimizerBase.cs (6)

91-123: Well-structured scheduler field declarations.

The new fields for scheduler integration are properly documented with clear XML comments explaining their purpose. The protected visibility allows subclasses to access scheduler state when needed.

---

162-169: LGTM - Clean scheduler initialization.

The scheduler initialization correctly uses the null-coalescing operator to set the learning rate from the scheduler if available, otherwise falling back to the initial rate. This addresses the previous review feedback about redundant assignments.

---

906-919: LGTM - Reset properly clears scheduler state.

The reset logic correctly handles all scheduler-related state: resets the scheduler itself, clears step/epoch counters, and restores the initial learning rate using the same pattern as initialization.

---

935-942: LGTM - StepScheduler correctly advances scheduler state.

The implementation properly guards against null scheduler and updates the internal learning rate field after stepping.

---

944-1012: LGTM - Clear documentation and correct stepping logic.

The documentation now clearly indicates when these methods must be called manually in the training loop. The SchedulerStepMode switch logic correctly handles all three modes:

• StepPerEpoch: steps only on epoch end
• StepPerBatch: steps only on batch end
• WarmupThenEpoch: steps per batch during warmup, per epoch after warmup

---

1052-1074: LGTM - Clean accessor implementations.

The GetCurrentLearningRate() method and CurrentStep/CurrentEpoch properties provide appropriate read-only access to scheduler state for monitoring and debugging purposes.

tests/AiDotNet.Tests/UnitTests/Diffusion/Models/DDPMModelTests.cs (1)

1-2: LGTM - Namespace updated to reflect diffusion module reorganization.

The using directives correctly reference the new AiDotNet.NeuralNetworks.Diffusion namespace structure following the refactor from AiDotNet.Diffusion.

tests/AiDotNet.Tests/UnitTests/Diffusion/Schedulers/DDIMSchedulerTests.cs (1)

1-1: LGTM - Namespace updated for scheduler refactor.

The using directive correctly references AiDotNet.NeuralNetworks.Diffusion.Schedulers to align with the diffusion module reorganization.

tests/AiDotNet.Tests/IntegrationTests/Diffusion/DiffusionSchedulersIntegrationTests.cs (1)

2-2: LGTM - Namespace updated for scheduler refactor.

The using directive correctly references AiDotNet.NeuralNetworks.Diffusion.Schedulers consistent with other test files.

tests/AiDotNet.Tests/UnitTests/Diffusion/Schedulers/PNDMSchedulerTests.cs (1)

1-1: LGTM - Namespace updated for scheduler refactor.

The using directive correctly references AiDotNet.NeuralNetworks.Diffusion.Schedulers.

tests/AiDotNet.Tests/UnitTests/Diffusion/Schedulers/SchedulerConfigTests.cs (1)

1-1: LGTM - Namespace updated for scheduler refactor.

The using directive correctly references AiDotNet.NeuralNetworks.Diffusion.Schedulers, completing the consistent namespace updates across all diffusion-related test files.

[coderabbitai]

Actionable comments posted: 0

Caution

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

⚠️ Outside diff range comments (2)

tests/AiDotNet.Tests/UnitTests/Optimizers/AdamWOptimizerTests.cs (1)

6-7: Remove duplicate using directive.

Line 7 duplicates the using directive on line 6.

🔎 Proposed fix

 using AiDotNet.Tensors.LinearAlgebra;
-using AiDotNet.Tensors.LinearAlgebra;

src/Optimizers/AdamWOptimizer.cs (1)

29-39: Update example code in XML documentation.

The example code in the XML documentation still references LearningRate instead of InitialLearningRate.

🔎 Proposed fix

 /// <example>
 /// <code>
 /// var options = new AdamWOptimizerOptions&lt;float, Matrix&lt;float&gt;, Vector&lt;float&gt;&gt;
 /// {
-///     LearningRate = 0.001,
+///     InitialLearningRate = 0.001,
 ///     WeightDecay = 0.01,
 ///     Beta1 = 0.9,
 ///     Beta2 = 0.999
 /// };
 /// var optimizer = new AdamWOptimizer&lt;float, Matrix&lt;float&gt;, Vector&lt;float&gt;&gt;(model, options);
 /// </code>
 /// </example>

🧹 Nitpick comments (3)

tests/AiDotNet.Tests/IntegrationTests/Optimizers/OptimizerSchedulerIntegrationTests.cs (3)

222-281: Consider adding a test for WarmupThenEpoch mode.

The PR objectives mention SchedulerStepMode.WarmupThenEpoch as one of the three modes. Tests cover StepPerBatch and StepPerEpoch, but WarmupThenEpoch (which should step per-batch during warmup, then per-epoch) is not exercised.

Would you like me to generate a test case for WarmupThenEpoch mode?

---

323-348: Test doesn't verify the constant learning rate behavior.

The test name Optimizer_WithNullScheduler_UsesConstantLearningRate implies verification that LR stays constant, but the assertions only check that parameters are updated. Consider adding an assertion that GetCurrentLearningRate() returns the initial value after all iterations.

🔎 Proposed fix to add LR verification

         // Assert - Optimizer should still work and update parameters
         Assert.True(parameters[0] < initialParams[0]);
         Assert.True(parameters[1] < initialParams[1]);
         Assert.True(parameters[2] < initialParams[2]);
+        
+        // LR should remain constant without a scheduler
+        Assert.Equal(0.01, optimizer.GetCurrentLearningRate(), Tolerance);

---

354-448: Consider consolidating duplicate test logic with parameterized tests.

These three tests (AdamOptimizer_WorksWithStepScheduler, AdamWOptimizer_WorksWithStepScheduler, LionOptimizer_WorksWithStepScheduler) share nearly identical structure. Using xUnit's [Theory] with a factory pattern or [MemberData] could reduce duplication while maintaining test isolation.

That said, the explicit tests are clear and maintainable as-is—this is a nice-to-have refinement.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dcd6aa4 and 6b3c3cc.

📒 Files selected for processing (27)

• src/Models/Options/AMSGradOptimizerOptions.cs
• src/Models/Options/AdaMaxOptimizerOptions.cs
• src/Models/Options/AdamOptimizerOptions.cs
• src/Models/Options/AdamWOptimizerOptions.cs
• src/Models/Options/GradientBasedOptimizerOptions.cs
• src/Models/Options/LionOptimizerOptions.cs
• src/Models/Options/MultilayerPerceptronRegressionOptions.cs
• src/Models/Options/NadamOptimizerOptions.cs
• src/Optimizers/AMSGradOptimizer.cs
• src/Optimizers/AdaMaxOptimizer.cs
• src/Optimizers/AdamOptimizer.cs
• src/Optimizers/AdamWOptimizer.cs
• src/Optimizers/GradientBasedOptimizerBase.cs
• src/Optimizers/LionOptimizer.cs
• src/Optimizers/NadamOptimizer.cs
• src/Regression/MultilayerPerceptronRegression.cs
• src/Regression/NeuralNetworkRegression.cs
• src/ReinforcementLearning/Agents/A3CAgent.cs
• src/ReinforcementLearning/Agents/DecisionTransformerAgent.cs
• src/ReinforcementLearning/Agents/DreamerAgent.cs
• src/ReinforcementLearning/Agents/MADDPGAgent.cs
• src/ReinforcementLearning/Agents/QMIXAgent.cs
• src/ReinforcementLearning/Agents/RainbowDQNAgent.cs
• src/ReinforcementLearning/Agents/TRPOAgent.cs
• tests/AiDotNet.Tests/IntegrationTests/Optimizers/OptimizerSchedulerIntegrationTests.cs
• tests/AiDotNet.Tests/UnitTests/Optimizers/AdamWOptimizerTests.cs
• tests/AiDotNet.Tests/UnitTests/Optimizers/LionOptimizerTests.cs

✅ Files skipped from review due to trivial changes (1)

• src/Optimizers/AdamOptimizer.cs

🚧 Files skipped from review as they are similar to previous changes (1)

• src/Optimizers/GradientBasedOptimizerBase.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/ReinforcementLearning/Agents/DreamerAgent.cs
• src/Optimizers/AMSGradOptimizer.cs
• src/Optimizers/NadamOptimizer.cs
• src/ReinforcementLearning/Agents/DecisionTransformerAgent.cs
• src/Models/Options/AdamOptimizerOptions.cs
• src/Optimizers/AdaMaxOptimizer.cs
• src/ReinforcementLearning/Agents/TRPOAgent.cs
• src/Regression/MultilayerPerceptronRegression.cs
• src/Regression/NeuralNetworkRegression.cs
• src/Models/Options/NadamOptimizerOptions.cs
• tests/AiDotNet.Tests/IntegrationTests/Optimizers/OptimizerSchedulerIntegrationTests.cs
• src/Models/Options/LionOptimizerOptions.cs
• src/ReinforcementLearning/Agents/A3CAgent.cs
• src/Optimizers/LionOptimizer.cs
• src/Optimizers/AdamWOptimizer.cs
• src/ReinforcementLearning/Agents/MADDPGAgent.cs
• tests/AiDotNet.Tests/UnitTests/Optimizers/AdamWOptimizerTests.cs
• src/ReinforcementLearning/Agents/QMIXAgent.cs
• src/ReinforcementLearning/Agents/RainbowDQNAgent.cs
• src/Models/Options/AMSGradOptimizerOptions.cs
• src/Models/Options/MultilayerPerceptronRegressionOptions.cs
• tests/AiDotNet.Tests/UnitTests/Optimizers/LionOptimizerTests.cs
• src/Models/Options/AdaMaxOptimizerOptions.cs
• src/Models/Options/AdamWOptimizerOptions.cs
• src/Models/Options/GradientBasedOptimizerOptions.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/ReinforcementLearning/Agents/DreamerAgent.cs
• src/Optimizers/AMSGradOptimizer.cs
• src/Optimizers/NadamOptimizer.cs
• src/ReinforcementLearning/Agents/DecisionTransformerAgent.cs
• src/Models/Options/AdamOptimizerOptions.cs
• src/Optimizers/AdaMaxOptimizer.cs
• src/ReinforcementLearning/Agents/TRPOAgent.cs
• src/Regression/MultilayerPerceptronRegression.cs
• src/Regression/NeuralNetworkRegression.cs
• src/Models/Options/NadamOptimizerOptions.cs
• tests/AiDotNet.Tests/IntegrationTests/Optimizers/OptimizerSchedulerIntegrationTests.cs
• src/Models/Options/LionOptimizerOptions.cs
• src/ReinforcementLearning/Agents/A3CAgent.cs
• src/Optimizers/LionOptimizer.cs
• src/Optimizers/AdamWOptimizer.cs
• src/ReinforcementLearning/Agents/MADDPGAgent.cs
• tests/AiDotNet.Tests/UnitTests/Optimizers/AdamWOptimizerTests.cs
• src/ReinforcementLearning/Agents/QMIXAgent.cs
• src/ReinforcementLearning/Agents/RainbowDQNAgent.cs
• src/Models/Options/AMSGradOptimizerOptions.cs
• src/Models/Options/MultilayerPerceptronRegressionOptions.cs
• tests/AiDotNet.Tests/UnitTests/Optimizers/LionOptimizerTests.cs
• src/Models/Options/AdaMaxOptimizerOptions.cs
• src/Models/Options/AdamWOptimizerOptions.cs
• src/Models/Options/GradientBasedOptimizerOptions.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/Models/Options/GradientBasedOptimizerOptions.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 (37)

src/Optimizers/NadamOptimizer.cs (1)

94-96: LGTM! Clean refactor to centralize learning rate initialization.

The removal of the explicit CurrentLearningRate assignment is correct. The base class now handles learning rate initialization from options.InitialLearningRate, which aligns with the broader scheduler integration effort.

src/Models/Options/AdamWOptimizerOptions.cs (1)

35-43: LGTM! Public API update aligns with scheduler integration.

The rename from LearningRate to InitialLearningRate with the new modifier correctly reflects the semantic change—this property now specifies the initial rate before any scheduler adjustments. The default value of 0.001 remains appropriate for AdamW.

src/ReinforcementLearning/Agents/TRPOAgent.cs (1)

56-62: LGTM! Consistent API usage update.

The property name change from LearningRate to InitialLearningRate correctly reflects the updated optimizer options API.

src/Optimizers/AdaMaxOptimizer.cs (1)

149-154: LGTM! Consistent refactor to centralize learning rate handling.

The removal of explicit CurrentLearningRate assignment is correct and consistent with other optimizers in this PR. The base class now manages initial learning rate setup from options.InitialLearningRate.

src/ReinforcementLearning/Agents/DecisionTransformerAgent.cs (1)

55-61: LGTM! Consistent API usage update.

The property rename to InitialLearningRate correctly reflects the updated AdamOptimizerOptions API.

src/ReinforcementLearning/Agents/RainbowDQNAgent.cs (1)

63-69: LGTM! Consistent API usage update.

The property rename to InitialLearningRate correctly reflects the updated AdamOptimizerOptions API. The learning rate of 0.0001 remains appropriate for Rainbow DQN.

src/Regression/NeuralNetworkRegression.cs (1)

86-92: LGTM! Consistent API usage update.

The property rename to InitialLearningRate correctly reflects the updated AdamOptimizerOptions API. The default learning rate of 0.001 remains appropriate for neural network regression.

src/ReinforcementLearning/Agents/DreamerAgent.cs (1)

66-72: Verify property reference consistency between line 68 and lines 305/334.

Line 68 correctly uses InitialLearningRate for the Adam optimizer options. However, lines 305 and 334 still reference _options.LearningRate. Confirm whether DreamerOptions<T> exposes a separate LearningRate property distinct from the optimizer's InitialLearningRate, or if these references should be updated to match the line 68 pattern.

src/ReinforcementLearning/Agents/A3CAgent.cs (1)

55-55: LGTM! Property rename aligned with PR objectives.

The change from LearningRate to InitialLearningRate is consistent with the repository-wide refactoring to support learning rate schedulers. The default value of 0.001 is preserved.

src/Regression/MultilayerPerceptronRegression.cs (1)

149-149: LGTM! Consistent property rename.

The change to InitialLearningRate maintains consistency with the new optimizer options API while preserving the default learning rate value.

src/Optimizers/AMSGradOptimizer.cs (1)

76-77: LGTM! Base class now handles learning rate initialization.

The refactoring to remove direct CurrentLearningRate assignment and delegate to the base class InitializeAdaptiveParameters() centralizes learning rate management and enables scheduler integration. The explanatory comment clearly documents this change.

src/ReinforcementLearning/Agents/QMIXAgent.cs (1)

63-63: LGTM! Property rename consistent across agents.

The InitialLearningRate property usage aligns with the optimizer options API changes throughout the codebase.

src/Models/Options/MultilayerPerceptronRegressionOptions.cs (1)

446-446: LGTM! Optimizer factory updated correctly.

The property rename in the default optimizer factory is consistent with the new InitialLearningRate API across all optimizer options.

src/Models/Options/AdaMaxOptimizerOptions.cs (1)

38-38: LGTM! Property declaration updated with appropriate modifier.

The new modifier correctly hides the base class member to provide AdaMax's specific default value (0.002). The property rename from LearningRate to InitialLearningRate aligns with the scheduler integration architecture.

src/Models/Options/AMSGradOptimizerOptions.cs (1)

45-45: LGTM! Property and documentation updated correctly.

The property declaration with the new modifier and the updated documentation referencing "initial step size" properly reflect the scheduler-based learning rate architecture.

src/Models/Options/LionOptimizerOptions.cs (1)

31-41: LGTM! Documentation and property declaration updated correctly.

Both the XML documentation (line 31) and property declaration (line 41) properly reference "initial learning rate". The new modifier preserves Lion's optimizer-specific default value of 1e-4, which is appropriate for sign-based updates.

src/Models/Options/NadamOptimizerOptions.cs (1)

84-84: LGTM!

The property rename from LearningRate to InitialLearningRate with the new modifier correctly shadows the base class property with Nadam's recommended default of 0.002, aligning with the broader refactor across optimizer options.

tests/AiDotNet.Tests/UnitTests/Optimizers/AdamWOptimizerTests.cs (1)

28-29: LGTM!

Tests correctly updated to validate InitialLearningRate instead of LearningRate, ensuring the property rename is properly tested across default options, custom options, and serialization scenarios.

src/Models/Options/AdamOptimizerOptions.cs (1)

38-48: LGTM!

The property rename to InitialLearningRate with the standard Adam default of 0.001 is consistent with the broader refactor. Documentation correctly describes the property as the "initial learning rate."

tests/AiDotNet.Tests/UnitTests/Optimizers/LionOptimizerTests.cs (1)

26-29: LGTM!

Tests correctly validate InitialLearningRate with Lion's recommended default of 1e-4 (lower than Adam/AdamW due to Lion's sign-based updates). All test scenarios properly reference the renamed property.

src/ReinforcementLearning/Agents/MADDPGAgent.cs (1)

65-71: LGTM!

The fallback Adam optimizer correctly uses InitialLearningRate instead of the old LearningRate property, aligning with the renamed property in AdamOptimizerOptions.

src/Optimizers/LionOptimizer.cs (3)

85-94: LGTM!

The removal of the private learning rate field and delegation to the base class CurrentLearningRate property correctly enables scheduler-controlled learning rates. The comment clearly documents this design decision.

---

465-470: Good: Reset() now properly calls base.Reset().

This ensures the base class's learning rate state (including scheduler-related counters like _currentStep and _currentEpoch) is properly reset alongside Lion's momentum state.

---

608-612: LGTM!

Using _options.InitialLearningRate for the cache key correctly identifies the optimizer configuration, as this is the configured starting point rather than the dynamic CurrentLearningRate.

src/Models/Options/GradientBasedOptimizerOptions.cs (2)

2-2: LGTM!

The using directive for AiDotNet.LearningRateSchedulers is required for the new ILearningRateScheduler and SchedulerStepMode types.

---

205-238: Well-documented scheduler integration properties.

The new LearningRateScheduler and SchedulerStepMode properties provide a clean API for scheduler integration:

• LearningRateScheduler defaults to null (constant learning rate), which maintains backward compatibility.
• SchedulerStepMode defaults to StepPerEpoch, the most common scheduler stepping behavior.
• Documentation clearly explains all three stepping modes and their use cases.

src/Optimizers/AdamWOptimizer.cs (3)

120-125: LGTM!

The removal of the private learning rate field and delegation to the base class CurrentLearningRate property correctly enables scheduler-controlled learning rates, consistent with the LionOptimizer refactor.

---

557-564: Good: Reset() now properly calls base.Reset().

This ensures the base class's learning rate state (including scheduler counters) is properly reset alongside AdamW's moment vectors.

---

697-701: LGTM!

Using _options.InitialLearningRate for the cache key correctly identifies the optimizer configuration, consistent with the LionOptimizer implementation.

tests/AiDotNet.Tests/IntegrationTests/Optimizers/OptimizerSchedulerIntegrationTests.cs (8)

1-17: LGTM!

The imports, namespace, and class setup are well-structured. The tolerance constant of 1e-6 is appropriate for double precision floating-point comparisons in these scheduler tests. Based on learnings, global usings cover Vector<T> and related types.

---

21-124: LGTM!

The AdamW scheduler integration tests are well-designed:

• StepLR decay test correctly verifies gamma-based decay after stepSize epochs.
• Cosine annealing test properly verifies monotonic decrease with tolerance handling.
• Linear warmup test correctly verifies LR increases during warmup and reaches base LR at completion.

---

130-183: LGTM!

Both Adam scheduler tests correctly verify expected decay behaviors with precise mathematical assertions.

---

189-218: LGTM!

The Lion optimizer test uses appropriately small learning rate values typical for Lion and correctly verifies cosine annealing behavior.

---

285-317: LGTM!

The reset test properly verifies that Reset() restores the learning rate to its initial value after decay.

---

454-491: LGTM!

The OneCycle scheduler test properly verifies the characteristic warmup-peak-cooldown learning rate trajectory.

---

497-529: LGTM!

The cyclic scheduler test correctly verifies that the learning rate varies significantly through the cycle, which is the key characteristic of cyclic LR scheduling.

---

535-602: LGTM!

The GetCurrentLearningRate, OnEpochEnd, and OnBatchEnd tests effectively verify the scheduler lifecycle hooks and their interaction with the respective SchedulerStepMode settings.

In general, to fix “cast to same type” issues, you delete the explicit cast when the compiler already knows the expression has the desired type. This avoids redundant syntax and potential confusion without changing runtime behavior.

For this specific case in src/Optimizers/AdamOptimizer.cs, you should remove the (Vector<T>) cast from the assignment on line 261 so that update is directly initialized from the result of Engine.Multiply(updateDiv, CurrentLearningRate). Since CodeQL has determined that this expression already has type Vector<T>, the variable declaration var update will still infer the same type, and no further changes are needed. No new methods, imports, or definitions are required, and no other lines in this snippet need modification.

In general, the fix for a "cast to same type" issue is simply to remove the unnecessary cast, leaving the expression as-is. This removes noise without changing semantics, since the compiler already knows the expression’s type is the same as the cast target.

For this specific case in src/Optimizers/AdamOptimizer.cs, on line 342 the code reads:

var scaledUpdate = (Vector<T>)Engine.Multiply(update, CurrentLearningRate);

Since Engine.Multiply(update, CurrentLearningRate) already returns Vector<T>, we should remove the explicit cast and keep the inferred var type, preserving existing behavior:

var scaledUpdate = Engine.Multiply(update, CurrentLearningRate);

No additional methods, imports, or definitions are needed. Only this line inside the shown method body needs to be changed.

To fix this, remove the redundant cast so that the code uses the value directly as the Vector<T> it already is. This keeps the same functionality while simplifying the code and addressing the CodeQL warning.

Concretely, in src/Optimizers/AdamOptimizer.cs around line 419, change:

var scaledUpdate = (Vector<T>)Engine.Multiply(update, CurrentLearningRate);

to:

var scaledUpdate = Engine.Multiply(update, CurrentLearningRate);

No additional imports, methods, or definitions are needed, as we are only removing an unnecessary cast and not changing types or behavior.

In general, to fix “cast to same type” issues, you remove explicit casts when the compiler and type system already ensure the expression has the desired type. This simplifies the code and avoids misleading readers into thinking a conversion is needed.

For this concrete case in src/Optimizers/AdamWOptimizer.cs, line 285 defines:

var scaledAdamUpdate = (Vector<T>)Engine.Multiply(adamUpdate, CurrentLearningRate);

CodeQL and the compiler understand that Engine.Multiply(adamUpdate, CurrentLearningRate) already returns a Vector<T>, so the (Vector<T>) cast is redundant. The best fix, without changing any behavior, is to remove the cast and let type inference handle it:

var scaledAdamUpdate = Engine.Multiply(adamUpdate, CurrentLearningRate);

No new methods, imports, or definitions are needed; we are only simplifying an existing assignment. All other lines remain unchanged.

To fix this, remove the unnecessary cast to Vector<T> on the result of Engine.Multiply(weightDecayTerm, CurrentLearningRate) and let the expression’s natural type be used. This preserves existing functionality because the expression is already of type Vector<T>; the cast adds no runtime behavior.

Concretely, in src/Optimizers/AdamWOptimizer.cs in the method where weight decay is applied (around line 290), change:

var scaledWeightDecay = (Vector<T>)Engine.Multiply(weightDecayTerm, CurrentLearningRate);

to:

var scaledWeightDecay = Engine.Multiply(weightDecayTerm, CurrentLearningRate);

No additional imports, methods, or definitions are needed, and no other lines in this file need to change for this specific issue.

In general, to fix a “cast to same type” issue, you remove the explicit cast when the compiler and static analyzer agree that the expression already has the desired type. This reduces clutter and eliminates misleading or unnecessary type noise.

Here, on line 481 in src/Optimizers/AdamWOptimizer.cs, CodeQL reports that (Vector<T>)Engine.Multiply(weightDecayTerm, CurrentLearningRate) is redundant because Engine.Multiply(weightDecayTerm, CurrentLearningRate) already returns Vector<T>. The best fix is to delete the cast and assign the result directly to var scaledWeightDecay. No behavior changes, and no additional imports or methods are required.

Concretely:

• In the method containing lines 479–485 (the update computation), change line 481 from:
  • var scaledWeightDecay = (Vector<T>)Engine.Multiply(weightDecayTerm, CurrentLearningRate);
    to:
  • var scaledWeightDecay = Engine.Multiply(weightDecayTerm, CurrentLearningRate);

No other lines need to be adjusted, since var will still infer Vector<T> based on the method’s return type.

In general, a "useless assignment to local variable" should be fixed either by removing the assignment (and the variable) if it is truly unnecessary, or by using the variable in place of repeated expressions if the intent was to cache or clarify a value. The key is to ensure that either the assignment has an observable effect (via subsequent reads) or it is not present at all.

For this specific case in src/Optimizers/LionOptimizer.cs, inside UpdateSolution, the variable effectiveLearningRate declared on line 229 is not used anywhere. The right-hand side CurrentLearningRate is a simple property access without side effects. The best, smallest fix that does not alter existing functionality is to remove the declaration/assignment line:

var effectiveLearningRate = CurrentLearningRate;

and leave the remaining code unchanged. No additional imports, methods, or definitions are needed, and no other lines in this method need to be updated because CurrentLearningRate is already used directly where required.

In general, a “cast to same type” problem is fixed by removing the unnecessary cast and letting the compiler use the expression’s inferred type. This simplifies the code and removes misleading type information.

Specifically for this case in src/Optimizers/LionOptimizer.cs, at line 249, the expression:

var lrTimesUpdate = (Vector<T>)Engine.Multiply(update, CurrentLearningRate);

should be changed to:

var lrTimesUpdate = Engine.Multiply(update, CurrentLearningRate);

No additional imports or helper methods are needed, since the method already returns the correct type (Vector<T> as per CodeQL’s inference). This change preserves existing functionality while cleaning up the redundant cast. All other lines remain as they are.

In general, a redundant cast to the same type should be removed so that the expression is left in its original form. This keeps the code clearer, avoids confusion about types, and prevents unnecessary clutter.

In this method, at line 303 inside LionOptimizer<T, TInput, TOutput>, the expression Engine.Multiply(update, CurrentLearningRate) is already of type Vector<T> (per CodeQL), so casting it to (Vector<T>) before assigning it to lrTimesUpdate is unnecessary. To fix this without changing behavior, we simply remove the cast and assign the result directly. No imports, method changes, or additional definitions are required.

Concretely, in src/Optimizers/LionOptimizer.cs, in the body of the method where _m and updatedParams are handled (the vectorized Lion update), change line 303 from:

var lrTimesUpdate = (Vector<T>)Engine.Multiply(update, CurrentLearningRate);

to:

var lrTimesUpdate = Engine.Multiply(update, CurrentLearningRate);

All other casts in the shown snippet should remain unchanged, since CodeQL has only flagged this specific one as redundant and we must not assume the types of other Engine operations.

General fix: For a “cast to same type” issue, delete the explicit cast and let the expression’s existing type stand. This removes noise without affecting functionality.

Best fix here: On line 443 in src/Optimizers/LionOptimizer.cs, Engine.Multiply(update, CurrentLearningRate) already returns Vector<T>, so we can assign it directly to lrTimesUpdate without casting. We will replace:

var lrTimesUpdate = (Vector<T>)Engine.Multiply(update, CurrentLearningRate);

with:

var lrTimesUpdate = Engine.Multiply(update, CurrentLearningRate);

No other changes, imports, or helper methods are required. All other casts in the shown snippet should remain untouched because the static analyzer did not flag them; they may be necessary if those particular Engine methods return a more general type.

[sonarqubecloud]

Quality Gate failed

Failed conditions
61.4% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud