Repository navigation
feat: dataloader batching utilities - #570
Conversation
- Add GetBatches() with yield return for lazy evaluation - Add GetBatchesAsync() with Channel-based prefetching - Implement in InputOutputDataLoaderBase, EpisodicDataLoaderBase, GraphDataLoaderBase, and RLDataLoaderBase - Support shuffle, dropLast, seed, and prefetchCount parameters - Use net471-compatible channel reading pattern Part of #443 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add DataLoaderExtensions with CreateBatches/CreateBatchesAsync - Add BatchConfigurationBuilder for fluent sync iteration - Add AsyncBatchConfigurationBuilder for fluent async iteration - Support Shuffled(), NoShuffle(), DropLast(), KeepLast(), WithSeed() Part of #443 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add IDataSampler, IBatchSampler, IWeightedSampler, IStratifiedSampler - Add RandomSampler with Fisher-Yates shuffle - Add SequentialSampler for deterministic iteration - Add SubsetSampler for working with data subsets - Add StratifiedSampler to maintain class distribution - Add StratifiedBatchSampler for balanced batches - Add WeightedSampler with inverse transform sampling - Include CreateBalancedWeights helper for class imbalance Part of #443 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add CurriculumSampler with Linear, Exponential, Stepped strategies - Add SelfPacedSampler for adaptive curriculum learning - Add ImportanceSampler with variance reduction and correction factors - Add ActiveLearningSampler for uncertainty/diversity-based selection - Support competence-based curriculum and hybrid active learning Part of #443 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Implements Phase 3B of DataLoader batching utilities: - ParallelBatchLoader<TBatch> with configurable workers and prefetch - Uses ConcurrentQueue for work distribution - Uses Channel for bounded output queue - ParallelBatchLoaderConfig for advanced settings - WithParallelLoading() extension method for IBatchIterable - net471 compatible (no ObjectDisposedException.ThrowIf) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Implements Phase 4A of DataLoader batching utilities: - StreamingDataLoader<TInput, TOutput> for async sample loading - FileStreamingDataLoader for directory-based datasets - CsvStreamingDataLoader for line-by-line CSV processing - MemoryMappedStreamingDataLoader for efficient random access - All implement full IBatchIterable interface - Supports parallel sample loading with configurable workers - net471 compatible 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Implements Phase 4B of DataLoader batching utilities: - DataPipeline<T> with fluent API for data transformations - Map, Filter, Cache, Shuffle, Batch, Take, Skip, Repeat operators - Concat, Interleave, Zip, Flatten for combining pipelines - Prefetch with Channel-based async prefetching - AsyncDataPipeline<T> for async operations - Extension methods: ToPipeline, PaddedBatch, Window, Sample - Uses IAsyncEnumerableProvider pattern for proper async iterators - net471 compatible 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- add istreamingdataloader interface for streaming loaders - add streamingdataloaderbase for common streaming functionality - add streaming factory methods to dataloaders facade - add samplers facade with beginner-friendly factory methods - add streaming data path in predictionmodelbuilder.buildasync() - add pipeline extension methods for istreamingdataloader - make optimizationdatabatcher internal with proper row selection 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Warning Rate limit exceeded@ooples has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 6 minutes and 17 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 (1)
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 DataLoader redesign: new streaming loader base and concrete streaming loaders, batch-iteration APIs (sync + async) on multiple loader bases, parallel batch loader, a sampling framework (interfaces, bases, many samplers, factory), DataPipeline (sync/async), optimizer integration with batchers and epoch notifications, fluent batch builders, aggregation/tensor helpers, benchmarks and unit tests. Some duplicate/overlapping regions and duplicate factory declarations were introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant Trainer as Trainer (Consumer)
participant Loader as StreamingDataLoaderBase
participant Producer as Producer Task
participant Worker as Worker Task (N)
participant Channel as Bounded Channel
Trainer->>Loader: GetBatchesAsync(shuffle, dropLast, seed)
Loader->>Channel: create bounded channel(prefetchCount)
Loader->>Producer: start producer task
loop per batchIndices
Producer->>Loader: GetShuffledIndices(...)
note right of Loader: compute indices (seeded Fisher‑Yates)
loop per sampleIndex in batch
Producer->>Worker: Task.Run(ReadSampleAsync(index))
Worker->>Loader: ReadSampleAsync(index)
Worker-->>Producer: (Input, Output)
end
Producer->>Loader: AggregateSamples(samples)
Producer->>Channel: WriteAsync(batchedSamples)
end
Producer->>Channel: Complete()
loop consumer
Trainer->>Channel: ReadAsync()
Channel-->>Trainer: batchedSamples
Trainer->>Trainer: training step
end
rect rgba(200,220,240,0.18)
note over Channel,Producer: Bounded channel enforces backpressure / prefetch
end
sequenceDiagram
participant Requester as Consumer
participant PBL as ParallelBatchLoader
participant Indexer as Index Provider
participant WorkerA as Worker 0
participant WorkerB as Worker 1
participant Channel as Bounded Channel
Requester->>PBL: GetBatchesAsync()
PBL->>Indexer: Precompute batch index list
PBL->>Channel: create bounded channel(prefetch)
par parallel workers
WorkerA->>Indexer: Dequeue batch indices
WorkerA->>PBL: batchFactory(indices)
WorkerA->>Channel: WriteAsync(batch)
and
WorkerB->>Indexer: Dequeue batch indices
WorkerB->>PBL: batchFactory(indices)
WorkerB->>Channel: WriteAsync(batch)
end
Requester->>Channel: ReadAsync() loop -> receive batches
PBL->>Channel: Complete() when workers finish
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 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 |
![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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (18)
docs/design/DataLoader-Design-Decisions.md (1)
14-40: Clarify current “streaming” limitations in higher‑level training pathsThe design emphasizes support for datasets “too large to fit in memory” and lists
StreamingDataLoaderand pipeline operators as the primary mechanisms. However, the currentPredictionModelBuilder.BuildAsyncintegration buffers all streaming batches into a single aggregated feature/label structure before training, so end‑to‑end “train without ever materializing the full dataset” isn’t realized yet.Worth adding a short note in the Memory Model or Streaming Data Loaders sections that:
- Low‑level streaming loaders and pipelines are fully streaming.
- High‑level facades like
PredictionModelBuilderpresently materialize streaming data and will gain true streaming training support in a future phase.This keeps expectations aligned without changing the overall design.
Also applies to: 85-115, 118-149
src/Data/Loaders/GraphDataLoaderBase.cs (1)
456-470: Minor: Redundant empty check.The check at line 467 (
if (totalGraphs == 0)) is redundant since line 456 already checksLoadedGraphs.Count == 0and would have exited viayield breakat line 462.🔎 Suggested simplification
// Multi-graph dataset case int totalGraphs = LoadedGraphs.Count; - if (totalGraphs == 0) - { - yield break; - } // Create fresh indices for this iterationsrc/Interfaces/IDataSampler.cs (2)
98-126: Consider documenting invariants for settable properties.
IWeightedSampler<T>.WeightsandIWeightedSampler<T>.NumSamplesare settable, which could lead to invalid states if:
Weightscontains negative valuesNumSamples > LengthwhenReplacement = falseConsider adding validation guidance in the remarks or using init-only setters if C# 9+ is supported.
131-142:Labelssetter may causeNumClassesto become stale.If
Labelsis set after construction, theNumClassesproperty (which is read-only) may not reflect the new label distribution. Implementations should recalculateNumClasseswhenLabelschanges, but this isn't enforced by the interface.Consider making
Labelsinit-only or documenting thatNumClassesmust be recalculated whenLabelschanges.src/Data/Loaders/RLDataLoaderBase.cs (1)
383-391: Yielding individual experiences may be inefficient for RL training.RL training typically processes batches of experiences together for efficiency (vectorized operations). Yielding individual
Experienceobjects requires the caller to manually batch them:// Current: caller must re-batch var batch = loader.GetBatches().Take(batchSize).ToList();Consider adding an overload that yields
IReadOnlyList<Experience<...>>batches directly.src/Data/Sampling/RandomSampler.cs (1)
162-169: Consider validating that subset indices are non-negative.
SubsetSampleraccepts arbitrary indices but doesn't validate they are valid (non-negative). Negative indices would causeIndexOutOfRangeExceptionwhen used with a data loader.🔎 Suggested validation
public SubsetSampler(IEnumerable<int> indices, bool shuffle = false, int? seed = null) { _indices = indices?.ToArray() ?? throw new ArgumentNullException(nameof(indices)); + + if (_indices.Any(i => i < 0)) + { + throw new ArgumentOutOfRangeException(nameof(indices), "All indices must be non-negative."); + } + _shuffle = shuffle; _random = seed.HasValue ? RandomHelper.CreateSeededRandom(seed.Value) : RandomHelper.CreateSecureRandom(); }src/Data/Loaders/ParallelBatchLoader.cs (2)
176-180: Dispose does not cancel in-flight async operations.Setting
_disposed = trueprevents new iterations but doesn't cancel activeGetBatchesAsynccalls. Workers continue running until completion or garbage collection.Consider storing and cancelling a
CancellationTokenSourceon dispose, or documenting that callers must cancel via the token.
276-280: Expensive fallback for TotalCount.The fallback iterates all batches just to count them, which defeats the purpose of lazy evaluation and could be very expensive for large datasets.
Consider throwing
NotSupportedExceptionor returning a sentinel value instead of silently iterating.🔎 Alternative: Throw instead of expensive enumeration
// Fallback: count by iterating (expensive) - return source.GetBatches(shuffle: false).Count(); + throw new NotSupportedException( + $"The source {source.GetType().Name} does not implement ICountable. " + + "Implement ICountable or provide TotalCount explicitly.");src/Data/Sampling/StratifiedSampler.cs (1)
91-98: Out-of-range labels are silently ignored.Labels outside
[0, numClasses)are quietly skipped without warning. This could mask data corruption or off-by-one errors in label preparation.Consider logging a warning or throwing for unexpected labels.
🔎 Proposed: Add validation or warning
for (int i = 0; i < _labels.Length; i++) { int label = _labels[i]; if (label >= 0 && label < _numClasses) { _classIndices[label].Add(i); } + else + { + // Consider: throw new ArgumentException($"Label {label} at index {i} is outside valid range [0, {_numClasses})."); + // Or log warning if available + } }src/Data/Sampling/WeightedSampler.cs (2)
159-204: Sampling without replacement has O(n²) complexity.Each sample requires iterating all remaining elements to recompute the CDF. For large datasets, this becomes prohibitively slow.
Consider using the alias method with removal, or reservoir sampling for better performance.
165-165: Unused variable allocation.
currentProbabilitiesis allocated but never used. Remove to avoid confusion and unnecessary allocation.🔎 Proposed fix
var selected = new HashSet<int>(); var availableIndices = Enumerable.Range(0, _weights.Length).ToList(); - double[] currentProbabilities = new double[_weights.Length];src/Extensions/DataLoaderExtensions.cs (1)
91-97: Consider validating prefetchCount.Negative or zero
prefetchCountcould cause issues downstream. Add validation here or document that the source handles it.🔎 Proposed validation
public static AsyncBatchConfigurationBuilder<TBatch> CreateBatchesAsync<TBatch>( this IBatchIterable<TBatch> source, int? batchSize = null, int prefetchCount = 2) { + if (prefetchCount < 1) + { + throw new ArgumentOutOfRangeException(nameof(prefetchCount), "Prefetch count must be at least 1."); + } return new AsyncBatchConfigurationBuilder<TBatch>(source, batchSize, prefetchCount); }src/Data/Sampling/ImportanceSampler.cs (2)
218-247: Rejection sampling may fail for skewed distributions.With
maxAttempts = count * 10, highly skewed importance distributions could exhaust attempts before selecting enough unique samples. The fallback (sequential fill) loses importance-based selection.Consider using a proper without-replacement algorithm or increasing the multiplier with a warning.
398-403: LabeledCount/UnlabeledCount use LINQ Count() repeatedly.Each property access iterates the entire array. For frequent access (e.g., in loops), consider caching these counts.
🔎 Proposed: Cache counts
+ private int _labeledCount = 0; + public int LabeledCount => _labeledCount; - public int LabeledCount => _isLabeled.Count(x => x); - - public int UnlabeledCount => _isLabeled.Count(x => !x); + public int UnlabeledCount => _isLabeled.Length - _labeledCount; + + public void MarkAsLabeled(int index) + { + if (index >= 0 && index < _isLabeled.Length && !_isLabeled[index]) + { + _isLabeled[index] = true; + _labeledCount++; + } + }src/Data/Sampling/CurriculumSampler.cs (2)
127-127: CompetenceBased fallback in property is misleading.The
CurrentDifficultyThresholdproperty returnsprogressforCompetenceBased, butGetIndicescorrectly uses_competenceinstead (line 157-159). The property value is thus misleading for this strategy.Consider returning
_competencefor theCompetenceBasedcase.🔎 Proposed fix
return _strategy switch { CurriculumStrategy.Linear => progress, CurriculumStrategy.Exponential => Math.Pow(progress, 2), CurriculumStrategy.Stepped => GetSteppedThreshold(progress), - CurriculumStrategy.CompetenceBased => progress, // Override with SetCompetence() + CurriculumStrategy.CompetenceBased => _competence, _ => progress };
328-336: Hardcoded 10% minimum sample threshold.The
_losses.Length / 10floor is arbitrary and may not suit all use cases. Consider making this configurable via constructor parameter.src/Data/Loaders/StreamingDataLoaderBase.cs (1)
153-154: SingleWriter=true but producer uses parallel Task.Run.The channel is configured as
SingleWriter = true, but the producer task spawns multiple nested tasks that may write concurrently via the outerawait channel.Writer.WriteAsync. The write itself is serialized afterTask.WhenAll, so this is actually safe - the hint is just suboptimal.Consider
SingleWriter = falsefor clarity, or keep as-is since writes are serialized.src/Optimizers/OptimizationDataBatcher.cs (1)
224-261: Type checking with boxing may impact performance.The
ispattern matching with boxing (data is Matrix<T>) followed by casting back toTDataadds overhead. For hot paths with many batches, this could be noticeable.Consider using generic type constraints or a dispatch pattern if performance becomes an issue.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
docs/design/DataLoader-Design-Decisions.mdsrc/Data/Loaders/DataLoaders.cssrc/Data/Loaders/EpisodicDataLoaderBase.cssrc/Data/Loaders/GraphDataLoaderBase.cssrc/Data/Loaders/InputOutputDataLoaderBase.cssrc/Data/Loaders/ParallelBatchLoader.cssrc/Data/Loaders/RLDataLoaderBase.cssrc/Data/Loaders/StreamingDataLoader.cssrc/Data/Loaders/StreamingDataLoaderBase.cssrc/Data/Pipeline/DataPipeline.cssrc/Data/Sampling/CurriculumSampler.cssrc/Data/Sampling/ImportanceSampler.cssrc/Data/Sampling/RandomSampler.cssrc/Data/Sampling/Samplers.cssrc/Data/Sampling/StratifiedSampler.cssrc/Data/Sampling/WeightedSampler.cssrc/Extensions/DataLoaderExtensions.cssrc/Interfaces/IBatchIterable.cssrc/Interfaces/IDataSampler.cssrc/Interfaces/IStreamingDataLoader.cssrc/Optimizers/OptimizationDataBatcher.cssrc/PredictionModelBuilder.cs
🧰 Additional context used
🧠 Learnings (5)
📚 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/Interfaces/IBatchIterable.cssrc/Data/Loaders/EpisodicDataLoaderBase.cssrc/Data/Loaders/GraphDataLoaderBase.cssrc/Interfaces/IStreamingDataLoader.cssrc/Data/Loaders/InputOutputDataLoaderBase.cssrc/Extensions/DataLoaderExtensions.cssrc/PredictionModelBuilder.cssrc/Data/Sampling/ImportanceSampler.cssrc/Data/Sampling/CurriculumSampler.cssrc/Data/Sampling/RandomSampler.cssrc/Optimizers/OptimizationDataBatcher.cssrc/Data/Loaders/RLDataLoaderBase.cssrc/Data/Loaders/ParallelBatchLoader.cssrc/Data/Sampling/WeightedSampler.cssrc/Data/Loaders/StreamingDataLoaderBase.cssrc/Data/Sampling/StratifiedSampler.cssrc/Data/Pipeline/DataPipeline.cssrc/Data/Loaders/DataLoaders.cssrc/Data/Loaders/StreamingDataLoader.cssrc/Interfaces/IDataSampler.cssrc/Data/Sampling/Samplers.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/Interfaces/IBatchIterable.cssrc/Data/Loaders/EpisodicDataLoaderBase.cssrc/Data/Loaders/GraphDataLoaderBase.cssrc/Interfaces/IStreamingDataLoader.cssrc/Data/Loaders/InputOutputDataLoaderBase.cssrc/Extensions/DataLoaderExtensions.cssrc/PredictionModelBuilder.cssrc/Data/Sampling/ImportanceSampler.cssrc/Data/Sampling/CurriculumSampler.cssrc/Data/Sampling/RandomSampler.cssrc/Optimizers/OptimizationDataBatcher.cssrc/Data/Loaders/RLDataLoaderBase.cssrc/Data/Loaders/ParallelBatchLoader.cssrc/Data/Sampling/WeightedSampler.cssrc/Data/Loaders/StreamingDataLoaderBase.cssrc/Data/Sampling/StratifiedSampler.cssrc/Data/Pipeline/DataPipeline.cssrc/Data/Loaders/DataLoaders.cssrc/Data/Loaders/StreamingDataLoader.cssrc/Interfaces/IDataSampler.cssrc/Data/Sampling/Samplers.cs
📚 Learning: 2025-12-21T04:19:31.255Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 445
File: src/PredictionModelBuilder.cs:752-763
Timestamp: 2025-12-21T04:19:31.255Z
Learning: Federated preprocessing invariant: When using IFederatedClientDataLoader in PredictionModelBuilder.BuildSupervisedInternalAsync (src/PredictionModelBuilder.cs), any preprocessing must preserve per-client row ordering and total sample counts. The code now enforces X/Y alignment and total count checks after PreprocessData when federated client ranges are used, failing fast with a clear exception if violated.
Applied to files:
src/Data/Loaders/EpisodicDataLoaderBase.cssrc/Data/Loaders/GraphDataLoaderBase.cssrc/Interfaces/IStreamingDataLoader.cssrc/Data/Loaders/StreamingDataLoaderBase.cssrc/Data/Loaders/StreamingDataLoader.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/Data/Loaders/EpisodicDataLoaderBase.cssrc/PredictionModelBuilder.cssrc/Data/Loaders/RLDataLoaderBase.cs
📚 Learning: 2025-12-21T04:19:31.255Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 445
File: src/PredictionModelBuilder.cs:752-763
Timestamp: 2025-12-21T04:19:31.255Z
Learning: Federated preprocessing invariant: In PredictionModelBuilder.BuildSupervisedInternalAsync (src/PredictionModelBuilder.cs), when using IFederatedClientDataLoader, preprocessing must preserve per-client row ordering and total sample counts. The code should enforce X/Y alignment and total count checks after PreprocessData whenever federated client ranges are active, and fail fast with a clear exception if violated. This guideline is specific to this file and to similar federated preprocessing paths; apply it to other implementations that use IFederatedClientDataLoader and related BuildSupervisedInternalAsync patterns.
Applied to files:
src/PredictionModelBuilder.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). (3)
- GitHub Check: CodeQL analysis (csharp)
- GitHub Check: Build (Windows)
- GitHub Check: CodeQL Analysis
🔇 Additional comments (25)
src/Data/Loaders/InputOutputDataLoaderBase.cs (2)
256-336: LGTM - Well-structured synchronous batch iteration.The implementation correctly:
- Creates a fresh indices array per iteration to avoid shared state issues
- Uses Fisher-Yates shuffle with O(n) complexity
- Handles
dropLastand partial batches correctly- Validates batch size before iteration
444-449: Producer exception may cause consumer to hang if cancellation isn't triggered.If the producer throws an exception other than
OperationCanceledException, thefinallyblock still callsComplete(), which is correct. However, the exception will be re-thrown when awaitingproducerTaskat line 461. This is the correct pattern, but consider logging or wrapping unexpected exceptions for better diagnostics.The current implementation is acceptable since exceptions will propagate when
await producerTaskis called.src/Data/Loaders/GraphDataLoaderBase.cs (1)
500-515: Consider returning a batched graph structure instead of individual graphs.The current implementation yields individual graphs even when
batchSize > 1. The comment at line 500-501 acknowledges this design choice, but it may be confusing since the method signature implies batch-level iteration. The batch size effectively becomes a count of graphs to process, not a true mini-batch.This is acceptable for GNN workloads where batching graphs requires special handling (e.g., graph pooling), but consider documenting this more prominently in the XML remarks.
src/Data/Sampling/RandomSampler.cs (1)
30-86: LGTM - Solid RandomSampler implementation.The implementation correctly:
- Validates dataset size
- Uses Fisher-Yates for O(n) unbiased shuffling
- Uses
RandomHelperfor consistent RNG creation across the codebasesrc/Interfaces/IBatchIterable.cs (1)
46-116: Verify that all existing implementers have been updated.Adding new methods to an existing interface is a breaking change for any external implementers. Ensure all classes implementing
IBatchIterable<TBatch>in the codebase have been updated to includeGetBatchesandGetBatchesAsync.src/Data/Loaders/DataLoaders.cs (1)
479-620: LGTM - Clean streaming factory methods with good documentation.The factory methods provide a beginner-friendly API for creating streaming data loaders with:
- Sensible defaults (
prefetchCount = 2,numWorkers = 4)- Comprehensive XML documentation with practical examples
- Proper delegation to underlying loader constructors
Verify that the referenced streaming loader types exist:
#!/bin/bash # Verify StreamingDataLoader types exist echo "=== Checking for StreamingDataLoader types ===" rg -n "class StreamingDataLoader<" --type=cs rg -n "class FileStreamingDataLoader<" --type=cs rg -n "class CsvStreamingDataLoader<" --type=cssrc/Data/Sampling/StratifiedSampler.cs (2)
158-163: Final shuffle may reduce per-batch stratification.The algorithm carefully interleaves classes proportionally, then shuffles the entire result. This ensures epoch-level proportionality but individual batches drawn from this sequence won't have guaranteed per-batch class balance.
This is fine if the intent is epoch-level stratification. If per-batch balance is needed, use
StratifiedBatchSamplerinstead. Consider clarifying this in the documentation.
329-376: Batch assembly may not achieve strict per-batch balance.When
batchSizedoesn't divide evenly bynumClasses, some classes will have more samples per batch. The "fill remaining slots" loop (lines 350-363) takes from any available class, potentially skewing distribution.This is acceptable behavior but worth documenting for users expecting strict balance.
src/Data/Sampling/WeightedSampler.cs (1)
255-298: Well-implemented class balancing helper.The inverse frequency weighting formula is correct and handles edge cases (zero counts, out-of-range labels). Good addition for imbalanced dataset handling.
src/Extensions/DataLoaderExtensions.cs (2)
110-212: Clean fluent builder implementation.The builder pattern is well-implemented with sensible defaults. Implementing
IEnumerable<TBatch>enables direct use inforeachloops and LINQ.
224-307: Async builder follows same clean pattern.Consistent with the sync builder.
IAsyncEnumerable<TBatch>implementation enablesawait foreach.src/Data/Sampling/ImportanceSampler.cs (2)
279-300: Well-implemented correction factor for unbiased gradients.The
1/(n * p_i)formula with epsilon floor correctly implements importance sampling correction. Good documentation explaining the purpose.
551-575: Diversity selection is index-based, not feature-based.The current implementation spreads selection evenly by index position, which doesn't guarantee feature-space diversity. The comment on line 553-554 acknowledges this limitation.
This is acceptable as a baseline but consider documenting that true diversity requires feature embeddings.
src/Data/Sampling/CurriculumSampler.cs (1)
86-99: Well-structured constructor with proper validation.Input validation and sensible defaults. Clean use of
RandomHelperfor consistent RNG behavior.src/Data/Loaders/StreamingDataLoaderBase.cs (1)
121-127: Blocking on async in synchronous path.Using
.GetAwaiter().GetResult()can cause deadlocks in certain synchronization contexts (e.g., UI threads, ASP.NET classic).This is acceptable for a data loader typically used in background processing, but consider documenting the limitation.
Verify that callers won't use this from contexts with synchronization contexts that could deadlock.
src/Optimizers/OptimizationDataBatcher.cs (3)
266-301: Robust tensor sample copying implementation.Correctly handles arbitrary tensor dimensions by computing flat-to-multidimensional index conversion. The algorithm is sound for any rank.
321-328: WithClassBalancing uses WeightedSampler instead of T.This always creates
WeightedSampler<double>regardless of the batcher's type parameterT. This is likely intentional for precision in weight calculations, but breaks the type consistency pattern.Consider documenting this design choice or using
Tif weights should match the model's numeric type.
36-70: Well-designed internal batcher with sampler integration.Clean constructor with validation, good integration with the new sampler infrastructure. The internal visibility is appropriate for optimizer-level usage.
src/Data/Pipeline/DataPipeline.cs (2)
1-449: Well-designed fluent data pipeline API.The
DataPipeline<T>class provides a comprehensive set of lazy, chainable operations following established patterns from TensorFlow's tf.data. The shuffle buffer algorithm, caching with thread-safety, and batch/window operations are correctly implemented.
551-636: AsyncDataPipeline implementation is solid.The async pipeline correctly wraps
IAsyncEnumerableProvider<T>and provides appropriate async counterparts for the sync operations. The iterator classes properly handle cancellation tokens and use theEnumeratorCancellationattribute.src/Data/Loaders/StreamingDataLoader.cs (3)
44-87: Clean streaming data loader implementation.The base
StreamingDataLoaderclass correctly validates inputs and delegates sample reading to the user-provided async function. The design allows flexible data sources while the base class handles batching and prefetching.
119-178: FileStreamingDataLoader correctly enumerates and streams files.The implementation properly validates inputs and enumerates files at construction time. Exposing
FilePathsasIReadOnlyList<string>is good for debugging and inspection.
316-355: GetSequentialBatches provides efficient true streaming.This method correctly reads the CSV line-by-line without loading into memory, making it ideal for memory-constrained scenarios where shuffling isn't needed.
src/Data/Sampling/Samplers.cs (2)
32-383: Well-organized factory class with comprehensive documentation.The
Samplersstatic factory provides a clean, discoverable API for creating various sampling strategies. The beginner-friendly documentation with code examples is particularly helpful for newcomers to ML data loading patterns.
152-159: Verifyreplacement: falseis intentional for balanced sampling.Balanced sampling typically uses replacement to allow minority class samples to be drawn multiple times per epoch. With
replacement: false, each sample is drawn at most once, which may not achieve the balancing effect whennumSamples == labels.Count. Confirm whether this is the intended behavior or if the weighting mechanism handles rebalancing differently.
- Create DataSamplerBase, WeightedSamplerBase<T>, and EpochAdaptiveSamplerBase<T> providing common sampler functionality (shuffle, seed management, epoch callbacks) - Refactor all sampler classes to inherit from appropriate base classes: - RandomSampler, SequentialSampler, SubsetSampler extend DataSamplerBase - StratifiedSampler, StratifiedBatchSampler extend DataSamplerBase - WeightedSampler<T> extends WeightedSamplerBase<T> - CurriculumSampler<T>, SelfPacedSampler<T> extend EpochAdaptiveSamplerBase<T> - ImportanceSampler<T>, ActiveLearningSampler<T> extend DataSamplerBase - Add OnEpochStart(int epoch) method to IDataSampler interface - Add DataSampler, ShuffleData, DropLastBatch, RandomSeed options to GradientBasedOptimizerOptions - Add CreateBatcher helper methods and NotifyEpochStart to GradientBasedOptimizerBase for DataLoader integration - Refactor MiniBatchGradientDescentOptimizer to use CreateBatcher helper - Move CurriculumStrategy and ActiveLearningStrategy enums to namespace level - Make OptimizationDataBatcher and extensions public for optimizer access Part of Issue #443: DataLoader and Batching Utilities (Phase 5) 🤖 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.
Pull request overview
This PR introduces comprehensive dataloader batching utilities supporting both PyTorch and TensorFlow paradigms, with streaming capabilities, advanced sampling strategies, and async/parallel processing.
Key Changes
- Added batch iteration interfaces (GetBatches, GetBatchesAsync) to IBatchIterable with lazy evaluation and prefetching
- Implemented streaming data loaders for on-demand data reading from files, directories, and CSV sources
- Created comprehensive sampling strategies including random, stratified, weighted, curriculum, importance, and active learning samplers
- Added TensorFlow-style data pipeline operators (map, filter, cache, shuffle, batch, prefetch)
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Interfaces/IBatchIterable.cs | Extended with GetBatches methods for lazy batch iteration |
| src/Interfaces/IStreamingDataLoader.cs | New interface for streaming data loaders with async batch iteration |
| src/Interfaces/IDataSampler.cs | New sampling interfaces supporting multiple strategies |
| src/PredictionModelBuilder.cs | Integrated streaming loader support with batch collection utilities |
| src/Optimizers/OptimizationDataBatcher.cs | New batcher for optimization input data with sampling support |
| src/Extensions/DataLoaderExtensions.cs | Fluent API for batch configuration and pipeline creation |
| src/Data/Sampling/*.cs | Complete sampling strategy implementations (random, stratified, weighted, curriculum, importance, active learning) |
| src/Data/Pipeline/DataPipeline.cs | TensorFlow-style pipeline operators with async support |
| src/Data/Loaders/StreamingDataLoaderBase.cs | Base class for streaming loaders with prefetching and parallel workers |
| src/Data/Loaders/StreamingDataLoader.cs | Concrete streaming implementations (generic, file, CSV) |
| src/Data/Loaders/ParallelBatchLoader.cs | Multi-worker parallel batch loading |
| src/Data/Loaders/*DataLoaderBase.cs | Added batch iteration support to all existing loader types |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Phase 6 of Issue #443 DataLoader Integration: - Add OptimizationDataBatcherTests.cs (29 tests) - Constructor validation - Property tests (DataSize, BatchSize, NumBatches) - GetBatches iteration tests - Shuffle and seed reproducibility - Edge cases (batch size larger than data) - Add DataSamplerTests.cs (45 tests) - RandomSampler tests - SequentialSampler tests - SubsetSampler tests - CurriculumSampler tests with all strategies - SelfPacedSampler tests - WeightedSampler tests including class balancing - Add OptimizerBatchingTests.cs (26 tests) - Batcher creation with various options - Custom sampler integration - Epoch notification forwarding - Default batch size for all optimizer types - WithSampler and WithCurriculumLearning methods - Update integration checklist with test documentation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Issue #443 DataLoader Integration (Phases 1-5): First-Order Optimizers (18 total): - SGD, MiniBatch GD, Gradient Descent (BatchSize defaults: 1, 32, 32) - Adam, AdamW, Nadam, AMSGrad, AdaMax (BatchSize default: 32) - Momentum, Nesterov (BatchSize default: 32) - Adagrad, AdaDelta, RMSprop (BatchSize default: 32) - Lion, FTRL, ProximalGD (BatchSize default: 32) - ADMM, Coordinate Descent (BatchSize: -1, epoch notification only) Second-Order Optimizers (7 total, full-batch): - ConjugateGradient, BFGS, L-BFGS, DFP - NewtonMethod, LevenbergMarquardt, TrustRegion - All use BatchSize = -1 for Hessian/quasi-Newton consistency Changes per optimizer: - Add BatchSize property to options (default: 32 or -1) - Add NotifyEpochStart(epoch) for curriculum learning - Change iteration loop to epoch-based loop - Add DataLoader Integration documentation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add BenchmarkDotNet benchmarks for DataLoader infrastructure: - Batcher creation benchmarks (with/without shuffle) - Batch iteration across dataset sizes (100, 1000, 10000) - Batch size comparison (1, 16, 32, 64, 128) - Shuffle vs no-shuffle overhead measurement - Sampler performance (Random, Sequential, Curriculum) - GetBatchIndices vs GetBatches comparison - DropLast impact measurement - WeightedSampler benchmarks Phase 7 of Issue #443 DataLoader Integration complete. 🤖 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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Optimizers/RootMeanSquarePropagationOptimizer.cs (2)
136-168: Duplicate<remarks>blocks in XML documentation.There are two
<remarks>sections on theOptimizemethod (lines 136-160 and 162-168). The second one should be merged into the first or the opening<remarks>tag at line 162 should be removed.🔎 Proposed fix
/// </para> /// </remarks> - /// <remarks> /// <para><b>DataLoader Integration:</b> This method uses the DataLoader API for efficient batch processing. /// It creates a batcher using <see cref="GradientBasedOptimizerBase{T,TInput,TOutput}.CreateBatcher"/> /// and notifies the sampler of epoch starts using /// <see cref="GradientBasedOptimizerBase{T,TInput,TOutput}.NotifyEpochStart"/>. /// </para> - /// </remarks>
271-305: Malformed XML documentation block.Lines 271-305 contain broken XML documentation. The
<remarks>section starting at line 278 appears incomplete, and there's a stray</summary>tag at line 280. This seems to be remnants of incomplete refactoring.🔎 Proposed fix - restructure the documentation
/// <summary> /// Reverses an RMSProp gradient update to recover original parameters. /// </summary> /// <param name="updatedParameters">Parameters after RMSProp update</param> /// <param name="appliedGradients">The gradients that were applied</param> /// <returns>Original parameters before the update</returns> /// <remarks> /// <para> - /// Updates a solution model using the RMSProp algorithm. - /// </summary> - /// <param name="currentSolution">The current solution model to update.</param> - /// <param name="gradient">The gradient vector for the solution.</param> - /// <returns>The updated solution model.</returns> - /// <remarks> - /// <para> - /// This method applies the RMSProp update rule to the coefficients of a solution model. - /// It follows the same steps as UpdateVector, but operates directly on the solution model's - /// coefficients. For each coefficient, it: - /// 1. Updates the running average of squared gradients - /// 2. Calculates an adaptive learning rate by dividing the base learning rate by the square root - /// of the running average (plus epsilon for numerical stability) - /// 3. Updates the coefficient by subtracting the product of the adaptive learning rate and the gradient + /// For RMSProp, the forward update is: + /// 1. _squaredGradient[i] = decay * _squaredGradient[i] + (1 - decay) * gradient[i]^2 + /// 2. update = learning_rate * gradient[i] / (sqrt(_squaredGradient[i]) + epsilon) + /// 3. params_new = params_old - update + /// + /// To reverse: params_old = params_new + update /// </para> - /// <para><b>For Beginners:</b> This method moves the solution in the direction of improvement. - /// - /// Think of it as the hiker taking one step: - /// - For each direction, it updates the memory of how steep that direction has been - /// - It calculates custom step sizes for each direction based on their history - /// - Steeper directions get smaller, more careful steps - /// - Gentler directions get larger, more confident steps - /// - The solution then moves according to these personalized step sizes - /// - /// This adaptive movement helps the algorithm navigate efficiently toward better solutions. + /// <para><b>For Beginners:</b> + /// This is like retracing the hiker's steps. Given where the hiker ended up (updated parameters) + /// and the terrain steepness history (squared gradients), we can calculate the exact step size + /// that was used and determine where the hiker started from. /// </para> /// </remarks> protected override IFullModel<T, TInput, TOutput> UpdateSolution(...)
♻️ Duplicate comments (2)
src/Data/Sampling/ImportanceSampler.cs (1)
44-45: Fields can be madereadonly.Both
_importanceScoresand_cumulativeProbabilitiesare assigned only in the constructor and never reassigned (only their elements are modified).src/Data/Sampling/WeightedSampler.cs (1)
146-154: Potential integer overflow in class weight calculation.At line 148,
numClasses * classCounts[c]performs integer multiplication before the division. For large datasets or many classes, this could overflow.This issue was flagged in a previous review. Consider casting earlier:
- classWeights[c] = (double)totalSamples / (numClasses * classCounts[c]); + classWeights[c] = (double)totalSamples / ((double)numClasses * classCounts[c]);
🧹 Nitpick comments (19)
src/Models/Options/LionOptimizerOptions.cs (1)
20-28: LGTM! BatchSize property addition aligns with dataloader batching objectives.The new
BatchSizeproperty is well-documented and provides a sensible default (32) for mini-batch training. The documentation style is consistent with the rest of the class.Optional enhancement: Consider adding input validation to enforce the "positive integer" constraint mentioned in the documentation, either through a private setter with validation logic or via data annotations. This would catch configuration errors earlier rather than at runtime during optimization.
Optional: Add validation for BatchSize
Example using a backing field with validation:
+ private int _batchSize = 32; + /// <summary> /// Gets or sets the batch size for mini-batch gradient descent. /// </summary> /// <value>A positive integer, defaulting to 32.</value> /// <remarks> /// <para><b>For Beginners:</b> The batch size controls how many examples the optimizer looks at /// before making an update to the model. The default of 32 is a good balance for Lion.</para> /// </remarks> - public int BatchSize { get; set; } = 32; + public int BatchSize + { + get => _batchSize; + set => _batchSize = value > 0 ? value : throw new ArgumentOutOfRangeException(nameof(value), "BatchSize must be positive."); + }Alternatively, if the optimizer implementation already validates this or if negative/zero values have special meaning (e.g., -1 for full-batch mode as mentioned in other optimizers), the current implementation is fine.
src/Models/Options/ConjugateGradientOptimizerOptions.cs (1)
30-30: Consider adding validation for BatchSize.The property currently accepts any integer value without validation. While validation may occur at usage time, adding a setter guard could provide clearer error messages for invalid values (e.g., 0, -2).
🔎 Example validation approach
- public int BatchSize { get; set; } = -1; + private int _batchSize = -1; + + public int BatchSize + { + get => _batchSize; + set + { + if (value != -1 && value <= 0) + { + throw new ArgumentOutOfRangeException(nameof(value), + "BatchSize must be a positive integer or -1 for full batch."); + } + _batchSize = value; + } + }src/Models/Options/DFPOptimizerOptions.cs (1)
32-32: Consider using a more type-safe representation for BatchSize.While the current
inttype with -1 as a sentinel value is functional, consider these alternatives for improved type safety and clarity:
int?wherenullmeans full batch- A dedicated enum or struct that clearly distinguishes between full-batch and mini-batch modes
This would eliminate invalid values (0, arbitrary negative numbers) at the type level rather than requiring runtime validation.
src/Models/Options/NadamOptimizerOptions.cs (1)
36-44: Consider adding validation for BatchSize.The BatchSize property lacks validation for invalid values (e.g., zero, negative values other than special sentinels, or excessively large values). While runtime validation may occur elsewhere, enforcing constraints at the property level improves early error detection and API clarity.
💡 Proposed validation approach
If -1 is the only valid negative value (indicating full-batch), consider validation like:
- public int BatchSize { get; set; } = 32; + private int _batchSize = 32; + + public int BatchSize + { + get => _batchSize; + set + { + if (value <= 0 && value != -1) + throw new ArgumentOutOfRangeException(nameof(value), "BatchSize must be positive or -1 for full-batch."); + _batchSize = value; + } + }Alternatively, document valid ranges in remarks if validation is intentionally deferred to the optimizer implementation.
src/Models/Options/CoordinateDescentOptimizerOptions.cs (1)
22-32: Consider adding validation for BatchSize.Similar to other optimizer options, BatchSize lacks validation for invalid values. The documentation indicates -1 is valid for full-batch mode, but other negative values, zero, or excessively large values should be rejected.
💡 Proposed validation approach
- public int BatchSize { get; set; } = -1; + private int _batchSize = -1; + + public int BatchSize + { + get => _batchSize; + set + { + if (value <= 0 && value != -1) + throw new ArgumentOutOfRangeException(nameof(value), "BatchSize must be positive or -1 for full-batch."); + _batchSize = value; + } + }src/Models/Options/AMSGradOptimizerOptions.cs (1)
21-29: Consider adding validation for BatchSize.BatchSize should validate that values are either positive or -1 (for full-batch mode), rejecting zero and other negative values.
src/Models/Options/AdaMaxOptimizerOptions.cs (1)
18-26: Consider adding validation for BatchSize.BatchSize should validate against invalid values (zero, negative values except -1).
src/Models/Options/LevenbergMarquardtOptimizerOptions.cs (1)
36-47: Reconsider exposing BatchSize for Levenberg-Marquardt.The documentation states that Levenberg-Marquardt "requires computing the Jacobian matrix across the entire dataset" and that "using mini-batches would introduce noise that makes the Jacobian approximation unreliable and would compromise the algorithm's ability to balance between Gauss-Newton and gradient descent." If only full-batch mode (BatchSize = -1) is algorithmically valid, consider one of the following approaches:
- Remove the property and hardcode full-batch behavior in the optimizer implementation.
- Make it read-only with a fixed value of -1.
- Add strict validation that throws if any value other than -1 is set, with a clear error message.
Exposing a configurable property that should never be changed invites misconfiguration and algorithmic incorrectness.
src/Models/Options/AdamOptimizerOptions.cs (1)
17-36: Consider adding validation for BatchSize.BatchSize should validate that values are positive or -1 (full-batch), rejecting zero and other negative values.
src/Interfaces/IDataSampler.cs (2)
116-144: Add type constraint to IWeightedSampler.The IWeightedSampler interface exposes Weights of type IReadOnlyList and performs operations that require T to be numeric (weight-based probability calculations, normalization, etc.). Without a type constraint, implementations cannot safely perform these operations.
💡 Proposed fix
-public interface IWeightedSampler<T> : IDataSampler +public interface IWeightedSampler<T> : IDataSampler + where T : struct, IComparable<T>, IConvertibleOr if the codebase uses a specific numeric constraint pattern (e.g.,
INumber<T>from .NET 7+), apply that constraint consistently.
125-125: Consider read-only properties or init-only setters for collection properties.The Weights property on IWeightedSampler and Labels property on IStratifiedSampler use IReadOnlyList for the getter but expose a setter. This pattern allows the entire collection to be replaced but not mutated in place. If these properties are intended to be set once during initialization and not changed afterward, consider:
- Using
initsetters (C# 9+) to enforce initialization-only semantics.- Documenting the intended mutability semantics clearly in remarks.
Example with init setter
- IReadOnlyList<T> Weights { get; set; } + IReadOnlyList<T> Weights { get; init; }This prevents accidental modification after construction while still allowing collection replacement during initialization.
Also applies to: 154-154
src/Data/Sampling/StratifiedSampler.cs (1)
86-93: Out-of-range labels are silently ignored.Labels outside
[0, numClasses)are silently skipped without warning. This could mask data quality issues where labels don't match the expected class count.Consider logging a warning or throwing when invalid labels are encountered, or at minimum documenting this behavior.
🔎 Example: Add validation or warning
for (int i = 0; i < _labels.Length; i++) { int label = _labels[i]; if (label >= 0 && label < _numClasses) { _classIndices[label].Add(i); } + else + { + // Consider: throw new ArgumentException($"Label {label} at index {i} is out of range [0, {_numClasses})"); + // Or: log a warning for debugging purposes + } }src/Data/Sampling/ImportanceSampler.cs (1)
538-562: SelectByDiversity is a simplified placeholder implementation.The current diversity selection uses stride-based sampling rather than true feature-based clustering. The inline comment acknowledges this limitation. For a production-ready implementation, consider implementing k-means or k-center greedy selection on feature embeddings.
src/Optimizers/BFGSOptimizer.cs (1)
113-116: Consider refactoring_iterationfield redundancy.The loop uses
epoch(lines 113, 115, 127), but line 116 increments_iteration, creating redundancy. The_iterationfield is used inGenerateGradientCacheKey(line 348), butepochcould serve the same purpose.Consider using
epochdirectly in the cache key instead of maintaining a separate_iterationcounter, reducing state and potential for inconsistency.Proposed refactor
In
GenerateGradientCacheKey, replace_iterationwith the current epoch value (could be passed as a parameter or stored differently):-return $"{baseKey}_BFGS_{_options.InitialLearningRate}_{_options.Tolerance}_{_iteration}"; +return $"{baseKey}_BFGS_{_options.InitialLearningRate}_{_options.Tolerance}_{epoch}";Then remove the
_iterationfield and its initialization/increment logic.src/Optimizers/AdamWOptimizer.cs (1)
271-287: AMSGrad variant handling is correct but could be vectorized.The element-wise max operation for AMSGrad at lines 276-281 uses a manual loop. While functionally correct, this could potentially be vectorized using the Engine for consistency with other operations.
🔎 Optional: Vectorize AMSGrad max operation
If the Engine supports element-wise max, consider:
if (_options.UseAMSGrad && _vMax != null) { - // Update vMax = max(vMax, vHat) - var newVMax = new Vector<T>(_vMax.Length); - for (int i = 0; i < _vMax.Length; i++) - { - newVMax[i] = MathHelper.Max(_vMax[i], vHat[i]); - } - _vMax = newVMax; + // Update vMax = max(vMax, vHat) - vectorized + _vMax = (Vector<T>)Engine.Max(_vMax, vHat); vHatEffective = _vMax; }This depends on whether
Engine.Maxsupports element-wise vector operations.src/Data/Sampling/WeightedSampler.cs (1)
68-111: Without-replacement sampling has O(n·k) complexity.The current implementation recomputes the cumulative sum over remaining indices for each sample. For large datasets with many samples requested, this could be slow. However, for typical batch sampling use cases, this is acceptable.
If performance becomes an issue, consider using the alias method or a reservoir-based approach for O(n + k) complexity.
docs/design/DataLoader-Design-Decisions.md (1)
80-88: Update implementation progress to reflect actual status.Phase 6 (Testing) and Phase 7 (Benchmarks) are marked as PENDING, but this PR includes unit tests (
OptimizationDataBatcherTests.cs,DataSamplerTests.cs) and benchmarks (DataLoaderBenchmarks.cs). Consider updating the status to reflect the current state.tests/AiDotNet.Tests/UnitTests/Data/Sampling/DataSamplerTests.cs (1)
315-331: Test usesOnEpochStart(10)withtotalEpochs: 10, which yields Progress > 1.0.At line 326, calling
sampler.OnEpochStart(10)whentotalEpochsis 10 results inProgress = 10/10 = 1.0(orMath.Min(1.0, 10/10) = 1.0). The comment says "After totalEpochs, threshold should be 1.0" but epoch 10 is actually the 11th epoch (0-indexed), which is beyond the total.This may be intentional to test boundary behavior, but consider clarifying the comment or using epoch 9 for "final epoch" semantics.
src/Optimizers/OptimizationDataBatcher.cs (1)
321-328:WithClassBalancing<TWeight>ignores the type parameter.The generic type parameter
TWeightis declared but never used. The method always createsWeightedSampler<double>. Either remove the unused type parameter or use it consistently.🔎 Proposed fix
-public OptimizationDataBatcher<T, TInput, TOutput> WithClassBalancing<TWeight>( +public OptimizationDataBatcher<T, TInput, TOutput> WithClassBalancing( IReadOnlyList<int> labels, int numClasses)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (66)
AiDotNetBenchmarkTests/BenchmarkTests/DataLoaderBenchmarks.csdocs/design/DataLoader-Design-Decisions.mddocs/design/Optimizer-DataLoader-Integration-Checklist.mdsrc/Data/Sampling/CurriculumSampler.cssrc/Data/Sampling/DataSamplerBase.cssrc/Data/Sampling/ImportanceSampler.cssrc/Data/Sampling/RandomSampler.cssrc/Data/Sampling/Samplers.cssrc/Data/Sampling/StratifiedSampler.cssrc/Data/Sampling/WeightedSampler.cssrc/Interfaces/IDataSampler.cssrc/Models/Options/ADMMOptimizerOptions.cssrc/Models/Options/AMSGradOptimizerOptions.cssrc/Models/Options/AdaDeltaOptimizerOptions.cssrc/Models/Options/AdaMaxOptimizerOptions.cssrc/Models/Options/AdagradOptimizerOptions.cssrc/Models/Options/AdamOptimizerOptions.cssrc/Models/Options/AdamWOptimizerOptions.cssrc/Models/Options/BFGSOptimizerOptions.cssrc/Models/Options/ConjugateGradientOptimizerOptions.cssrc/Models/Options/CoordinateDescentOptimizerOptions.cssrc/Models/Options/DFPOptimizerOptions.cssrc/Models/Options/FTRLOptimizerOptions.cssrc/Models/Options/GradientBasedOptimizerOptions.cssrc/Models/Options/GradientDescentOptimizerOptions.cssrc/Models/Options/LBFGSOptimizerOptions.cssrc/Models/Options/LevenbergMarquardtOptimizerOptions.cssrc/Models/Options/LionOptimizerOptions.cssrc/Models/Options/MomentumOptimizerOptions.cssrc/Models/Options/NadamOptimizerOptions.cssrc/Models/Options/NesterovAcceleratedGradientOptimizerOptions.cssrc/Models/Options/NewtonMethodOptimizerOptions.cssrc/Models/Options/ProximalGradientDescentOptimizerOptions.cssrc/Models/Options/RootMeanSquarePropagationOptimizerOptions.cssrc/Models/Options/StochasticGradientDescentOptimizerOptions.cssrc/Models/Options/TrustRegionOptimizerOptions.cssrc/Optimizers/ADMMOptimizer.cssrc/Optimizers/AMSGradOptimizer.cssrc/Optimizers/AdaDeltaOptimizer.cssrc/Optimizers/AdaMaxOptimizer.cssrc/Optimizers/AdagradOptimizer.cssrc/Optimizers/AdamOptimizer.cssrc/Optimizers/AdamWOptimizer.cssrc/Optimizers/BFGSOptimizer.cssrc/Optimizers/ConjugateGradientOptimizer.cssrc/Optimizers/CoordinateDescentOptimizer.cssrc/Optimizers/DFPOptimizer.cssrc/Optimizers/FTRLOptimizer.cssrc/Optimizers/GradientBasedOptimizerBase.cssrc/Optimizers/GradientDescentOptimizer.cssrc/Optimizers/LBFGSOptimizer.cssrc/Optimizers/LevenbergMarquardtOptimizer.cssrc/Optimizers/LionOptimizer.cssrc/Optimizers/MiniBatchGradientDescentOptimizer.cssrc/Optimizers/MomentumOptimizer.cssrc/Optimizers/NadamOptimizer.cssrc/Optimizers/NesterovAcceleratedGradientOptimizer.cssrc/Optimizers/NewtonMethodOptimizer.cssrc/Optimizers/OptimizationDataBatcher.cssrc/Optimizers/ProximalGradientDescentOptimizer.cssrc/Optimizers/RootMeanSquarePropagationOptimizer.cssrc/Optimizers/StochasticGradientDescentOptimizer.cssrc/Optimizers/TrustRegionOptimizer.cstests/AiDotNet.Tests/UnitTests/Data/Sampling/DataSamplerTests.cstests/AiDotNet.Tests/UnitTests/Optimizers/OptimizationDataBatcherTests.cstests/AiDotNet.Tests/UnitTests/Optimizers/OptimizerBatchingTests.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/Models/Options/LBFGSOptimizerOptions.cssrc/Models/Options/LionOptimizerOptions.cssrc/Models/Options/ConjugateGradientOptimizerOptions.cssrc/Optimizers/ConjugateGradientOptimizer.cssrc/Models/Options/AdaMaxOptimizerOptions.cssrc/Models/Options/RootMeanSquarePropagationOptimizerOptions.cssrc/Models/Options/AdagradOptimizerOptions.cssrc/Optimizers/DFPOptimizer.cssrc/Models/Options/FTRLOptimizerOptions.cssrc/Models/Options/TrustRegionOptimizerOptions.cssrc/Models/Options/LevenbergMarquardtOptimizerOptions.cssrc/Optimizers/CoordinateDescentOptimizer.cssrc/Models/Options/AdamWOptimizerOptions.cssrc/Models/Options/AMSGradOptimizerOptions.cssrc/Interfaces/IDataSampler.cssrc/Models/Options/AdamOptimizerOptions.cssrc/Optimizers/AdamOptimizer.cssrc/Models/Options/NesterovAcceleratedGradientOptimizerOptions.cssrc/Optimizers/ProximalGradientDescentOptimizer.cssrc/Optimizers/TrustRegionOptimizer.cssrc/Optimizers/AMSGradOptimizer.cssrc/Optimizers/NadamOptimizer.cssrc/Models/Options/BFGSOptimizerOptions.cstests/AiDotNet.Tests/UnitTests/Data/Sampling/DataSamplerTests.cssrc/Optimizers/NesterovAcceleratedGradientOptimizer.cssrc/Optimizers/BFGSOptimizer.cssrc/Models/Options/AdaDeltaOptimizerOptions.cssrc/Models/Options/MomentumOptimizerOptions.cssrc/Optimizers/GradientDescentOptimizer.cssrc/Models/Options/CoordinateDescentOptimizerOptions.cssrc/Optimizers/NewtonMethodOptimizer.cssrc/Optimizers/MiniBatchGradientDescentOptimizer.cssrc/Optimizers/StochasticGradientDescentOptimizer.cssrc/Models/Options/GradientBasedOptimizerOptions.cssrc/Optimizers/LionOptimizer.cssrc/Models/Options/ADMMOptimizerOptions.cssrc/Optimizers/AdaDeltaOptimizer.cssrc/Models/Options/DFPOptimizerOptions.cssrc/Data/Sampling/WeightedSampler.cssrc/Data/Sampling/StratifiedSampler.cssrc/Models/Options/NewtonMethodOptimizerOptions.cssrc/Optimizers/AdamWOptimizer.cssrc/Optimizers/AdaMaxOptimizer.cssrc/Optimizers/LevenbergMarquardtOptimizer.cstests/AiDotNet.Tests/UnitTests/Optimizers/OptimizationDataBatcherTests.cssrc/Optimizers/FTRLOptimizer.cssrc/Models/Options/ProximalGradientDescentOptimizerOptions.cssrc/Data/Sampling/DataSamplerBase.csAiDotNetBenchmarkTests/BenchmarkTests/DataLoaderBenchmarks.cssrc/Optimizers/OptimizationDataBatcher.cssrc/Optimizers/LBFGSOptimizer.cssrc/Models/Options/StochasticGradientDescentOptimizerOptions.cssrc/Optimizers/MomentumOptimizer.cssrc/Data/Sampling/Samplers.cssrc/Data/Sampling/RandomSampler.cssrc/Optimizers/AdagradOptimizer.cssrc/Data/Sampling/ImportanceSampler.cssrc/Optimizers/ADMMOptimizer.cssrc/Optimizers/RootMeanSquarePropagationOptimizer.cssrc/Data/Sampling/CurriculumSampler.cssrc/Models/Options/NadamOptimizerOptions.cstests/AiDotNet.Tests/UnitTests/Optimizers/OptimizerBatchingTests.cssrc/Optimizers/GradientBasedOptimizerBase.cssrc/Models/Options/GradientDescentOptimizerOptions.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/Models/Options/LBFGSOptimizerOptions.cssrc/Models/Options/LionOptimizerOptions.cssrc/Models/Options/ConjugateGradientOptimizerOptions.cssrc/Optimizers/ConjugateGradientOptimizer.cssrc/Models/Options/AdaMaxOptimizerOptions.cssrc/Models/Options/RootMeanSquarePropagationOptimizerOptions.cssrc/Models/Options/AdagradOptimizerOptions.cssrc/Optimizers/DFPOptimizer.cssrc/Models/Options/FTRLOptimizerOptions.cssrc/Models/Options/TrustRegionOptimizerOptions.cssrc/Models/Options/LevenbergMarquardtOptimizerOptions.cssrc/Optimizers/CoordinateDescentOptimizer.cssrc/Models/Options/AdamWOptimizerOptions.cssrc/Models/Options/AMSGradOptimizerOptions.cssrc/Interfaces/IDataSampler.cssrc/Models/Options/AdamOptimizerOptions.cssrc/Optimizers/AdamOptimizer.cssrc/Models/Options/NesterovAcceleratedGradientOptimizerOptions.cssrc/Optimizers/ProximalGradientDescentOptimizer.cssrc/Optimizers/TrustRegionOptimizer.cssrc/Optimizers/AMSGradOptimizer.cssrc/Optimizers/NadamOptimizer.cssrc/Models/Options/BFGSOptimizerOptions.cstests/AiDotNet.Tests/UnitTests/Data/Sampling/DataSamplerTests.cssrc/Optimizers/NesterovAcceleratedGradientOptimizer.cssrc/Optimizers/BFGSOptimizer.cssrc/Models/Options/AdaDeltaOptimizerOptions.cssrc/Models/Options/MomentumOptimizerOptions.cssrc/Optimizers/GradientDescentOptimizer.cssrc/Models/Options/CoordinateDescentOptimizerOptions.cssrc/Optimizers/NewtonMethodOptimizer.cssrc/Optimizers/MiniBatchGradientDescentOptimizer.cssrc/Optimizers/StochasticGradientDescentOptimizer.cssrc/Models/Options/GradientBasedOptimizerOptions.cssrc/Optimizers/LionOptimizer.cssrc/Models/Options/ADMMOptimizerOptions.cssrc/Optimizers/AdaDeltaOptimizer.cssrc/Models/Options/DFPOptimizerOptions.cssrc/Data/Sampling/WeightedSampler.cssrc/Data/Sampling/StratifiedSampler.cssrc/Models/Options/NewtonMethodOptimizerOptions.cssrc/Optimizers/AdamWOptimizer.cssrc/Optimizers/AdaMaxOptimizer.cssrc/Optimizers/LevenbergMarquardtOptimizer.cstests/AiDotNet.Tests/UnitTests/Optimizers/OptimizationDataBatcherTests.cssrc/Optimizers/FTRLOptimizer.cssrc/Models/Options/ProximalGradientDescentOptimizerOptions.cssrc/Data/Sampling/DataSamplerBase.csAiDotNetBenchmarkTests/BenchmarkTests/DataLoaderBenchmarks.cssrc/Optimizers/OptimizationDataBatcher.cssrc/Optimizers/LBFGSOptimizer.cssrc/Models/Options/StochasticGradientDescentOptimizerOptions.cssrc/Optimizers/MomentumOptimizer.cssrc/Data/Sampling/Samplers.cssrc/Data/Sampling/RandomSampler.cssrc/Optimizers/AdagradOptimizer.cssrc/Data/Sampling/ImportanceSampler.cssrc/Optimizers/ADMMOptimizer.cssrc/Optimizers/RootMeanSquarePropagationOptimizer.cssrc/Data/Sampling/CurriculumSampler.cssrc/Models/Options/NadamOptimizerOptions.cstests/AiDotNet.Tests/UnitTests/Optimizers/OptimizerBatchingTests.cssrc/Optimizers/GradientBasedOptimizerBase.cssrc/Models/Options/GradientDescentOptimizerOptions.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.cssrc/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). (2)
- GitHub Check: Build (Windows)
- GitHub Check: CodeQL Analysis
ParallelBatchLoader: - Change batchFactory from Func<int, TBatch> to Func<int[], TBatch> to properly process all indices in each batch - Add proper exception handling with worker fault tracking - Propagate exceptions through channel completion StreamingDataLoader/CsvStreamingDataLoader: - Add thread-safe lazy initialization with double-checked locking - Add _cacheLock and volatile _cachedLines for proper synchronization MemoryMappedStreamingDataLoader: - Implement actual memory-mapped file access using MemoryMappedFile - Add proper thread-safe initialization via GetViewAccessor() - Add proper null checks instead of null-forgiving operator - Validate file size against expected data size - Implement IDisposable with proper cleanup - Add read byte count validation StreamingDataLoaderBase: - Add SemaphoreSlim to respect _numWorkers configuration - Limit concurrent sample reads to configured worker count 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- PredictionModelBuilder: implement BuildStreamingSupervisedAsync with true streaming training using ComputeGradients and ApplyGradients, avoiding materializing all data in memory - EpisodicDataLoaderBase: fix shared state in GetBatches/GetBatchesAsync, use proper default for TryGetNextBatch instead of default - RLDataLoaderBase: add thread-safe random access with _randomLock, fix SelectRandomAction and SetSeed to use locking - DataPipeline: implement proper Channel-based parallel processing in MapAsyncIterator, fix misleading partial windows comment - IStreamingDataLoader: fix example to show BatchSize property instead of non-existent parameter 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
AMSGradOptimizer: - Fix MAJOR bug: Remove duplicate _t increment causing incorrect bias correction - Merge duplicate <remarks> blocks into single block Other optimizers: - Merge duplicate <remarks> documentation blocks in 8 optimizers (AdaDelta, AdaMax, Adagrad, GradientDescent, Lion, Nadam, NesterovAcceleratedGradient, RootMeanSquarePropagation) Optimizer options: - Fix doc contradiction in ConjugateGradientOptimizerOptions and DFPOptimizerOptions where <value> said "positive integer" but default was -1 Samplers: - ImportanceSampler: Make fields readonly - WeightedSampler: Fix potential integer overflow, use ternary operators, fix useless assignment - StratifiedSampler: Use LINQ FirstOrDefault for explicit filtering, iterate over Values instead of mapping Keys PredictionModelBuilder: - Fix silent data loss in aggregation helpers - throw NotSupportedException for unsupported types instead of returning first element 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Use explicit for loop instead of foreach to avoid implicit filtering pattern in InterleaveIterator - Add exception aggregation in finally block to ensure all enumerators are disposed even if one fails - Addresses PR review comments about foreach filtering and manual disposal patterns 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Extract the CopyTensorSample method into a shared TensorCopyHelper class in the AiDotNet.Helpers namespace. This eliminates duplication of the tensor sample copying logic across three files: - InMemoryDataLoader.cs - OptimizationDataBatcher.cs - PredictionModelBuilder.cs 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Extract the aggregation logic for Matrix, Vector, and Tensor types into a shared DataAggregationHelper class. This eliminates duplication between AggregateStreamingInputs and AggregateStreamingOutputs methods in PredictionModelBuilder. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace explicit (TData)(object)result pattern with a helper method CastToDataType that uses implicit boxing. This satisfies code analyzers that flag explicit upcast to object as unnecessary while maintaining the same runtime behavior. Affected files: - DataAggregationHelper.cs - OptimizationDataBatcher.cs - InMemoryDataLoader.cs 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The trainingStartTime variable in BuildStreamingSupervisedAsync was declared but never used. This removes the useless assignment to improve code quality. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove unused trainingStartTime variable in BuildStreamingSupervisedAsync - Extract ComputeCumulativeProbabilitiesCore method to avoid virtual call in WeightedSamplerBase constructor The virtual method is now called via a private non-virtual core method from the constructor, preventing potential issues with uninitialized derived class state. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Pass batchSize parameter to StreamingDataLoaderBase constructor instead of setting the virtual BatchSize property in derived class constructors. This eliminates virtual call warnings in: - StreamingDataLoader - FileStreamingDataLoader - CsvStreamingDataLoader - MemoryMappedStreamingDataLoader Also make Dispose(bool) non-virtual in MemoryMappedStreamingDataLoader to avoid virtual call in destructor warning. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace implicit filtering within foreach loop with explicit .Where() filter for cleaner separation of concerns. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace broad Exception catch clauses with filtered catches that exclude OutOfMemoryException to satisfy code analysis requirements while still capturing worker exceptions for propagation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add protected constructor to DataLoaderBase that sets _batchSize backing field directly instead of through the virtual BatchSize property. Update StreamingDataLoaderBase to pass batchSize to base constructor. 🤖 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Optimizers/RootMeanSquarePropagationOptimizer.cs (1)
269-303: Severely malformed XML documentation.The XML documentation for
ReverseUpdateis corrupted. It appears that documentation from a different method (UpdateSolution) was accidentally merged or copy-pasted into this section:
- Line 275 opens
<remarks>for ReverseUpdate- Lines 276-278 contain an incomplete sentence ending with
</summary>(a stray tag)- Lines 279-302 contain documentation that belongs to
UpdateSolutionThis will cause XML documentation build warnings/errors and incorrect IntelliSense.
🔎 Proposed fix: Restore proper ReverseUpdate documentation
/// <summary> /// Reverses an RMSProp gradient update to recover original parameters. /// </summary> /// <param name="updatedParameters">Parameters after RMSProp update</param> /// <param name="appliedGradients">The gradients that were applied</param> /// <returns>Original parameters before the update</returns> /// <remarks> /// <para> - /// Updates a solution model using the RMSProp algorithm. - /// </summary> - /// <param name="currentSolution">The current solution model to update.</param> - /// <param name="gradient">The gradient vector for the solution.</param> - /// <returns>The updated solution model.</returns> - /// <remarks> - /// <para> - /// This method applies the RMSProp update rule to the coefficients of a solution model. - /// It follows the same steps as UpdateVector, but operates directly on the solution model's - /// coefficients. For each coefficient, it: - /// 1. Updates the running average of squared gradients - /// 2. Calculates an adaptive learning rate by dividing the base learning rate by the square root - /// of the running average (plus epsilon for numerical stability) - /// 3. Updates the coefficient by subtracting the product of the adaptive learning rate and the gradient - /// </para> - /// <para><b>For Beginners:</b> This method moves the solution in the direction of improvement. - /// - /// Think of it as the hiker taking one step: - /// - For each direction, it updates the memory of how steep that direction has been - /// - It calculates custom step sizes for each direction based on their history - /// - Steeper directions get smaller, more careful steps - /// - Gentler directions get larger, more confident steps - /// - The solution then moves according to these personalized step sizes - /// - /// This adaptive movement helps the algorithm navigate efficiently toward better solutions. + /// For RMSprop, the forward update is: + /// 1. _squaredGradient[i] = decay * _squaredGradient[i] + (1 - decay) * gradient[i]^2 + /// 2. update = learning_rate * gradient[i] / (sqrt(_squaredGradient[i]) + epsilon) + /// 3. params_new = params_old - update + /// + /// To reverse: params_old = params_new + update + /// + /// This requires access to the current squared gradient state and the applied gradients + /// to recalculate the adaptive update that was applied. + /// </para> + /// <para><b>For Beginners:</b> + /// This is like retracing the hiker's steps. Given where the hiker ended up (updated parameters) + /// and the terrain steepness history (squared gradients), we can calculate the exact step size + /// that was used and determine where the hiker started from. /// </para> /// </remarks> protected override IFullModel<T, TInput, TOutput> UpdateSolution(...)Note: The actual
ReverseUpdatemethod starting at line 524 has correct documentation. The malformed section at lines 269-303 appears to be misplaced content that should be removed entirely.
♻️ Duplicate comments (3)
src/Optimizers/AdaDeltaOptimizer.cs (1)
171-175: Duplicate<remarks>XML documentation block.The Optimize method already has a
<remarks>block at lines 153-170. XML documentation allows only one<remarks>element per method. This duplicate will cause XML documentation compiler warnings (CS1570 or similar).This issue was previously flagged and marked as addressed, but appears to have been reintroduced.
🔎 Recommended fix
Merge the DataLoader integration paragraph into the existing
<remarks>block:/// <para><b>For Beginners:</b> This is the main learning process of the optimizer. /// /// Here's what happens: /// 1. It starts with a random guess for the best solution /// 2. In each step (iteration): /// - It calculates how to improve the current solution /// - It updates the solution using the AdaDelta method /// - It checks if the new solution is better than the previous best /// - It decides whether to stop early if the solution is good enough /// 3. It repeats this process until it reaches the maximum number of steps or finds a good enough solution /// /// This is like practicing a skill over and over, getting a little better each time, until you're satisfied with your performance. /// </para> + /// <para><b>DataLoader Integration:</b> This method uses the DataLoader API for efficient batch processing. + /// It creates a batcher using <see cref="GradientBasedOptimizerBase{T,TInput,TOutput}.CreateBatcher"/> + /// and notifies the sampler of epoch starts using + /// <see cref="GradientBasedOptimizerBase{T,TInput,TOutput}.NotifyEpochStart"/>. + /// </para> /// </remarks> - /// <remarks> - /// <para><b>DataLoader Integration:</b> This method uses the DataLoader API for efficient batch processing. - /// It creates a batcher using <see cref="GradientBasedOptimizerBase{T,TInput,TOutput}.CreateBatcher"/> - /// and notifies the sampler of epoch starts using - /// <see cref="GradientBasedOptimizerBase{T,TInput,TOutput}.NotifyEpochStart"/>. - /// </para> - /// </remarks> public override OptimizationResult<T, TInput, TOutput> Optimize(OptimizationInputData<T, TInput, TOutput> inputData)src/Data/Loaders/InMemoryDataLoader.cs (1)
240-257: DuplicateCastToDataTypehelper.This is the same helper duplicated in
DataAggregationHelper.csandOptimizationDataBatcher.cs. As noted earlier, consider centralizing this utility.src/Optimizers/OptimizationDataBatcher.cs (1)
271-281: DuplicateCastToDataTypehelper.Same pattern duplicated across multiple files.
🧹 Nitpick comments (9)
src/Optimizers/AdaDeltaOptimizer.cs (1)
198-199: Consider removing unnecessary intermediate variable.The
newSolutionvariable is created and immediately assigned tocurrentSolution. This can be simplified by directly assigning the result ofUpdateSolutiontocurrentSolution.🔎 Proposed simplification
foreach (var (xBatch, yBatch, batchIndices) in batcher.GetBatches()) { var gradient = CalculateGradient(currentSolution, xBatch, yBatch); - var newSolution = UpdateSolution(currentSolution, gradient); - currentSolution = newSolution; + currentSolution = UpdateSolution(currentSolution, gradient); }src/Data/Loaders/DataLoaderBase.cs (1)
95-99: Consider thread-safety consistency forBatchSizeproperty.Other mutable properties in this class (
IsLoaded,CurrentIndex,CurrentBatchIndex) use locking for thread-safe access. TheBatchSizeproperty's setter lacks similar protection, which could cause data races if modified during iteration.If
BatchSizeis intended to be set only during initialization, consider making the setterprotectedorinit-only. Otherwise, add locking for consistency.🔎 Option 1: Make setter init-only or protected
public virtual int BatchSize { get => _batchSize; - set => _batchSize = value; + protected set => _batchSize = value; }🔎 Option 2: Add locking for thread-safety
public virtual int BatchSize { - get => _batchSize; - set => _batchSize = value; + get + { + lock (_lock) + { + return _batchSize; + } + } + set + { + lock (_lock) + { + _batchSize = value; + } + } }src/Data/Sampling/WeightedSampler.cs (1)
56-112: Sampling implementation is correct but has O(n²) complexity for without-replacement mode.The without-replacement sampling recalculates the cumulative distribution for each sample drawn (line 77-82), resulting in O(n²) time complexity for n samples. This is acceptable for typical batch sizes but may become a bottleneck for very large datasets.
For performance-critical scenarios with large sample counts, consider using the alias method or reservoir sampling for O(n) complexity.
src/Helpers/DataAggregationHelper.cs (1)
82-92: Consider centralizing theCastToDataTypehelper.This same helper pattern appears in multiple files (
InMemoryDataLoader.cs,OptimizationDataBatcher.cs). Consider extracting it to a shared utility class to reduce duplication.🔎 Potential centralization
Create a shared helper in
src/Helpers/TypeCastHelper.cs:public static class TypeCastHelper { public static TTarget CastToDataType<TSource, TTarget>(TSource source) where TSource : class { object boxed = source; if (boxed is TTarget result) { return result; } throw new InvalidCastException($"Cannot cast {typeof(TSource).Name} to {typeof(TTarget).Name}"); } }src/Optimizers/OptimizationDataBatcher.cs (1)
86-97: Potential inconsistency betweenNumBatchesproperty andGetBatchesiteration.The
NumBatchesproperty (lines 90-96) andGetBatchesmethod (line 116) use slightly different calculations:
NumBatches:_dataSize / _batchSizeplus conditional incrementGetBatches:(_dataSize + _batchSize - 1) / _batchSize(ceiling division when!_dropLast)While mathematically equivalent, the differing implementations could diverge if modified independently. Consider extracting to a shared helper.
🔎 Proposed consolidation
+ private int CalculateNumBatches() + { + int batches = _dataSize / _batchSize; + if (!_dropLast && _dataSize % _batchSize > 0) + { + batches++; + } + return batches; + } + public int NumBatches { - get - { - int batches = _dataSize / _batchSize; - if (!_dropLast && _dataSize % _batchSize > 0) - { - batches++; - } - return batches; - } + get => CalculateNumBatches(); }Then use
CalculateNumBatches()inGetBatchesas well.Also applies to: 110-138
src/Data/Loaders/ParallelBatchLoader.cs (2)
166-177: Batch ordering is not guaranteed.With multiple workers dequeuing from a
ConcurrentQueueand writing to a channel, the output batch order may differ from the input index order. This is typically acceptable for ML training (where order doesn't matter) but should be documented.🔎 Suggested documentation addition
/// <summary> /// Iterates through batches using parallel workers. /// </summary> /// <param name="cancellationToken">Cancellation token.</param> /// <returns>Async enumerable of batches.</returns> + /// <remarks> + /// <b>Note:</b> Batch ordering is not guaranteed due to parallel processing. + /// This is typically acceptable for ML training where order doesn't affect results. + /// </remarks> public async IAsyncEnumerable<TBatch> GetBatchesAsync(
234-274: Configuration class properties are unused in current implementation.
ParallelBatchLoaderConfigdefinesPinMemory,WorkerTimeoutMs, andPersistentWorkersbut these are not used byParallelBatchLoader. Either implement these features or mark them as planned future functionality.🔎 Suggested documentation
/// <summary> /// Configuration for parallel batch loading. /// </summary> +/// <remarks> +/// <b>Note:</b> Some properties (PinMemory, WorkerTimeoutMs, PersistentWorkers) +/// are reserved for future implementation and are not currently used. +/// </remarks> public class ParallelBatchLoaderConfigsrc/Data/Loaders/RLDataLoaderBase.cs (1)
355-408: Method yields individual experiences, not batched data.The method is named
GetBatchesbut yields individualExperienceobjects (lines 403-406) rather than aggregated batch structures. This differs from other data loaders whereGetBatchesreturns aggregated batch data (matrices/tensors).This is intentional for RL (sampling from replay buffer), but the naming could be confusing. Consider documenting this distinction more prominently.
🔎 Suggested documentation enhancement
/// <remarks> /// <para> /// For RL data loaders, this method samples experiences from the replay buffer. /// Unlike supervised learning, RL uses random sampling from the buffer rather than /// sequential iteration, which helps break temporal correlations. /// </para> + /// <para> + /// <b>Important:</b> Unlike other data loaders, this method yields individual + /// <see cref="Experience{T, TState, TAction}"/> objects rather than aggregated + /// batch structures. Use <see cref="SampleBatch"/> for a single call returning + /// multiple experiences as a list. + /// </para>src/Data/Loaders/StreamingDataLoaderBase.cs (1)
127-128: Sync-over-async pattern may cause deadlocks in some contexts.Lines 127-128 use
GetAwaiter().GetResult()to synchronously block on async sample reads. While this pattern is sometimes necessary to provide synchronous enumeration, it can cause deadlocks in UI contexts or when the SynchronizationContext captures the continuation.Consider documenting this limitation in the XML docs for
GetBatches, noting that callers should preferGetBatchesAsyncwhen possible to avoid blocking.📝 Suggested documentation update
/// <inheritdoc/> +/// <remarks> +/// This method blocks on async operations using GetAwaiter().GetResult(). +/// Prefer <see cref="GetBatchesAsync"/> for non-blocking iteration. +/// </remarks> public virtual IEnumerable<(TInput[] Inputs, TOutput[] Outputs)> GetBatches(
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
src/Data/Loaders/DataLoaderBase.cssrc/Data/Loaders/EpisodicDataLoaderBase.cssrc/Data/Loaders/InMemoryDataLoader.cssrc/Data/Loaders/ParallelBatchLoader.cssrc/Data/Loaders/RLDataLoaderBase.cssrc/Data/Loaders/StreamingDataLoader.cssrc/Data/Loaders/StreamingDataLoaderBase.cssrc/Data/Pipeline/DataPipeline.cssrc/Data/Sampling/DataSamplerBase.cssrc/Data/Sampling/ImportanceSampler.cssrc/Data/Sampling/StratifiedSampler.cssrc/Data/Sampling/WeightedSampler.cssrc/Helpers/DataAggregationHelper.cssrc/Helpers/TensorCopyHelper.cssrc/Interfaces/IStreamingDataLoader.cssrc/Models/Options/ConjugateGradientOptimizerOptions.cssrc/Models/Options/DFPOptimizerOptions.cssrc/Optimizers/AMSGradOptimizer.cssrc/Optimizers/AdaDeltaOptimizer.cssrc/Optimizers/AdaMaxOptimizer.cssrc/Optimizers/AdagradOptimizer.cssrc/Optimizers/GradientDescentOptimizer.cssrc/Optimizers/LionOptimizer.cssrc/Optimizers/NadamOptimizer.cssrc/Optimizers/NesterovAcceleratedGradientOptimizer.cssrc/Optimizers/OptimizationDataBatcher.cssrc/Optimizers/RootMeanSquarePropagationOptimizer.cssrc/PredictionModelBuilder.cs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Models/Options/ConjugateGradientOptimizerOptions.cs
- src/PredictionModelBuilder.cs
- src/Data/Sampling/StratifiedSampler.cs
- src/Optimizers/AMSGradOptimizer.cs
🧰 Additional context used
🧠 Learnings (4)
📚 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/Optimizers/GradientDescentOptimizer.cssrc/Helpers/DataAggregationHelper.cssrc/Helpers/TensorCopyHelper.cssrc/Optimizers/AdaMaxOptimizer.cssrc/Optimizers/AdagradOptimizer.cssrc/Optimizers/LionOptimizer.cssrc/Optimizers/NesterovAcceleratedGradientOptimizer.cssrc/Data/Loaders/DataLoaderBase.cssrc/Data/Sampling/WeightedSampler.cssrc/Optimizers/NadamOptimizer.cssrc/Optimizers/OptimizationDataBatcher.cssrc/Optimizers/AdaDeltaOptimizer.cssrc/Models/Options/DFPOptimizerOptions.cssrc/Data/Loaders/RLDataLoaderBase.cssrc/Optimizers/RootMeanSquarePropagationOptimizer.cssrc/Data/Loaders/InMemoryDataLoader.cssrc/Interfaces/IStreamingDataLoader.cssrc/Data/Loaders/EpisodicDataLoaderBase.cssrc/Data/Loaders/StreamingDataLoader.cssrc/Data/Sampling/DataSamplerBase.cssrc/Data/Sampling/ImportanceSampler.cssrc/Data/Pipeline/DataPipeline.cssrc/Data/Loaders/ParallelBatchLoader.cssrc/Data/Loaders/StreamingDataLoaderBase.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/Optimizers/GradientDescentOptimizer.cssrc/Helpers/DataAggregationHelper.cssrc/Helpers/TensorCopyHelper.cssrc/Optimizers/AdaMaxOptimizer.cssrc/Optimizers/AdagradOptimizer.cssrc/Optimizers/LionOptimizer.cssrc/Optimizers/NesterovAcceleratedGradientOptimizer.cssrc/Data/Loaders/DataLoaderBase.cssrc/Data/Sampling/WeightedSampler.cssrc/Optimizers/NadamOptimizer.cssrc/Optimizers/OptimizationDataBatcher.cssrc/Optimizers/AdaDeltaOptimizer.cssrc/Models/Options/DFPOptimizerOptions.cssrc/Data/Loaders/RLDataLoaderBase.cssrc/Optimizers/RootMeanSquarePropagationOptimizer.cssrc/Data/Loaders/InMemoryDataLoader.cssrc/Interfaces/IStreamingDataLoader.cssrc/Data/Loaders/EpisodicDataLoaderBase.cssrc/Data/Loaders/StreamingDataLoader.cssrc/Data/Sampling/DataSamplerBase.cssrc/Data/Sampling/ImportanceSampler.cssrc/Data/Pipeline/DataPipeline.cssrc/Data/Loaders/ParallelBatchLoader.cssrc/Data/Loaders/StreamingDataLoaderBase.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/Helpers/DataAggregationHelper.cssrc/Data/Loaders/RLDataLoaderBase.cssrc/Data/Loaders/EpisodicDataLoaderBase.cs
📚 Learning: 2025-12-21T04:19:31.255Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 445
File: src/PredictionModelBuilder.cs:752-763
Timestamp: 2025-12-21T04:19:31.255Z
Learning: Federated preprocessing invariant: When using IFederatedClientDataLoader in PredictionModelBuilder.BuildSupervisedInternalAsync (src/PredictionModelBuilder.cs), any preprocessing must preserve per-client row ordering and total sample counts. The code now enforces X/Y alignment and total count checks after PreprocessData when federated client ranges are used, failing fast with a clear exception if violated.
Applied to files:
src/Data/Loaders/StreamingDataLoader.cssrc/Data/Loaders/ParallelBatchLoader.cssrc/Data/Loaders/StreamingDataLoaderBase.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 (31)
src/Models/Options/DFPOptimizerOptions.cs (2)
22-25: Documentation improvement looks good.The previous documentation contradiction has been properly addressed. The
<value>tag now clearly states "A positive integer for mini-batch size, or -1 for full batch (default)" which resolves the earlier confusion.
26-32: Verify that DFP optimizer implementation properly handles mini-batches.The remarks explicitly warn that "Using mini-batches would introduce noise that disrupts the Hessian approximation." This raises a concern: if mini-batches are fundamentally problematic for DFP's algorithmic correctness, the implementation should either:
- Validate and restrict
BatchSizeto -1 (full batch only), or- Implement sophisticated handling to make mini-batches work correctly despite the noise
Please verify which approach is taken in the DFP optimizer implementation.
src/Optimizers/LionOptimizer.cs (2)
110-114: LGTM!The documentation clearly describes the DataLoader integration and properly references the relevant methods. The duplicate
<remarks>issue from the previous review has been addressed.
141-156: LGTM!The evaluation and early-stopping logic is correctly structured:
- Evaluation occurs after all batches complete using the full dataset
- Early stopping and convergence checks operate on epoch boundaries, which is appropriate for batch-based training
src/Optimizers/AdaDeltaOptimizer.cs (1)
193-200: Verify batcher disposal to prevent potential resource leaks.The
batcherinstance is created on line 193 but not explicitly disposed. IfCreateBatcherreturns a type implementingIDisposable(which is likely given the PR context mentioning memory-mapped files, SemaphoreSlim, and other resources in dataloader implementations), this could leak resources such as file handles, semaphores, or unmanaged memory.Consider wrapping the batcher in a
usingstatement:var batcher = CreateBatcher(inputData, _options.BatchSize); + using var batcher = CreateBatcher(inputData, _options.BatchSize);src/Optimizers/AdaMaxOptimizer.cs (1)
198-216: Epoch-based batching integration looks correct.The optimization loop correctly:
- Notifies epoch start before processing batches
- Creates a batcher per epoch
- Increments
_tper batch (appropriate for AdaMax's bias correction)- Updates
currentSolutionincrementally within the batch loop- Evaluates against full dataset after all batches complete
src/Data/Loaders/DataLoaderBase.cs (1)
36-43: Constructor initialization is clean.The protected constructor with a sensible default batch size (32) provides a good foundation for derived classes.
src/Optimizers/GradientDescentOptimizer.cs (1)
86-110: Epoch-based batch processing correctly implemented.The refactored optimization loop properly:
- Creates batchers per epoch for shuffling
- Applies momentum per batch
- Updates solution incrementally
- Evaluates on full dataset after epoch completion
src/Helpers/TensorCopyHelper.cs (1)
64-68: Index calculation assumes identical shapes.Line 67 uses
dest.Shape[d]while line 68 usessource.Shape[d]for the same logical operation. If shapes differ (which the validation above would prevent), this creates inconsistent behavior. With matching shapes, the current code works correctly.src/Optimizers/AdagradOptimizer.cs (1)
154-183: Adagrad batch processing implementation is correct.The epoch-based refactoring properly:
- Updates accumulated squared gradients per batch (maintaining Adagrad's adaptive learning rate behavior)
- Applies the tolerance-based convergence check correctly after epoch evaluation
- Maintains consistency with other optimizer implementations in this PR
src/Optimizers/NadamOptimizer.cs (1)
134-163: Nadam batch processing implementation is correct.The epoch-based optimization:
- Correctly increments
_tper batch for proper bias correction in moment estimates- Updates momentum (
_m) and variance (_v) vectors throughUpdateSolutionper batch- Maintains Nesterov lookahead semantics within the batched flow
src/Optimizers/RootMeanSquarePropagationOptimizer.cs (1)
179-209: RMSProp batch processing implementation is correct.The epoch-based refactoring properly updates the squared gradient moving average per batch through
UpdateSolution, maintaining RMSProp's adaptive learning rate behavior.src/Optimizers/NesterovAcceleratedGradientOptimizer.cs (1)
118-148: NAG batch processing implementation is correct.The epoch-based refactoring properly maintains NAG's lookahead semantics:
- Computes lookahead position per batch
- Evaluates gradient at the lookahead position
- Updates velocity and solution incrementally
The implementation correctly preserves the Nesterov momentum advantage within the batched training flow.
src/Data/Sampling/WeightedSampler.cs (2)
1-53: LGTM! Clean implementation with proper delegation.The constructor correctly delegates validation to the base class, and the class structure follows good OOP principles with clear separation of concerns.
129-160: Well-implemented class balancing with proper overflow protection.The
CreateBalancedWeightsmethod correctly:
- Uses explicit
.Where()filtering for valid labels (line 133)- Casts to
doublebefore multiplication to prevent integer overflow (line 145)- Uses ternary expressions for concise conditional assignment (lines 144-146, 154-156)
- Handles edge cases (zero class counts, invalid labels)
src/Helpers/DataAggregationHelper.cs (1)
37-73: Well-structured aggregation with proper validation and type handling.The method correctly handles:
- Empty list validation with clear exception message
- Single-item optimization (returns directly without copying)
- Type-specific delegation with clear fallback error message
Good defensive programming with the
NotSupportedExceptionfor unsupported types.src/Data/Loaders/InMemoryDataLoader.cs (1)
179-206: Good refactoring to use centralized helpers.The extraction of tensor copying to
TensorCopyHelper.CopySampleand the use ofCastToDataTypeimproves code consistency across the codebase. The implementation correctly handles both Matrix and Tensor input types.src/Data/Loaders/EpisodicDataLoaderBase.cs (4)
69-72: Good addition of thread-safety mechanism.The
_randomLockobject provides thread-safe access to the sharedRandomInstanceduring batch generation, addressing the concern about concurrent enumerations affecting shared state.
481-524: Well-implemented parameter validation for episodic loaders.The explicit validation of
shuffleanddropLastparameters with clear error messages properly documents that episodic loaders generate inherently random tasks. The lock-based thread-safety ensures reproducible behavior when a seed is provided.
583-610: Producer holds lock for entire task generation.The lock is held while generating all tasks (lines 588-604), which could block other callers. The inline comment (lines 599-601) justifies this as acceptable because task generation is fast compared to model training. This trade-off is reasonable for reproducibility.
However, consider that if
GetNextTaskCore()throws, the lock is still released correctly due to the try-finally structure.
612-622: Correct async enumeration pattern for .NET 4.7.1 compatibility.The consumer loop using
WaitToReadAsync+TryReadis the correct pattern for .NET Framework compatibility (noReadAllAsync). Theawait producerTaskat the end ensures any producer exceptions are propagated.src/Interfaces/IStreamingDataLoader.cs (1)
1-101: Well-designed streaming data loader interface.The interface provides a clean abstraction for streaming data with:
- Clear property/method separation (BatchSize as settable property)
- Both sync and async iteration patterns
- Proper documentation including "For Beginners" sections
- Correct example showing BatchSize property usage before iteration (lines 21-23)
The design aligns with PyTorch's DataLoader patterns while being idiomatic C#.
src/Optimizers/OptimizationDataBatcher.cs (3)
56-71: Solid constructor with proper validation.The constructor correctly validates inputs, throws appropriate exceptions for null/invalid arguments, and stores configuration immutably.
288-325: Good fluent API design for sampler configuration.The
WithSampler,WithClassBalancing, andWithCurriculumLearningmethods provide a clean fluent interface for configuring sampling strategies. Each method creates a new batcher instance (immutable pattern).
175-210: I cannot verify the review comment because I'm unable to access the codebase (repository clone failed) and the IDataSampler interface is not publicly documented.To properly assess whether this concern is valid, I would need:
- The
IDataSamplerinterface definition and its contract documentation- How the interface specifies the expected behavior of
GetIndices()(e.g., minimum/maximum indices, whether fewer indices than requested is acceptable)- Actual sampler implementations to understand real-world usage patterns
The logical concern about
.Take(_dataSize)silently truncating a smaller result set is sound, but I cannot determine whether this represents an actual bug or is expected behavior without understanding the interface contract.src/Data/Loaders/ParallelBatchLoader.cs (2)
88-104: Well-designed constructor with sensible defaults.The constructor properly validates inputs and provides reasonable defaults (processor count for workers, 2x workers for prefetch). The
Math.Max(1, ...)guards ensure minimum viable values.
154-219: Robust parallel worker implementation with proper error handling.The implementation correctly:
- Captures the first worker exception via
Interlocked.CompareExchange(line 187)- Completes the channel with the exception (line 195) - addresses past review
- Propagates exceptions after iteration completes (lines 215-218)
- Uses bounded channel to limit memory usage
- Handles cancellation appropriately
src/Data/Loaders/RLDataLoaderBase.cs (4)
53-56: Thread-safety improvements correctly address past review concerns.The
_randomLockaddition and its use inSetSeedensures thread-safe RNG reseeding, preventing race conditions when multiple threads access the loader.Also applies to: 276-280
314-337: Well-implemented thread-safe random action selection.The lock encompasses the entire action selection logic for both continuous and discrete action spaces, ensuring consistent behavior under concurrent access.
390-397: Edge case: last batch size calculation could yield zero or negative.Line 391 calculates
currentBatchSizefor the last batch, but ifbufferCount - batchIdx * effectiveBatchSizeis zero or negative (possible with certain buffer/batch size combinations), the condition on line 394 catches it. However, the calculation could be simplified.
423-513: Async implementation correctly mirrors sync version with prefetching.The async implementation properly:
- Uses bounded channel for backpressure
- Documents the seed parameter behavior (lines 462-464)
- Handles cancellation throughout
- Ensures producer completion and exception propagation
- Add column dimension validation to matrix aggregation - Add shape validation to tensor aggregation (rank and dimensions 1+) - Add comprehensive validation to TensorCopyHelper: - Validate tensor rank compatibility - Validate shape compatibility for non-sample dimensions - Validate source and destination indices are within bounds 🤖 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: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Helpers/DataAggregationHelper.cssrc/Helpers/TensorCopyHelper.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Helpers/TensorCopyHelper.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/Helpers/DataAggregationHelper.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/Helpers/DataAggregationHelper.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/Helpers/DataAggregationHelper.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 (2)
src/Helpers/DataAggregationHelper.cs (2)
101-110: Validation improvements successfully implemented.The matrix column validation (lines 101-110) and tensor shape/rank validation (lines 151-173) address the concerns from previous reviews. These defensive checks will catch shape mismatches early and provide clear error messages.
Also applies to: 151-173
187-187: Verify TensorCopyHelper.CopySample availability and method signature.The code at line 187 depends on
TensorCopyHelper.CopySample(tensor, result, s, currentSample++). Confirm this helper class exists in the codebase with the method signature matching this invocation.
Validate that all items in the list are of the same type before attempting to cast and aggregate. Prevents runtime cast exceptions when list contains mixed types. 🤖 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: 3
🧹 Nitpick comments (1)
src/Helpers/DataAggregationHelper.cs (1)
37-87: Excellent improvements addressing past feedback.The type homogeneity validation (lines 50-62) successfully addresses the past review comment about preventing runtime cast exceptions when aggregating mixed types. The implementation is thorough and provides clear error messages.
The overall structure is well-designed with appropriate early returns and delegation to type-specific aggregators.
💡 Optional: Consider explicit null validation for clearer error messages
Currently, if
itemscontains null elements, the validation at line 51 (items[0]?.GetType()) capturesnull, but the type checks (lines 65, 71, 77) will fail, eventually throwingNotSupportedExceptioninstead of a more specificArgumentException. While this does fail safely, explicitly validating for null items could provide clearer diagnostics:public static TData Aggregate<T, TData>(List<TData> items, string itemTypeName) { if (items.Count == 0) { throw new InvalidOperationException($"Cannot aggregate empty {itemTypeName} list."); } + + if (items.Any(item => item == null)) + { + throw new ArgumentException($"Cannot aggregate {itemTypeName}s containing null items."); + } // If items are already in the right format (single item)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Data/Loaders/DataLoaders.cssrc/Helpers/DataAggregationHelper.cssrc/Optimizers/AdamWOptimizer.cssrc/Optimizers/GradientBasedOptimizerBase.cssrc/PredictionModelBuilder.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Optimizers/GradientBasedOptimizerBase.cs
- src/PredictionModelBuilder.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/Optimizers/AdamWOptimizer.cssrc/Helpers/DataAggregationHelper.cssrc/Data/Loaders/DataLoaders.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/Optimizers/AdamWOptimizer.cssrc/Helpers/DataAggregationHelper.cssrc/Data/Loaders/DataLoaders.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/Helpers/DataAggregationHelper.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 (7)
src/Data/Loaders/DataLoaders.cs (1)
511-651: Verify duplicate method declarations and type existence.The review requires manual verification to confirm:
- Whether
Streaming,FromDirectory, andFromCsvfactory methods are duplicated elsewhere inDataLoaders.csor related files- Whether
StreamingDataLoader<T, TInput, TOutput>,FileStreamingDataLoader<T, TInput, TOutput>, andCsvStreamingDataLoader<T, TInput, TOutput>types are properly defined in the codebase before being instantiated in the factory methodssrc/Helpers/DataAggregationHelper.cs (2)
111-137: Matrix aggregation correctly validated.The column count validation (lines 115-124) successfully addresses the past review feedback, ensuring all matrices have compatible shapes before aggregation. The error message clearly identifies the mismatched matrix index and column counts.
161-205: Tensor aggregation with comprehensive shape validation.The shape and rank validation (lines 165-187) successfully addresses the past review feedback, ensuring tensors are compatible for concatenation along the first dimension. The validation is thorough, checking both rank equality and per-dimension matching with clear error messages.
The use of
TensorCopyHelper.CopySample(line 201) aligns well with the centralized tensor-copy utilities mentioned in the AI summary.src/Optimizers/AdamWOptimizer.cs (4)
148-156: LGTM! Clear documentation of DataLoader integration.The remarks effectively communicate the new batching capabilities and configuration options.
159-173: Initialization logic is sound.The moment vectors and adaptive parameters are properly initialized. The call to
InitializeAdaptiveParametersat line 172 resets learning rate and beta values at the start of optimization, which ensures consistent state across multipleOptimizecalls.
196-216: Epoch-level evaluation and early stopping are correctly implemented.Evaluating the solution after processing all batches (line 197) and checking convergence at epoch boundaries aligns with standard mini-batch training practices.
176-194: Batched training loop is well-structured.The epoch-level iteration with per-batch gradient updates correctly implements mini-batch AdamW training. Incrementing
_tper batch (line 186) is correct for Adam's bias correction, which should track the total number of gradient updates.Verify that
CreateBatcherandNotifyEpochStartmethods are available in the base classGradientBasedOptimizerBase.cs, as these are required for the code at lines 182 and 179 to function.
Add null checks and range validation to Streaming, FromDirectory, and FromCsv factory methods to match validation patterns in other factory methods in the DataLoaders class. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Production-Ready PR Checklist
Code Implementation
Testing
Documentation
Validation
Review Process
User Story / Context
master(default) or feature branch if stacking PRsSummary
Verification
Copilot Review Loop (Outcome-Based)
Record counts before/after your last push:
Files Modified
Performance Characteristics
Breaking Changes
Migration Guide
Security Considerations
Additional Context
Related Issues
Screenshots (if applicable)
Notes