feat: comprehensive dataset and dataloader abstractions (#282)

[ooples]

Summary

• 10-phase implementation of comprehensive Dataset/DataLoader abstractions for AiDotNet, covering transforms, checkpointing, domain loaders (image/audio/video/text), benchmark datasets (MNIST, CIFAR, Fashion-MNIST, IMDB), distributed sampling, collation, streaming formats (Parquet, WebDataset, JSONL, Sharded), multimodal datasets, pipeline persistence, and a unified DataLoaders factory API
• PNG/JPEG image support via SixLabors.ImageSharp (conditionally compiled for .NET 6+; net471 retains BMP/PPM/PGM only)
• Typed streaming data loader facades (WebDatasetDataLoader<T>, JsonlDataLoader<T>, ShardedStreamingDataLoader<T>) that implement IDataLoader<T> so streaming formats can be passed to AiModelBuilder.ConfigureDataLoader()

Test plan

• Verify dotnet build src/AiDotNet.csproj --framework net10.0 -c Release passes with 0 errors
• Verify dotnet build src/AiDotNet.csproj --framework net471 -c Release passes with 0 errors
• Verify ImageHelper<T>.LoadImage("test.png") routes through ImageSharp on .NET 10
• Verify typed loaders inherit from StreamingDataLoaderBase and satisfy IDataLoader<T>
• Verify DataLoaders.FromWebDataset<T>, FromJsonl<T>, FromShards<T> require parser delegates and return typed loaders

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Many new dataset loaders and factory helpers (image-folder, video-frames, audio-files, WebDataset, sharded streaming, JSONL, Parquet, memory-mapped, tokenized/text-line, CIFAR/MNIST/Fashion/IMDB), multimodal types and mixers, collate/padding/packing utilities, transforms, samplers, stateful & transformed loaders, and a snapshot pipeline with disk cache.

• Chores

  • Added Parquet support and conditional ImageSharp-based image handling on newer runtimes.

• Tests

  • Unit tests added for DynamicBatchSampler.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
aidotnet-playground-api	Ready	Preview, Comment	Feb 8, 2026 11:44pm

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Walkthrough

Adds a large data subsystem: many new dataset loaders (image, video, audio, text, multimodal), streaming/sharded/parquet readers, collate/sampler/transform primitives, pipeline snapshotting/disk cache and dataset downloader; plus project package updates for Parquet and conditional ImageSharp. (50 words)

Changes

Cohort / File(s)	Summary
Project file src/AiDotNet.csproj	Added Parquet.Net and conditional SixLabors.ImageSharp PackageReferences; preserved PostgreSQL EF conditional entry.
Audio src/Data/Audio/... src/Data/Audio/AudioFileDataset.cs, src/Data/Audio/AudioFileDatasetOptions.cs	New AudioFileDataset<T> + AudioFileDatasetOptions: WAV/PCM parsing, normalization, mono conversion, deterministic sampling, async load/unload, batching and Split API.
Vision & Image helper src/Data/Vision/..., src/Helpers/ImageHelper.cs	Added ImageFolderDataset<T>, ImageClassificationDataset<T> and options; ImageHelper includes ImageSharp loader under NET6+.
Video src/Data/Video/... src/Data/Video/VideoFrameDataset.cs, ...Options.cs	New VideoFrameDataset<T> + options: frame discovery, sampling per video, resizing, normalization, labels, and Split API.
Text & benchmarks src/Data/Text/...	Added TextLineDataset, TokenizedTextDataset, IMDB-50k loader and options: tokenization, vocab build, encoding, tensorization and Split support.
Vision Benchmarks src/Data/Vision/Benchmarks/*	New MNIST, Fashion-MNIST, CIFAR-10/100 loaders and options with download/parsing, normalization, one-hot labels, batching and Split methods.
Formats & streaming src/Data/Formats/*	New streaming/parsing formats and loaders: JsonlStreamingLoader/JsonlDataLoader, WebDataset/WebDatasetDataLoader, ShardedStreamingDataset/ShardedStreamingDataLoader, MemoryMappedDataset, ParquetDataLoader + options.
Collation src/Data/Collation/*	New ICollateFunction plus implementations: DefaultCollateFunction, PaddingCollateFunction, PackedSequenceCollateFunction and PackedSequenceBatch.
Sampling src/Data/Sampling/*	Added samplers: BucketBatchSampler, DistributedSampler, DistributedBucketSampler, DynamicBatchSampler (token-budget batching).
Transforms src/Data/Transforms/*	Added ITransform, Compose, IdentityTransform, LambdaTransform, and numeric transforms (MinMaxScaleTransform, StandardScaleTransform, NormalizeTransform, ToTensorTransform, OneHotEncodeTransform).
Multimodal src/Data/Multimodal/*	New multimodal primitives: ModalityType, ModalitySample, MultimodalSample, MultimodalDataset, DatasetMixer, interleaved sequences and mixers.
Loaders & wrappers src/Data/Loaders/*.cs	Expanded DataLoaders factory surface; added StatefulDataLoader (checkpoint/resume), TransformedDataLoader, many From* factory methods (Parquet/WebDataset/Jsonl/Shards/etc.).
Pipeline & downloader src/Data/Pipeline/*, src/Data/DatasetDownloader.cs	SnapshotPipeline<T> with DiskCacheOptions/CacheInfo, on-disk tensor cache (gzip/sha256), eviction/integrity; DatasetDownloader supports ZIP/tar.gz/gzip download+extract and manual tar parsing.
Interfaces & state src/Interfaces/IStatefulDataLoader.cs	Added IStatefulDataLoader<T> and DataLoaderCheckpoint for checkpoint/resume state.
Streaming loader wrappers (caching) src/Data/Formats/...DataLoader.cs	Streaming wrappers that cache parsed records and expose async sample reads via user parsers: JsonlDataLoader, ShardedStreamingDataLoader, WebDatasetDataLoader.
Misc options & helpers multiple src/Data/* option classes and helpers	Many new option classes (image/video/audio/webdataset/shards/disk cache), CacheInfo, and utilities.
Tests tests/AiDotNet.Tests/...	Added unit tests for DynamicBatchSampler.

Sequence Diagram(s)

sequenceDiagram
    rect rgba(200,220,255,0.5)
    participant Client
    participant SnapshotPipeline
    participant SourcePipeline
    participant FileSystem
    end

    Client->>SnapshotPipeline: GetCachedPipeline()
    SnapshotPipeline->>FileSystem: Check ActiveCacheDirectory / metadata
    alt Cache valid
        FileSystem-->>SnapshotPipeline: Cache valid
        SnapshotPipeline-->>Client: Return pipeline reading from cache
    else Cache missing/invalid
        SnapshotPipeline->>SourcePipeline: Enumerate samples/tensors
        SourcePipeline-->>SnapshotPipeline: Yield tensors/samples
        SnapshotPipeline->>FileSystem: Write cache files (tensor, gzip, sha256)
        FileSystem-->>SnapshotPipeline: Confirm writes
        SnapshotPipeline-->>Client: Return pipeline reading from new cache
    end

Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related PRs

• feat: dataloader batching utilities #570 — Strong overlap on samplers, streaming loaders and batching utilities; likely interacts with the new sampling/streaming components.

Poem

🎉 New loaders arrive, archives unwind,

Shards and parquet rows neatly aligned.
Collate and shuffle, pack, then stream,
Snapshot caches each epoch's dream.
Tensors hum — pipelines redeemed.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

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

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'feat: comprehensive dataset and dataloader abstractions' clearly and concisely summarizes the main change: introducing extensive dataset/dataloader abstractions across multiple modalities and formats.

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

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feat/dataset-dataloader-abstractions-282

---

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

[coderabbitai]

Actionable comments posted: 2

Note

Due to the large number of review comments, Critical severity comments were prioritized as inline comments.

🤖 Fix all issues with AI agents

In `@src/Data/DatasetDownloader.cs`:
- Around line 181-188: The current path traversal check using
fullPath.StartsWith(normalizedExtractDir) is bypassable (e.g., "/data_evil"
matches "/data"); fix it by normalizing the extract directory to include a
trailing directory separator and compare against that boundary: compute
normalizedExtractDirTrimmed =
normalizedExtractDir.TrimEnd(Path.DirectorySeparatorChar,
Path.AltDirectorySeparatorChar) + Path.DirectorySeparatorChar and then check if
fullPath.StartsWith(normalizedExtractDirTrimmed,
StringComparison.OrdinalIgnoreCase); if it does not, call SkipTarEntry(gzStream,
size) and continue as before. Ensure you use Path.DirectorySeparatorChar and
Path.AltDirectorySeparatorChar when trimming/adding the separator so the check
works across platforms.

In `@src/Data/Loaders/TransformedDataLoader.cs`:
- Around line 110-121: The Split method currently returns the raw splits from
_inner without applying the transform; update Split to wrap each returned loader
with the same transform before returning — e.g., after var (train, val, test) =
_inner.Split(...), return new TransformedDataLoader<T>(train, _transform), new
TransformedDataLoader<T>(val, _transform), new TransformedDataLoader<T>(test,
_transform) (or use the class's existing helper/wrap method if present) so
Train/Validation/Test are transformed loaders.

🟠 Major comments (64)

src/Data/Text/TextLineDataset.cs-19-37 (1)

19-37: ⚠️ Potential issue | 🟠 Major

Reduce public API surface to preserve the facade pattern.

This class is public in src/**, which exposes a new API outside the facade. Consider making it internal and expose access via AiModelBuilder if needed.

🛡️ Proposed change

-public class TextLineDataset<T> : DataLoaderBase<T>
+internal class TextLineDataset<T> : DataLoaderBase<T>

As per coding guidelines, “Users should ONLY interact with AiModelBuilder.cs and AiModelResult.cs” and “Prefer internal over public for all classes.”

src/Data/Text/TokenizedTextDataset.cs-24-24 (1)

24-24: ⚠️ Potential issue | 🟠 Major

Reduce public API surface for TokenizedTextDataset.

This public type expands the surface outside the facade. Unless it’s explicitly exposed via AiModelBuilder / AiModelResult, prefer internal and keep users on the facade path.

🔧 Suggested change

-public class TokenizedTextDataset<T> : InputOutputDataLoaderBase<T, Tensor<T>, Tensor<T>>
+internal class TokenizedTextDataset<T> : InputOutputDataLoaderBase<T, Tensor<T>, Tensor<T>>

As per coding guidelines: “Users should ONLY interact with AiModelBuilder.cs and AiModelResult.cs” and “Prefer internal over public for all classes, methods, and properties unless they are part of the facade API.”

src/Data/Text/TokenizedTextDataset.cs-65-79 (1)

65-79: ⚠️ Potential issue | 🟠 Major

Validate labels are non-negative before computing _numClasses.

Negative labels silently produce all‑zero rows and can yield an invalid OutputDimension. Reject them up front.

🧷 Suggested fix

-    _sequenceLength = sequenceLength;
-    _numClasses = labels.Length > 0 ? labels.Max() + 1 : 0;
+    if (labels.Any(l => l < 0))
+    {
+        throw new ArgumentOutOfRangeException(nameof(labels), "Labels must be non-negative.");
+    }
+
+    _sequenceLength = sequenceLength;
+    _numClasses = labels.Length > 0 ? labels.Max() + 1 : 0;

src/Data/Text/TokenizedTextDataset.cs-165-174 (1)

165-174: ⚠️ Potential issue | 🟠 Major

Remove null-forgiving operators in Split.

The project rules prohibit null-forgiving; rely on explicit null checks instead.

✅ Suggested fix

-    return (
-        new InMemoryDataLoader<T, Tensor<T>, Tensor<T>>(
-            ExtractTensorBatch(LoadedFeatures!, shuffled.Take(trainSize).ToArray()),
-            ExtractTensorBatch(LoadedLabels!, shuffled.Take(trainSize).ToArray())),
-        new InMemoryDataLoader<T, Tensor<T>, Tensor<T>>(
-            ExtractTensorBatch(LoadedFeatures!, shuffled.Skip(trainSize).Take(valSize).ToArray()),
-            ExtractTensorBatch(LoadedLabels!, shuffled.Skip(trainSize).Take(valSize).ToArray())),
-        new InMemoryDataLoader<T, Tensor<T>, Tensor<T>>(
-            ExtractTensorBatch(LoadedFeatures!, shuffled.Skip(trainSize + valSize).ToArray()),
-            ExtractTensorBatch(LoadedLabels!, shuffled.Skip(trainSize + valSize).ToArray()))
-    );
+    var features = LoadedFeatures ?? throw new InvalidOperationException("Features not loaded.");
+    var labels = LoadedLabels ?? throw new InvalidOperationException("Labels not loaded.");
+
+    return (
+        new InMemoryDataLoader<T, Tensor<T>, Tensor<T>>(
+            ExtractTensorBatch(features, shuffled.Take(trainSize).ToArray()),
+            ExtractTensorBatch(labels, shuffled.Take(trainSize).ToArray())),
+        new InMemoryDataLoader<T, Tensor<T>, Tensor<T>>(
+            ExtractTensorBatch(features, shuffled.Skip(trainSize).Take(valSize).ToArray()),
+            ExtractTensorBatch(labels, shuffled.Skip(trainSize).Take(valSize).ToArray())),
+        new InMemoryDataLoader<T, Tensor<T>, Tensor<T>>(
+            ExtractTensorBatch(features, shuffled.Skip(trainSize + valSize).ToArray()),
+            ExtractTensorBatch(labels, shuffled.Skip(trainSize + valSize).ToArray()))
+    );

src/Data/Pipeline/SnapshotPipeline.cs-515-540 (1)

515-540: 🛠️ Refactor suggestion | 🟠 Major

Move CacheInfo to its own file per "one class per file" rule.

The PR objectives specify "one class per file" as an architectural requirement. CacheInfo should be extracted to src/Data/Pipeline/CacheInfo.cs.

src/Data/Pipeline/SnapshotPipeline.cs-400-408 (1)

400-408: ⚠️ Potential issue | 🟠 Major

Critical: Default pipeline hash includes timestamp, defeating cache reuse.

When pipelineId is null, ComputePipelineHash() incorporates DateTime.UtcNow.Ticks, producing a unique hash on every instantiation. This means:

1. Each new SnapshotPipeline instance creates a new cache directory
2. Existing caches are never discovered or reused
3. The cache directory fills with orphaned caches

The comment acknowledges this ("users should provide a meaningful pipelineId"), but this default behavior is a pitfall that will silently waste disk space and provide no caching benefit.

Suggested fix: Compute deterministic hash or require pipelineId

 private string ComputePipelineHash()
 {
-    // Hash based on pipeline type and timestamp for uniqueness
-    // In practice, users should provide a meaningful pipelineId
-    string input = $"{typeof(T).FullName}_{_sourcePipeline.GetType().FullName}_{DateTime.UtcNow.Ticks}";
+    // Hash based on pipeline type for basic deduplication
+    // For reliable caching, users should provide a meaningful pipelineId
+    string input = $"{typeof(T).FullName}_{_sourcePipeline.GetType().FullName}";
     using var sha = SHA256.Create();
     byte[] hash = sha.ComputeHash(System.Text.Encoding.UTF8.GetBytes(input));
     return Convert.ToBase64String(hash).Replace("/", "_").Replace("+", "-").Substring(0, 16);
 }

Alternatively, throw if pipelineId is null to force explicit configuration:

_pipelineHash = pipelineId ?? throw new ArgumentNullException(nameof(pipelineId), 
    "A unique pipelineId is required for cache identification. " +
    "Provide a stable identifier based on your pipeline configuration.");

src/Data/Sampling/BucketBatchSampler.cs-18-30 (1)

18-30: ⚠️ Potential issue | 🟠 Major

Reduce public API surface for sampler types.

BucketBatchSampler is public and exposes implementation details outside the facade. Consider making it internal unless it is explicitly part of the facade API. As per coding guidelines, "Users should ONLY interact with AiModelBuilder.cs and AiModelResult.cs" and "Prefer internal over public for all classes, methods, and properties unless they are part of the facade API."

src/Data/Sampling/DistributedBucketSampler.cs-15-33 (1)

15-33: ⚠️ Potential issue | 🟠 Major

Reduce public API surface for sampler types.

DistributedBucketSampler is public and exposes implementation details outside the facade. Consider making it internal unless it is part of the facade API. As per coding guidelines, "Users should ONLY interact with AiModelBuilder.cs and AiModelResult.cs" and "Prefer internal over public for all classes, methods, and properties unless they are part of the facade API."

src/Data/Sampling/BucketBatchSampler.cs-30-31 (1)

30-31: ⚠️ Potential issue | 🟠 Major

Length overstates yielded indices when DropLast is enabled.

Dropping remainders per bucket means fewer indices are yielded than Length reports, which can skew epoch sizing or progress reporting.

Also applies to: 129-140

src/Data/Sampling/DynamicBatchSampler.cs-19-34 (1)

19-34: ⚠️ Potential issue | 🟠 Major

Reduce public API surface for sampler types.

DynamicBatchSampler is public and expands the API outside the facade; consider making it internal (and exposing it via AiModelBuilder/AiModelResult only if needed). As per coding guidelines, "Users should ONLY interact with AiModelBuilder.cs and AiModelResult.cs" and "Prefer internal over public for all classes, methods, and properties unless they are part of the facade API."

src/Data/Sampling/DistributedSampler.cs-19-35 (1)

19-35: ⚠️ Potential issue | 🟠 Major

Reduce public API surface for sampler types.

DistributedSampler is public; unless it is part of the facade, prefer internal to keep the API surface tight. As per coding guidelines, "Users should ONLY interact with AiModelBuilder.cs and AiModelResult.cs" and "Prefer internal over public for all classes, methods, and properties unless they are part of the facade API."

src/Data/Sampling/DistributedBucketSampler.cs-31-33 (1)

31-33: ⚠️ Potential issue | 🟠 Major

Length can over-report when DropLast is enabled.

When remainder batches are dropped, fewer indices are yielded than Length reports.

Also applies to: 156-166

src/Data/Sampling/DynamicBatchSampler.cs-30-34 (1)

30-34: ⚠️ Potential issue | 🟠 Major

Length can over-report when DropLast is true.

When the last batch is dropped, fewer indices are yielded than Length reports. If consumers use Length for epoch sizing, consider computing an effective length (or explicitly documenting that Length returns dataset size regardless of DropLast).

Also applies to: 109-113

src/Data/Sampling/DistributedBucketSampler.cs-56-64 (1)

56-64: ⚠️ Potential issue | 🟠 Major

Validate numBuckets to prevent divide-by-zero.

numBuckets is never checked; passing 0 will throw during bucket sizing.

Proposed fix

         if (rank < 0 || rank >= numReplicas)
             throw new ArgumentOutOfRangeException(nameof(rank), $"Rank must be in [0, {numReplicas - 1}].");
         if (batchSize < 1)
             throw new ArgumentOutOfRangeException(nameof(batchSize), "Batch size must be at least 1.");
+        if (numBuckets < 1)
+            throw new ArgumentOutOfRangeException(nameof(numBuckets), "Number of buckets must be at least 1.");

src/AiDotNet.csproj-74-74 (1)

74-74: ⚠️ Potential issue | 🟠 Major

Upgrade Parquet.Net to a more recent version—4.24.0 is outdated.

Version 4.24.0 (Jun 2024) is significantly behind the current release (5.5.0 as of Feb 2026). More importantly, it depends on System.Text.Json >= 8.0.3, which carries known DoS vulnerabilities (CVE-2024-30105, CVE-2024-43485) patched in 8.0.5+. Upgrade to 5.5.0 (latest) or 5.4.0 (latest 2025 stable) to mitigate transitive dependency risk and stay current with improvements.

src/Interfaces/IStatefulDataLoader.cs-17-30 (1)

17-30: ⚠️ Potential issue | 🟠 Major

Keep IStatefulDataLoader<T> and DataLoaderCheckpoint internal.

These types expand the public API beyond the facade and interfaces are intended for internal extensibility.

🛠️ Proposed change

-public interface IStatefulDataLoader<T> : IDataLoader<T>
+internal interface IStatefulDataLoader<T> : IDataLoader<T>

-public class DataLoaderCheckpoint
+internal class DataLoaderCheckpoint

As per coding guidelines, “Users should ONLY interact with `AiModelBuilder.cs` and `AiModelResult.cs`” and “Most interfaces should be `internal` unless they need to be implemented by library consumers.”

Also applies to: 41-87

src/Data/Formats/JsonlStreamingLoader.cs-20-21 (1)

20-21: ⚠️ Potential issue | 🟠 Major

Keep JsonlStreamingLoader internal per the facade pattern.

This class should not be part of the public surface.

🛠️ Proposed change

-public class JsonlStreamingLoader : IDisposable
+internal class JsonlStreamingLoader : IDisposable

As per coding guidelines, “Users should ONLY interact with `AiModelBuilder.cs` and `AiModelResult.cs`” and “Prefer `internal` over `public` for all classes, methods, and properties unless they are part of the facade API.”

src/Data/Transforms/Numeric/StandardScaleTransform.cs-17-18 (1)

17-18: ⚠️ Potential issue | 🟠 Major

Keep StandardScaleTransform<T> internal per the facade pattern.

This class is public but is not part of the facade API.

🛠️ Proposed change

-public class StandardScaleTransform<T> : ITransform<T[], T[]>
+internal class StandardScaleTransform<T> : ITransform<T[], T[]>

As per coding guidelines, “Users should ONLY interact with `AiModelBuilder.cs` and `AiModelResult.cs`” and “Prefer `internal` over `public` for all classes, methods, and properties unless they are part of the facade API.”

src/Data/Collation/PackedSequenceCollateFunction.cs-19-33 (1)

19-33: ⚠️ Potential issue | 🟠 Major

Make packed sequence collation types internal.

Both classes are internal implementation details and should not be public.

🛠️ Proposed change

-public class PackedSequenceCollateFunction<T> : ICollateFunction<Tensor<T>, PackedSequenceBatch<T>>
+internal class PackedSequenceCollateFunction<T> : ICollateFunction<Tensor<T>, PackedSequenceBatch<T>>

-public class PackedSequenceBatch<T>
+internal class PackedSequenceBatch<T>

As per coding guidelines, “Users should ONLY interact with `AiModelBuilder.cs` and `AiModelResult.cs`” and “Prefer `internal` over `public` for all classes, methods, and properties unless they are part of the facade API.”

Also applies to: 83-113

src/Data/Collation/PaddingCollateFunction.cs-18-34 (1)

18-34: ⚠️ Potential issue | 🟠 Major

Make PaddingCollateFunction<T> internal to preserve the facade-only public surface.

This is an internal implementation detail and shouldn’t be public.

🛠️ Proposed change

-public class PaddingCollateFunction<T> : ICollateFunction<Tensor<T>, Tensor<T>>
+internal class PaddingCollateFunction<T> : ICollateFunction<Tensor<T>, Tensor<T>>

As per coding guidelines, “Users should ONLY interact with `AiModelBuilder.cs` and `AiModelResult.cs`” and “Prefer `internal` over `public` for all classes, methods, and properties unless they are part of the facade API.”

src/Data/Transforms/Numeric/StandardScaleTransform.cs-27-53 (1)

27-53: ⚠️ Potential issue | 🟠 Major

Validate reference rows for nulls and consistent feature length.

The constructor assumes every row exists and matches referenceData[0].Length; inconsistent data will throw IndexOutOfRangeException.

🛠️ Proposed change

-        int featureCount = referenceData[0].Length;
+        if (referenceData[0] is null)
+        {
+            throw new ArgumentException("Reference data cannot contain null rows.", nameof(referenceData));
+        }
+
+        int featureCount = referenceData[0].Length;
+        for (int i = 1; i < referenceData.Length; i++)
+        {
+            if (referenceData[i] is null)
+            {
+                throw new ArgumentException("Reference data cannot contain null rows.", nameof(referenceData));
+            }
+            if (referenceData[i].Length != featureCount)
+            {
+                throw new ArgumentException("All reference rows must have the same length.", nameof(referenceData));
+            }
+        }

src/Data/Vision/Benchmarks/MnistDataLoaderOptions.cs-8-38 (1)

8-38: ⚠️ Potential issue | 🟠 Major

Make MnistDataLoaderOptions internal to keep the facade-only API.

This options type is public and expands the surface area outside the facade. Consider keeping it internal and expose configuration via AiModelBuilder if needed.

🛠️ Proposed change

-public sealed class MnistDataLoaderOptions
+internal sealed class MnistDataLoaderOptions

As per coding guidelines, “Users should ONLY interact with `AiModelBuilder.cs` and `AiModelResult.cs`” and “Prefer `internal` over `public` for all classes, methods, and properties unless they are part of the facade API.”

src/Data/Vision/Benchmarks/Cifar10DataLoaderOptions.cs-8-8 (1)

8-8: ⚠️ Potential issue | 🟠 Major

Keep loader options internal unless surfaced via the facade.

This options type is public but not part of AiModelBuilder/AiModelResult. Prefer internal and expose via the facade if needed.

🔧 Suggested change

-public sealed class Cifar10DataLoaderOptions
+internal sealed class Cifar10DataLoaderOptions

As per coding guidelines: “Users should ONLY interact with `AiModelBuilder.cs` and `AiModelResult.cs`” and “Prefer `internal` over `public` for all classes, methods, and properties unless they are part of the facade API.”

src/Data/Formats/ParquetDataLoader.cs-62-62 (1)

62-62: ⚠️ Potential issue | 🟠 Major

Limit ParquetDataLoader to internal API surface.

This loader should be internal unless routed through the facade.

🔧 Suggested change

-public class ParquetDataLoader<T> : InputOutputDataLoaderBase<T, Tensor<T>, Tensor<T>>
+internal class ParquetDataLoader<T> : InputOutputDataLoaderBase<T, Tensor<T>, Tensor<T>>

As per coding guidelines: “Users should ONLY interact with `AiModelBuilder.cs` and `AiModelResult.cs`” and “Prefer `internal` over `public` for all classes, methods, and properties unless they are part of the facade API.”

src/Data/Formats/MemoryMappedDataset.cs-34-34 (1)

34-34: ⚠️ Potential issue | 🟠 Major

Constrain MemoryMappedDataset to internal API surface.

This type should be internal unless it is intentionally exposed via the facade.

🔧 Suggested change

-public class MemoryMappedDataset<T> : IDisposable
+internal class MemoryMappedDataset<T> : IDisposable

As per coding guidelines: “Users should ONLY interact with `AiModelBuilder.cs` and `AiModelResult.cs`” and “Prefer `internal` over `public` for all classes, methods, and properties unless they are part of the facade API.”

src/Data/Loaders/StatefulDataLoader.cs-33-37 (1)

33-37: ⚠️ Potential issue | 🟠 Major

Keep StatefulDataLoader internal unless exposed via the facade.

This public wrapper expands the API surface beyond the facade.

🔧 Suggested change

-public class StatefulDataLoader<T, TInput, TOutput> :
+internal class StatefulDataLoader<T, TInput, TOutput> :

As per coding guidelines: “Users should ONLY interact with `AiModelBuilder.cs` and `AiModelResult.cs`” and “Prefer `internal` over `public` for all classes, methods, and properties unless they are part of the facade API.”

src/Data/Vision/Benchmarks/Cifar10DataLoader.cs-17-17 (1)

17-17: ⚠️ Potential issue | 🟠 Major

Restrict CIFAR-10 loader to internal API surface.

This public loader expands the API beyond the facade; please make it internal unless surfaced via AiModelBuilder/AiModelResult.

🔧 Suggested change

-public class Cifar10DataLoader<T> : InputOutputDataLoaderBase<T, Tensor<T>, Tensor<T>>
+internal class Cifar10DataLoader<T> : InputOutputDataLoaderBase<T, Tensor<T>, Tensor<T>>

As per coding guidelines: “Users should ONLY interact with `AiModelBuilder.cs` and `AiModelResult.cs`” and “Prefer `internal` over `public` for all classes, methods, and properties unless they are part of the facade API.”

src/Data/Formats/ParquetDataLoader.cs-11-11 (1)

11-11: ⚠️ Potential issue | 🟠 Major

Keep Parquet options internal unless exposed through the facade.

This public options type expands the API surface beyond the facade.

🔧 Suggested change

-public sealed class ParquetDataLoaderOptions
+internal sealed class ParquetDataLoaderOptions

As per coding guidelines: “Users should ONLY interact with `AiModelBuilder.cs` and `AiModelResult.cs`” and “Prefer `internal` over `public` for all classes, methods, and properties unless they are part of the facade API.”

src/Data/Vision/Benchmarks/Cifar100DataLoader.cs-16-16 (1)

16-16: ⚠️ Potential issue | 🟠 Major

Restrict CIFAR-100 loader to internal API surface.

This class is public but does not appear to be part of the facade API. Please make it internal or route access via AiModelBuilder/AiModelResult.

🔧 Suggested change

-public class Cifar100DataLoader<T> : InputOutputDataLoaderBase<T, Tensor<T>, Tensor<T>>
+internal class Cifar100DataLoader<T> : InputOutputDataLoaderBase<T, Tensor<T>, Tensor<T>>

As per coding guidelines: “Users should ONLY interact with `AiModelBuilder.cs` and `AiModelResult.cs`” and “Prefer `internal` over `public` for all classes, methods, and properties unless they are part of the facade API.”

src/Data/Vision/Benchmarks/Cifar10DataLoader.cs-159-167 (1)

159-167: ⚠️ Potential issue | 🟠 Major

Avoid null-forgiving in Split.

Use explicit null checks after EnsureLoaded() instead of LoadedFeatures!/LoadedLabels!.

🔧 Suggested change

-        return (
-            new InMemoryDataLoader<T, Tensor<T>, Tensor<T>>(
-                ExtractTensorBatch(LoadedFeatures!, shuffled.Take(trainSize).ToArray()),
-                ExtractTensorBatch(LoadedLabels!, shuffled.Take(trainSize).ToArray())),
-            new InMemoryDataLoader<T, Tensor<T>, Tensor<T>>(
-                ExtractTensorBatch(LoadedFeatures!, shuffled.Skip(trainSize).Take(valSize).ToArray()),
-                ExtractTensorBatch(LoadedLabels!, shuffled.Skip(trainSize).Take(valSize).ToArray())),
-            new InMemoryDataLoader<T, Tensor<T>, Tensor<T>>(
-                ExtractTensorBatch(LoadedFeatures!, shuffled.Skip(trainSize + valSize).ToArray()),
-                ExtractTensorBatch(LoadedLabels!, shuffled.Skip(trainSize + valSize).ToArray()))
-        );
+        var features = LoadedFeatures ?? throw new InvalidOperationException("Features not loaded.");
+        var labels = LoadedLabels ?? throw new InvalidOperationException("Labels not loaded.");
+        return (
+            new InMemoryDataLoader<T, Tensor<T>, Tensor<T>>(
+                ExtractTensorBatch(features, shuffled.Take(trainSize).ToArray()),
+                ExtractTensorBatch(labels, shuffled.Take(trainSize).ToArray())),
+            new InMemoryDataLoader<T, Tensor<T>, Tensor<T>>(
+                ExtractTensorBatch(features, shuffled.Skip(trainSize).Take(valSize).ToArray()),
+                ExtractTensorBatch(labels, shuffled.Skip(trainSize).Take(valSize).ToArray())),
+            new InMemoryDataLoader<T, Tensor<T>, Tensor<T>>(
+                ExtractTensorBatch(features, shuffled.Skip(trainSize + valSize).ToArray()),
+                ExtractTensorBatch(labels, shuffled.Skip(trainSize + valSize).ToArray()))
+        );

src/Data/Formats/MemoryMappedDataset.cs-152-163 (1)

152-163: ⚠️ Potential issue | 🟠 Major

Validate sampleShape matches _elementsPerSample.

Without this, a mismatched shape can cause invalid slicing or silently wrong batches.

🔧 Suggested change

-        if (sampleShape is not null)
-        {
-            batchShape = new int[sampleShape.Length + 1];
-            batchShape[0] = indices.Length;
-            Array.Copy(sampleShape, 0, batchShape, 1, sampleShape.Length);
-        }
+        if (sampleShape is not null)
+        {
+            int expectedElements = 1;
+            for (int d = 0; d < sampleShape.Length; d++)
+            {
+                expectedElements *= sampleShape[d];
+            }
+            if (expectedElements != _elementsPerSample)
+            {
+                throw new ArgumentException(
+                    $"sampleShape produces {expectedElements} elements but sample has {_elementsPerSample}.",
+                    nameof(sampleShape));
+            }
+
+            batchShape = new int[sampleShape.Length + 1];
+            batchShape[0] = indices.Length;
+            Array.Copy(sampleShape, 0, batchShape, 1, sampleShape.Length);
+        }

src/Data/Transforms/IdentityTransform.cs-13-13 (1)

13-13: ⚠️ Potential issue | 🟠 Major

Keep IdentityTransform internal unless surfaced through the facade.

This public transform increases the API surface outside the facade.

🔧 Suggested change

-public class IdentityTransform<T> : ITransform<T, T>
+internal class IdentityTransform<T> : ITransform<T, T>

As per coding guidelines: “Users should ONLY interact with `AiModelBuilder.cs` and `AiModelResult.cs`” and “Prefer `internal` over `public` for all classes, methods, and properties unless they are part of the facade API.”

src/Data/Vision/Benchmarks/Cifar100DataLoader.cs-142-150 (1)

142-150: ⚠️ Potential issue | 🟠 Major

Avoid null-forgiving in Split.

Project rules disallow null-forgiving; use explicit null checks and local variables after EnsureLoaded().

🔧 Suggested change

-        return (
-            new InMemoryDataLoader<T, Tensor<T>, Tensor<T>>(
-                ExtractTensorBatch(LoadedFeatures!, shuffled.Take(trainSize).ToArray()),
-                ExtractTensorBatch(LoadedLabels!, shuffled.Take(trainSize).ToArray())),
-            new InMemoryDataLoader<T, Tensor<T>, Tensor<T>>(
-                ExtractTensorBatch(LoadedFeatures!, shuffled.Skip(trainSize).Take(valSize).ToArray()),
-                ExtractTensorBatch(LoadedLabels!, shuffled.Skip(trainSize).Take(valSize).ToArray())),
-            new InMemoryDataLoader<T, Tensor<T>, Tensor<T>>(
-                ExtractTensorBatch(LoadedFeatures!, shuffled.Skip(trainSize + valSize).ToArray()),
-                ExtractTensorBatch(LoadedLabels!, shuffled.Skip(trainSize + valSize).ToArray()))
-        );
+        var features = LoadedFeatures ?? throw new InvalidOperationException("Features not loaded.");
+        var labels = LoadedLabels ?? throw new InvalidOperationException("Labels not loaded.");
+        return (
+            new InMemoryDataLoader<T, Tensor<T>, Tensor<T>>(
+                ExtractTensorBatch(features, shuffled.Take(trainSize).ToArray()),
+                ExtractTensorBatch(labels, shuffled.Take(trainSize).ToArray())),
+            new InMemoryDataLoader<T, Tensor<T>, Tensor<T>>(
+                ExtractTensorBatch(features, shuffled.Skip(trainSize).Take(valSize).ToArray()),
+                ExtractTensorBatch(labels, shuffled.Skip(trainSize).Take(valSize).ToArray())),
+            new InMemoryDataLoader<T, Tensor<T>, Tensor<T>>(
+                ExtractTensorBatch(features, shuffled.Skip(trainSize + valSize).ToArray()),
+                ExtractTensorBatch(labels, shuffled.Skip(trainSize + valSize).ToArray()))
+        );

src/Data/Formats/ParquetDataLoader.cs-310-329 (1)

310-329: ⚠️ Potential issue | 🟠 Major

Validate user-specified feature columns are numeric.

When FeatureColumns is provided, non-numeric fields are accepted and later converted to zeros, which is incorrect. Validate and throw early.

🔧 Suggested change

                 if (field is null)
                 {
                     throw new InvalidOperationException(
                         $"Feature column '{colName}' not found in Parquet schema. " +
                         $"Available columns: {string.Join(", ", allFields.Select(f => f.Name))}");
                 }
+                if (!IsNumericField(field))
+                {
+                    throw new InvalidOperationException(
+                        $"Feature column '{colName}' must be numeric.");
+                }

                 result.Add(field);

src/Data/Loaders/StatefulDataLoader.cs-85-130 (1)

85-130: ⚠️ Potential issue | 🟠 Major

Checkpoint restore doesn’t reapply shuffle order or inner index.

GetState() captures _currentShuffledIndices but it’s never populated, and LoadState() never replays the shuffle order. Also, CurrentIndex comes from the wrapper while all batches are pulled from _inner, so the saved position may be incorrect. Please wire state to the inner loader’s actual index/shuffle order (or re-shuffle deterministically with the saved seed) before replaying batches.

#!/bin/bash
# Inspect inner loader state surfaces for CurrentIndex / ShuffledIndices to restore accurately.
rg -n "CurrentIndex|CurrentBatchIndex|ShuffledIndices|Shuffle\\(" src/Data/Loaders -g '*.cs' -C2

src/Data/Transforms/Numeric/OneHotEncodeTransform.cs-17-17 (1)

17-17: ⚠️ Potential issue | 🟠 Major

Facade breach: OneHotEncodeTransform should be internal.
Public transform classes should not be exposed outside the facade.

🔧 Suggested change

-public class OneHotEncodeTransform<T> : ITransform<int, T[]>
+internal class OneHotEncodeTransform<T> : ITransform<int, T[]>

As per coding guidelines: “Users should ONLY interact with AiModelBuilder.cs and AiModelResult.cs” and “Prefer internal over public for all classes.”

src/Data/Transforms/Numeric/MinMaxScaleTransform.cs-16-16 (1)

16-16: ⚠️ Potential issue | 🟠 Major

Facade breach: MinMaxScaleTransform should be internal.
Public transform types under src/** should be hidden or routed via the facade.

🔧 Suggested change

-public class MinMaxScaleTransform<T> : ITransform<T[], T[]>
+internal class MinMaxScaleTransform<T> : ITransform<T[], T[]>

As per coding guidelines: “Users should ONLY interact with AiModelBuilder.cs and AiModelResult.cs” and “Prefer internal over public for all classes.”

src/Data/Transforms/Numeric/ToTensorTransform.cs-17-17 (1)

17-17: ⚠️ Potential issue | 🟠 Major

Facade breach: ToTensorTransform should be internal unless exposed via facade.
Public transform types should not be surfaced outside AiModelBuilder/AiModelResult.

🔧 Suggested change

-public class ToTensorTransform<T> : ITransform<T[], Tensor<T>>
+internal class ToTensorTransform<T> : ITransform<T[], Tensor<T>>

As per coding guidelines: “Users should ONLY interact with AiModelBuilder.cs and AiModelResult.cs” and “Prefer internal over public for all classes.”

src/Data/Transforms/LambdaTransform.cs-17-17 (1)

17-17: ⚠️ Potential issue | 🟠 Major

Facade breach: make LambdaTransform internal or expose via AiModelBuilder.
Public types under src/** should be internal unless surfaced by the facade. Consider changing this to internal or routing through AiModelBuilder/AiModelResult.

🔧 Suggested change

-public class LambdaTransform<TInput, TOutput> : ITransform<TInput, TOutput>
+internal class LambdaTransform<TInput, TOutput> : ITransform<TInput, TOutput>

As per coding guidelines: “Users should ONLY interact with AiModelBuilder.cs and AiModelResult.cs” and “Prefer internal over public for all classes.”

src/Data/Formats/ShardedStreamingDatasetOptions.cs-6-17 (1)

6-17: ⚠️ Potential issue | 🟠 Major

Facade breach: options type should not be public.
This class expands the public API outside the facade; make it internal or expose via AiModelBuilder only if required.

🔧 Suggested change

-public sealed class ShardedStreamingDatasetOptions
+internal sealed class ShardedStreamingDatasetOptions

As per coding guidelines: “Users should ONLY interact with AiModelBuilder.cs and AiModelResult.cs” and “Prefer internal over public for all classes.”

src/Data/Multimodal/ModalityType.cs-12-39 (1)

12-39: ⚠️ Potential issue | 🟠 Major

Facade breach: ModalityType should be internal.
Public enums under src/** expand the API beyond the facade.

🔧 Suggested change

-public enum ModalityType
+internal enum ModalityType

As per coding guidelines: “Users should ONLY interact with AiModelBuilder.cs and AiModelResult.cs” and “Prefer internal over public for all classes.”

src/Data/Formats/WebDatasetOptions.cs-6-17 (1)

6-17: ⚠️ Potential issue | 🟠 Major

Facade breach: WebDatasetOptions should be internal or routed via the facade.
Public options in src/** expand the surface area beyond AiModelBuilder/AiModelResult.

🔧 Suggested change

-public sealed class WebDatasetOptions
+internal sealed class WebDatasetOptions

As per coding guidelines: “Users should ONLY interact with AiModelBuilder.cs and AiModelResult.cs” and “Prefer internal over public for all classes.”

src/Data/Transforms/Numeric/MinMaxScaleTransform.cs-50-75 (1)

50-75: ⚠️ Potential issue | 🟠 Major

Validate each reference row to prevent out-of-range access.
If any row is null or has a different length than referenceData[0], indexing will throw or compute incorrect min/max.

🔧 Suggested change

-        for (int i = 0; i < referenceData.Length; i++)
-        {
-            for (int j = 0; j < featureCount; j++)
-            {
-                double val = NumOps.ToDouble(referenceData[i][j]);
+        for (int i = 0; i < referenceData.Length; i++)
+        {
+            var row = referenceData[i] 
+                ?? throw new ArgumentException($"Reference row {i} is null.", nameof(referenceData));
+            if (row.Length != featureCount)
+            {
+                throw new ArgumentException(
+                    $"Reference row {i} length ({row.Length}) must match feature count ({featureCount}).",
+                    nameof(referenceData));
+            }
+            for (int j = 0; j < featureCount; j++)
+            {
+                double val = NumOps.ToDouble(row[j]);
                 if (val < _min[j])
                 {
                     _min[j] = val;
                 }

src/Data/Transforms/Numeric/ToTensorTransform.cs-38-50 (1)

38-50: ⚠️ Potential issue | 🟠 Major

Guard against shape-product overflow.
total *= shape[i] can overflow int for large shapes, leading to invalid _expectedLength. Use checked with long, and throw if the product exceeds int.MaxValue.

🔧 Suggested change

-        int total = 1;
+        long total = 1;
         for (int i = 0; i < shape.Length; i++)
         {
             if (shape[i] <= 0)
             {
                 throw new ArgumentException($"Shape dimension {i} must be positive, got {shape[i]}.", nameof(shape));
             }
 
-            total *= shape[i];
+            checked { total *= shape[i]; }
+            if (total > int.MaxValue)
+            {
+                throw new ArgumentOutOfRangeException(nameof(shape), "Total tensor size exceeds Int32.MaxValue.");
+            }
         }
 
-        _shape = (int[])shape.Clone();
-        _expectedLength = total;
+        _shape = (int[])shape.Clone();
+        _expectedLength = (int)total;

src/Data/Transforms/Numeric/MinMaxScaleTransform.cs-123-148 (1)

123-148: ⚠️ Potential issue | 🟠 Major

Don’t silently scale only part of the vector.
Using Math.Min allows length mismatches and leaves tail values unchanged, which can mask bugs. Consider enforcing equal lengths.

🔧 Suggested change

-        int len = Math.Min(input.Length, _min.Length);
+        if (input.Length != _min.Length)
+        {
+            throw new ArgumentException(
+                $"Input length ({input.Length}) must match feature count ({_min.Length}).",
+                nameof(input));
+        }
+        int len = input.Length;
         var result = new T[input.Length];
         double targetRange = _targetMax - _targetMin;
@@
-        for (int i = len; i < input.Length; i++)
-        {
-            result[i] = input[i];
-        }
-
         return result;

src/Data/Vision/Benchmarks/FashionMnistDataLoaderOptions.cs-8-21 (1)

8-21: ⚠️ Potential issue | 🟠 Major

Avoid exposing options types publicly; keep them behind the facade.
Line 8 makes this a public API surface area outside AiModelBuilder/AiModelResult. Please make it internal (and expose configuration via the facade if needed).

🔧 Proposed fix

-public sealed class FashionMnistDataLoaderOptions
+internal sealed class FashionMnistDataLoaderOptions

As per coding guidelines, “Users should ONLY interact with AiModelBuilder.cs and AiModelResult.cs” and “Prefer internal over public for all classes, methods, and properties unless they are part of the facade API.”

src/Data/Multimodal/ModalitySample.cs-16-56 (1)

16-56: ⚠️ Potential issue | 🟠 Major

Make ModalitySample<T> internal to keep the facade clean.
Line 16 exposes a new public type outside AiModelBuilder/AiModelResult. Please make it internal and surface via the facade if required.

🔧 Proposed fix

-public class ModalitySample<T>
+internal class ModalitySample<T>

As per coding guidelines, “Users should ONLY interact with AiModelBuilder.cs and AiModelResult.cs” and “Prefer internal over public for all classes, methods, and properties unless they are part of the facade API.”

src/Data/Transforms/Numeric/NormalizeTransform.cs-16-71 (1)

16-71: ⚠️ Potential issue | 🟠 Major

Keep NormalizeTransform<T> internal to preserve the facade API.
Line 16 adds a public class outside AiModelBuilder/AiModelResult. Make it internal and expose via the facade.

🔧 Proposed fix

-public class NormalizeTransform<T> : ITransform<T[], T[]>
+internal class NormalizeTransform<T> : ITransform<T[], T[]>

As per coding guidelines, “Users should ONLY interact with AiModelBuilder.cs and AiModelResult.cs” and “Prefer internal over public for all classes, methods, and properties unless they are part of the facade API.”

src/Data/Transforms/Compose.cs-19-66 (1)

19-66: ⚠️ Potential issue | 🟠 Major

Keep Compose<T> internal to preserve the facade API.
Line 19 exposes a new public type outside AiModelBuilder/AiModelResult. Make it internal and surface usage through the facade if needed.

🔧 Proposed fix

-public class Compose<T> : ITransform<T, T>
+internal class Compose<T> : ITransform<T, T>

As per coding guidelines, “Users should ONLY interact with AiModelBuilder.cs and AiModelResult.cs” and “Prefer internal over public for all classes, methods, and properties unless they are part of the facade API.”

src/Data/Vision/ImageFolderDatasetOptions.cs-8-58 (1)

8-58: ⚠️ Potential issue | 🟠 Major

Keep dataset option types internal to the facade.
Line 8 introduces a public class outside AiModelBuilder/AiModelResult. Make it internal and route configuration through the facade.

🔧 Proposed fix

-public sealed class ImageFolderDatasetOptions
+internal sealed class ImageFolderDatasetOptions

As per coding guidelines, “Users should ONLY interact with AiModelBuilder.cs and AiModelResult.cs” and “Prefer internal over public for all classes, methods, and properties unless they are part of the facade API.”

src/Data/Audio/AudioFileDatasetOptions.cs-6-51 (1)

6-51: ⚠️ Potential issue | 🟠 Major

Keep options classes internal; avoid widening the public surface.
Line 6 adds a new public type outside the facade. Make it internal and wire configuration through AiModelBuilder.

🔧 Proposed fix

-public sealed class AudioFileDatasetOptions
+internal sealed class AudioFileDatasetOptions

As per coding guidelines, “Users should ONLY interact with AiModelBuilder.cs and AiModelResult.cs” and “Prefer internal over public for all classes, methods, and properties unless they are part of the facade API.”

src/Data/Transforms/Numeric/NormalizeTransform.cs-72-103 (1)

72-103: ⚠️ Potential issue | 🟠 Major

Fail fast on length mismatches to avoid partially normalized data.
Line 80 normalizes only min(input.Length, _mean.Length) and copies the rest unchanged, which can silently corrupt preprocessing. Prefer throwing when lengths differ.

🔧 Proposed fix

         if (input is null)
         {
             throw new ArgumentNullException(nameof(input));
         }
 
-        int len = Math.Min(input.Length, _mean.Length);
+        if (input.Length != _mean.Length)
+        {
+            throw new ArgumentException(
+                $"Input length ({input.Length}) must match mean/std length ({_mean.Length}).",
+                nameof(input));
+        }
+        int len = input.Length;
         var result = new T[input.Length];

src/Data/Formats/ShardedStreamingDataset.cs-21-178 (1)

21-178: ⚠️ Potential issue | 🟠 Major

Don’t expose ShardedStreamingDataset publicly; keep it behind the facade.
Line 21 makes this a public API. Please make it internal and surface usage through AiModelBuilder.

🔧 Proposed fix

-public class ShardedStreamingDataset : IDisposable
+internal class ShardedStreamingDataset : IDisposable

As per coding guidelines, “Users should ONLY interact with AiModelBuilder.cs and AiModelResult.cs” and “Prefer internal over public for all classes, methods, and properties unless they are part of the facade API.”

src/Data/Formats/WebDataset.cs-20-38 (1)

20-38: ⚠️ Potential issue | 🟠 Major

WebDataset should be internal unless explicitly part of the facade.

This adds a new public entry point outside AiModelBuilder/AiModelResult. Consider making it internal (or exposing only via the facade).

As per coding guidelines: "Users should ONLY interact with AiModelBuilder.cs and AiModelResult.cs" and "Prefer internal over public for all classes, methods, and properties unless they are part of the facade API."

src/Data/Vision/Benchmarks/FashionMnistDataLoader.cs-16-53 (1)

16-53: ⚠️ Potential issue | 🟠 Major

Public loader violates the facade-only API surface.

This adds a public entry point outside AiModelBuilder/AiModelResult. Consider making it internal or exposing only via the facade.

As per coding guidelines: "Users should ONLY interact with AiModelBuilder.cs and AiModelResult.cs" and "Prefer internal over public for all classes, methods, and properties unless they are part of the facade API."

src/Data/Loaders/DataLoaders.cs-1052-1148 (1)

1052-1148: ⚠️ Potential issue | 🟠 Major

Public factory methods expand the API beyond the facade.

These new public entry points make DataLoaders part of the public surface. Consider making these methods internal (or moving them behind AiModelBuilder) to preserve the facade-only contract.

As per coding guidelines: "Users should ONLY interact with AiModelBuilder.cs and AiModelResult.cs" and "Prefer internal over public for all classes, methods, and properties unless they are part of the facade API."

src/Data/Multimodal/MultimodalDataset.cs-29-66 (1)

29-66: ⚠️ Potential issue | 🟠 Major

Public dataset type should be internal unless part of the facade.

This adds a new public API outside AiModelBuilder/AiModelResult. Consider internal visibility or facade exposure.

As per coding guidelines: "Users should ONLY interact with AiModelBuilder.cs and AiModelResult.cs" and "Prefer internal over public for all classes, methods, and properties unless they are part of the facade API."

src/Data/Multimodal/MultimodalSample.cs-28-63 (1)

28-63: ⚠️ Potential issue | 🟠 Major

Public multimodal sample type should be internal unless part of the facade.

This introduces a new public API outside AiModelBuilder/AiModelResult. Consider internal visibility or facade exposure.

As per coding guidelines: "Users should ONLY interact with AiModelBuilder.cs and AiModelResult.cs" and "Prefer internal over public for all classes, methods, and properties unless they are part of the facade API."

src/Data/Multimodal/InterleavedDataset.cs-28-216 (1)

28-216: ⚠️ Potential issue | 🟠 Major

Interleaved multimodal types should be internal unless part of the facade.

These public classes add new API surface outside AiModelBuilder/AiModelResult. Consider internal visibility or facade exposure.

As per coding guidelines: "Users should ONLY interact with AiModelBuilder.cs and AiModelResult.cs" and "Prefer internal over public for all classes, methods, and properties unless they are part of the facade API."

src/Data/Text/Benchmarks/Imdb50kDataLoader.cs-18-46 (1)

18-46: ⚠️ Potential issue | 🟠 Major

Public loader should be internal per facade guideline.

This creates a public entry point outside AiModelBuilder/AiModelResult. Consider making it internal or exposing only through the facade.

As per coding guidelines: "Users should ONLY interact with AiModelBuilder.cs and AiModelResult.cs" and "Prefer internal over public for all classes, methods, and properties unless they are part of the facade API."

src/Data/Formats/WebDataset.cs-171-191 (1)

171-191: ⚠️ Potential issue | 🟠 Major

Guard against TAR entries larger than int.MaxValue.

size is a long, but the code allocates new byte[size] and casts to (int)size. Large entries will overflow or throw. Add a bounds check before allocation/read.

Proposed fix

-                byte[] fileData = new byte[size];
-                ReadFull(stream, fileData, (int)size);
+                if (size > int.MaxValue)
+                    throw new NotSupportedException("TAR entry too large to load into memory.");
+                byte[] fileData = new byte[size];
+                ReadFull(stream, fileData, (int)size);

src/Data/Text/Benchmarks/Imdb50kDataLoader.cs-69-88 (1)

69-88: ⚠️ Potential issue | 🟠 Major

MaxSamples truncation can yield single-class data.

Reviews are loaded as all positives then all negatives, and truncation happens before any shuffle. With small MaxSamples, you can end up with only one class. Shuffle/stratify before truncation.

Proposed fix

-        int totalSamples = reviews.Count;
-        if (_options.MaxSamples.HasValue && _options.MaxSamples.Value < totalSamples)
-            totalSamples = _options.MaxSamples.Value;
+        int totalSamples = reviews.Count;
+        if (_options.MaxSamples.HasValue && _options.MaxSamples.Value < totalSamples)
+        {
+            var rng = new Random();
+            var indices = Enumerable.Range(0, reviews.Count).OrderBy(_ => rng.Next()).Take(_options.MaxSamples.Value).ToArray();
+            reviews = indices.Select(i => reviews[i]).ToList();
+            labels = indices.Select(i => labels[i]).ToList();
+            totalSamples = reviews.Count;
+        }

src/Data/Vision/Benchmarks/FashionMnistDataLoader.cs-18-23 (1)

18-23: ⚠️ Potential issue | 🟠 Major

Switch to HTTPS endpoint for Fashion-MNIST dataset downloads.

The current HTTP base URL creates a MITM attack surface. Use the official GitHub-hosted HTTPS endpoint instead:

https://github.com/zalandoresearch/fashion-mnist/raw/master/data/fashion/

Update BaseUrl and ensure all dataset file URLs use this verified HTTPS source.

src/Data/Audio/AudioFileDataset.cs-335-358 (1)

335-358: ⚠️ Potential issue | 🟠 Major

Incomplete multi-channel to mono conversion for 24-bit and 32-bit audio.

The mono averaging logic at lines 343-355 only handles 8-bit and 16-bit channel values. When bitsPerSample is 24 or 32, the loop iterates over additional channels but never adds their values to sum, resulting in incorrect averaging (dividing a single channel's value by numChannels).

🐛 Proposed fix to handle 24-bit and 32-bit channels

                     if (bitsPerSample == 16)
                     {
                         short s = (short)(rawBytes[chOffset] | (rawBytes[chOffset + 1] << 8));
                         double chVal = s;
                         if (_options.Normalize) chVal /= 32768.0;
                         sum += chVal;
                     }
                     else if (bitsPerSample == 8)
                     {
                         double chVal = rawBytes[chOffset] - 128.0;
                         if (_options.Normalize) chVal /= 128.0;
                         sum += chVal;
                     }
+                    else if (bitsPerSample == 24)
+                    {
+                        int pcm24 = rawBytes[chOffset] | (rawBytes[chOffset + 1] << 8) | (rawBytes[chOffset + 2] << 16);
+                        if ((pcm24 & 0x800000) != 0) pcm24 |= unchecked((int)0xFF000000);
+                        double chVal = pcm24;
+                        if (_options.Normalize) chVal /= 8388608.0;
+                        sum += chVal;
+                    }
+                    else if (bitsPerSample == 32)
+                    {
+                        int pcm32 = BitConverter.ToInt32(rawBytes, chOffset);
+                        double chVal = pcm32;
+                        if (_options.Normalize) chVal /= 2147483648.0;
+                        sum += chVal;
+                    }
                 }

🟡 Minor comments (7)

src/Data/Text/TextLineDataset.cs-111-140 (1)

111-140: ⚠️ Potential issue | 🟡 Minor

Validate batch size to avoid ambiguous behavior.

A zero or negative batch size will yield confusing results or throw from List<T> with an unclear message. Guarding early provides a better contract.

✅ Suggested guard

 int effectiveBatchSize = batchSize ?? BatchSize;
+if (effectiveBatchSize <= 0)
+{
+    throw new ArgumentOutOfRangeException(nameof(batchSize), "Batch size must be greater than 0.");
+}
 var batch = new List<string>(effectiveBatchSize);

src/Data/Text/TokenizedTextDataset.cs-81-94 (1)

81-94: ⚠️ Potential issue | 🟡 Minor

Apply paddingTokenId when tokenIds[i] is null.

Currently null sequences stay zero-filled, which is incorrect when paddingTokenId != 0.

🧩 Suggested fix

-    _tokenIds[i] = new int[sequenceLength];
-    if (tokenIds[i] is not null)
-    {
-        int copyLen = Math.Min(tokenIds[i].Length, sequenceLength);
-        Array.Copy(tokenIds[i], _tokenIds[i], copyLen);
-        for (int j = copyLen; j < sequenceLength; j++)
-        {
-            _tokenIds[i][j] = paddingTokenId;
-        }
-    }
+    _tokenIds[i] = new int[sequenceLength];
+    Array.Fill(_tokenIds[i], paddingTokenId);
+    if (tokenIds[i] is not null)
+    {
+        int copyLen = Math.Min(tokenIds[i].Length, sequenceLength);
+        Array.Copy(tokenIds[i], _tokenIds[i], copyLen);
+    }

src/Data/Sampling/DynamicBatchSampler.cs-45-63 (1)

45-63: ⚠️ Potential issue | 🟡 Minor

Validate maxSamplesPerBatch/BatchSize to avoid invalid batching.

Zero or negative values currently slip through and create undefined batching behavior.

Proposed fix

         if (sampleLengths is null || sampleLengths.Length == 0)
             throw new ArgumentException("Sample lengths cannot be null or empty.", nameof(sampleLengths));
         if (maxTokensPerBatch < 1)
             throw new ArgumentOutOfRangeException(nameof(maxTokensPerBatch), "Max tokens per batch must be at least 1.");
+        if (maxSamplesPerBatch < 1)
+            throw new ArgumentOutOfRangeException(nameof(maxSamplesPerBatch), "Max samples per batch must be at least 1.");

src/Data/Multimodal/MultimodalSample.cs-67-86 (1)

67-86: ⚠️ Potential issue | 🟡 Minor

Guard against null modality entries.

A null ModalitySample<T> will throw a NullReferenceException later. Add explicit checks for clearer errors.

Proposed fix

         for (int i = 0; i < modalities.Length; i++)
         {
+            if (modalities[i] is null)
+                throw new ArgumentNullException(nameof(modalities), "Modality samples cannot contain null entries.");
             string key = modalities[i].Key;
             if (_keyIndex.ContainsKey(key))
             {

src/Data/Multimodal/MultimodalDataset.cs-107-197 (1)

107-197: ⚠️ Potential issue | 🟡 Minor

Validate batchSize (and startIndex) in batch APIs.

GetModalityBatch and GetLabelBatch accept non‑positive batchSize, leading to negative lengths and runtime exceptions. Add explicit checks (and mirror the startIndex guard in GetLabelBatch).

Proposed fix

     public Tensor<T> GetModalityBatch(int startIndex, int batchSize, string modalityKey)
     {
+        if (batchSize <= 0)
+            throw new ArgumentOutOfRangeException(nameof(batchSize), "Batch size must be positive.");
         if (startIndex < 0 || startIndex >= _samples.Count)
             throw new ArgumentOutOfRangeException(nameof(startIndex));
         ...
     }

     public Tensor<T>? GetLabelBatch(int startIndex, int batchSize)
     {
+        if (batchSize <= 0)
+            throw new ArgumentOutOfRangeException(nameof(batchSize), "Batch size must be positive.");
+        if (startIndex < 0 || startIndex >= _samples.Count)
+            throw new ArgumentOutOfRangeException(nameof(startIndex));
         int endIndex = Math.Min(startIndex + batchSize, _samples.Count);
         ...
     }

src/Data/Vision/Benchmarks/FashionMnistDataLoader.cs-80-123 (1)

80-123: ⚠️ Potential issue | 🟡 Minor

Validate label count and file lengths before indexing.

The loader derives samplesToLoad from the image file, then indexes labels without checking the label count or byte lengths. Truncated or mismatched files will throw or corrupt data.

Proposed fix

-        int imageCount = ReadBigEndianInt32(imageBytes, 4);
+        int imageCount = ReadBigEndianInt32(imageBytes, 4);
+        int labelCount = ReadBigEndianInt32(labelBytes, 4);
+        int availableSamples = Math.Min(imageCount, labelCount);
+
-        int samplesToLoad = imageCount;
+        int samplesToLoad = availableSamples;
         if (_options.MaxSamples.HasValue && _options.MaxSamples.Value < samplesToLoad)
             samplesToLoad = _options.MaxSamples.Value;
+
+        int expectedImageBytes = 16 + samplesToLoad * (ReadBigEndianInt32(imageBytes, 8) * ReadBigEndianInt32(imageBytes, 12));
+        int expectedLabelBytes = 8 + samplesToLoad;
+        if (imageBytes.Length < expectedImageBytes || labelBytes.Length < expectedLabelBytes)
+            throw new InvalidDataException("Fashion-MNIST files appear truncated.");

src/Data/Vision/ImageClassificationDataset.cs-153-210 (1)

153-210: ⚠️ Potential issue | 🟡 Minor

Documentation claims [C, H, W] support but implementation only handles [H, W, C].

The XML doc on line 156 states the constructor accepts tensors of shape [H, W, C] or [C, H, W], but the implementation (lines 193-210) only handles:

• 3D as [H, W, C] (lines 193-198)
• 2D as [H, W] with implicit C=1 (lines 199-204)

If CHW format support is intended, additional logic is needed. Otherwise, update the documentation to reflect actual behavior.

📝 Fix documentation to match implementation

     /// <summary>
     /// Creates an in-memory image classification dataset from tensors.
     /// </summary>
-    /// <param name="imageTensors">Individual image tensors, each of shape [H, W, C] or [C, H, W].</param>
+    /// <param name="imageTensors">Individual image tensors, each of shape [H, W, C] or [H, W] (grayscale).</param>
     /// <param name="labels">Class label for each image (0-based).</param>
     /// <param name="transform">Optional transform pipeline applied to pixel data.</param>

[github-advanced-security]

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[coderabbitai]

Actionable comments posted: 12

🤖 Fix all issues with AI agents

In `@src/Data/Formats/JsonlDataLoader.cs`:
- Around line 83-95: The LoadDataCoreAsync implementation can leak the
JsonlStreamingLoader and leave _cachedRecords partially filled if enumeration
throws or cancellation occurs; change it to instantiate _jsonlLoader, then
iterate inside a try block and use a finally (or a using pattern) to Dispose()
the loader if an exception or cancellation happens, and in the catch/finally
clear _cachedRecords and rethrow/propagate the cancellation; reference the
LoadDataCoreAsync method, the _jsonlLoader field/JsonlStreamingLoader
constructor, the ReadObjects() enumeration, and the _cachedRecords list when
making these changes.
- Around line 23-24: The JsonlDataLoader<T> class is publicly exposed which
bypasses the AiModelBuilder/AiModelResult facade; change its visibility to
internal so consumers must go through the facade. Locate the declaration of
JsonlDataLoader<T> (subclassing StreamingDataLoaderBase<T, Tensor<T>,
Tensor<T>>) and change its access modifier to internal, ensuring any
factory/builder methods in AiModelBuilder or exposure points in AiModelResult
provide the intended creation path and update any tests/usages to use those
facade methods instead of constructing JsonlDataLoader<T> directly.

In `@src/Data/Formats/ShardedStreamingDataLoader.cs`:
- Around line 22-23: Change the public API of the ShardedStreamingDataLoader<T>
class to internal to prevent direct construction and force usage via the
AiModelBuilder/AiModelResult facade; update the class declaration from public to
internal (ShardedStreamingDataLoader<T> : StreamingDataLoaderBase<T, Tensor<T>,
Tensor<T>>) and ensure any constructors or factory methods remain accessible to
the facade (adjust their accessibility if needed) and that AiModelBuilder
exposes a creation method that returns the loader or a wrapper as required; also
update any call sites/tests to use the facade (AiModelBuilder/AiModelResult)
rather than constructing ShardedStreamingDataLoader directly.
- Around line 74-86: LoadDataCoreAsync currently creates _shardedDataset and
enumerates ReadRecords() without ensuring disposal on cancellation or
exceptions; wrap the enumeration in a try/catch/finally (or a using pattern) so
that if cancellationToken.ThrowIfCancellationRequested() or ReadRecords()
throws, you call _shardedDataset.Dispose() (or _shardedDataset?.Dispose()),
clear _cachedRecords, and rethrow/propagate the exception; ensure successful
completion does not double-dispose and that _shardedDataset remains assigned for
later use only when the load fully completes.

In `@src/Data/Formats/WebDatasetDataLoader.cs`:
- Around line 22-23: The WebDatasetDataLoader<T> class is declared public but
should be internal to limit the API surface; change the declaration of
WebDatasetDataLoader<T> (which inherits StreamingDataLoaderBase<T, Tensor<T>,
Tensor<T>>) from public to internal and ensure it is instantiated/exposed only
through the existing facade methods on AiModelBuilder and AiModelResult (add or
use a factory/creator method on AiModelBuilder that returns the appropriate type
so consumers never construct WebDatasetDataLoader<T> directly).
- Around line 74-86: LoadDataCoreAsync currently creates _webDataset and then
enumerates ReadSamples() without guarding against exceptions or cancellation,
which can leave _webDataset open and _cachedSamples partially filled; change
LoadDataCoreAsync to wrap the enumeration in a try/finally (or try/catch) so
that on any exception or when cancellationToken.ThrowIfCancellationRequested()
triggers you dispose _webDataset and clear _cachedSamples before rethrowing (or
returning), and ensure on successful completion _webDataset remains assigned for
later disposal or is disposed as intended; reference _webDataset,
_cachedSamples, LoadDataCoreAsync and WebDataset.ReadSamples() when applying the
fix.

In `@src/Data/Loaders/DataLoaders.cs`:
- Around line 1054-1148: The public factory methods ImageFolder<T>,
AudioFiles<T>, and VideoFrames<T> on DataLoaders expose concrete dataset types;
change their visibility or return types so implementation classes are not part
of the public surface: either make these methods internal, have them return an
interface (e.g., IDatasetLoader<T>) instead of
ImageFolderDataset<T>/AudioFileDataset<T>/VideoFrameDataset<T>, or move the
factory calls behind the facade methods on AiModelBuilder/AiModelResult; update
references to use the chosen interface/facade and ensure ArgumentNullException
checks remain in the entry point that is kept public.
- Around line 1253-1475: Add input validation to the format-specific factory
overloads so they fail fast on invalid batchSize and shuffleBufferSize: check in
FromWebDataset<T>(string[] tarPaths, ...) and FromWebDataset<T>(string tarPath,
...) that batchSize > 0 and throw ArgumentOutOfRangeException if not; check in
FromJsonl<T>(string filePath, ...) and FromJsonl<T>(string[] filePaths, ...)
that batchSize > 0 and shuffleBufferSize >= 0 and throw
ArgumentOutOfRangeException for invalid values; and check in
FromShards<T>(string[] shardPaths, ...) that batchSize > 0 and throw
ArgumentOutOfRangeException if not. Place these guards alongside the existing
null/empty checks (functions: FromWebDataset, FromJsonl, FromShards).

In `@src/Data/Video/VideoFrameDatasetOptions.cs`:
- Around line 20-23: The default FrameExtensions on VideoFrameDatasetOptions
includes formats (".png",".jpg",".jpeg",".gif",".tiff") that ImageHelper does
not support on net471 and can throw NotSupportedException; update the
initialization for FrameExtensions to be framework-conditional so net471
defaults only to supported formats (".bmp",".ppm",".pgm") and other frameworks
keep the broader list, or alternatively require callers to explicitly set
FrameExtensions and document this requirement; modify the
VideoFrameDatasetOptions class constructor/initializer (FrameExtensions) to use
`#if` NET471 (or a runtime target-framework check) to pick the safe defaults for
net471 while preserving the full defaults for modern frameworks and add a short
XML doc note on FrameExtensions about framework-specific support.
- Around line 13-14: The VideoFrameDatasetOptions class is declared public but
should be internal unless it's intentionally exposed by the public facade;
change the accessibility of VideoFrameDatasetOptions from public to internal (or
verify and document if it must remain public because AiModelBuilder or
AiModelResult expose it) so the options type is not part of the external API
surface unless explicitly surfaced via AiModelBuilder/AiModelResult.

In `@src/Data/Vision/ImageFolderDatasetOptions.cs`:
- Around line 8-9: The ImageFolderDatasetOptions class is publicly exposed but
should be internal unless surfaced by the facade; change the declaration of
ImageFolderDatasetOptions (the type named ImageFolderDatasetOptions) from public
sealed class to internal sealed class so it is not part of the public API
surface, and update any callers/tests accordingly (either reference it from the
same assembly or add an InternalsVisibleTo for test assemblies); if this options
type must be consumed by AiModelBuilder or AiModelResult then instead expose a
facade DTO or property there and keep the class internal only when not directly
exposed by those facades.
- Around line 15-18: The current default for
ImageFolderDatasetOptions.Extensions includes formats unsupported on net471
(PNG/JPEG/GIF/TIFF) and will fail when ImageHelper on net471 is used; change the
default to be framework-conditional or require callers to override. Update the
Extensions property initialization in ImageFolderDatasetOptions to use
conditional compilation (e.g., set to new[] { ".bmp", ".ppm", ".pgm" } under
NET471 and the broader list for other frameworks) or remove the initializer and
throw/validate with a clear message if Extensions is null/empty; also update the
XML summary to document that net471 only supports BMP/PPM/PGM or that callers
must supply compatible extensions when targeting net471.

🧹 Nitpick comments (1)

src/Helpers/ImageHelper.cs (1)

506-525: Use ImageSharp ProcessPixelRows for efficient pixel access.
Direct image[x, y] indexing performs per-pixel bounds checks; ProcessPixelRows with GetRowSpan is the ImageSharp-recommended pattern for bulk pixel reads and improves performance by leveraging span-based access.

♻️ Suggested refactor

-        for (int y = 0; y < height; y++)
-        {
-            for (int x = 0; x < width; x++)
-            {
-                var pixel = image[x, y];
-                span[0 * height * width + y * width + x] = NumOps.FromDouble(pixel.R / normFactor);
-                span[1 * height * width + y * width + x] = NumOps.FromDouble(pixel.G / normFactor);
-                span[2 * height * width + y * width + x] = NumOps.FromDouble(pixel.B / normFactor);
-            }
-        }
+        image.ProcessPixelRows(accessor =>
+        {
+            for (int y = 0; y < height; y++)
+            {
+                var row = accessor.GetRowSpan(y);
+                for (int x = 0; x < width; x++)
+                {
+                    var pixel = row[x];
+                    span[0 * height * width + y * width + x] = NumOps.FromDouble(pixel.R / normFactor);
+                    span[1 * height * width + y * width + x] = NumOps.FromDouble(pixel.G / normFactor);
+                    span[2 * height * width + y * width + x] = NumOps.FromDouble(pixel.B / normFactor);
+                }
+            }
+        });

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@src/Data/Formats/ShardedStreamingDataset.cs`:
- Around line 152-157: The code in ShardedStreamingDataset currently checks only
for non-positive recordLength and then allocates a byte[] of that size, which
can OOM on huge/corrupted values; update the read logic (the block using
BitConverter.ToInt32(lengthBuf, 0), recordLength and shardPath in
ShardedStreamingDataset) to enforce an upper bound (e.g., 1_073_741_824 bytes /
1GB or a configurable maxRecordSize) and throw a clear InvalidDataException if
recordLength is larger than the limit, rather than attempting to allocate the
array.

🧹 Nitpick comments (3)

src/Data/Formats/WebDataset.cs (1)

20-25: IDisposable is misleading without disposed guards.

_disposed is set but never enforced. Either add a guard in public entrypoints or drop the IDisposable implementation to avoid false lifecycle expectations.

Also applies to: 318-325

src/Data/Formats/ShardedStreamingDataset.cs (2)

38-45: Consider defensive copy of the shard paths array.

The constructor stores a direct reference to the caller's array. If the caller mutates shardPaths after construction, iteration behavior becomes unpredictable. A defensive copy would ensure immutability.

🛡️ Proposed defensive copy

     public ShardedStreamingDataset(string[] shardPaths, ShardedStreamingDatasetOptions? options = null)
     {
         if (shardPaths is null || shardPaths.Length == 0)
             throw new ArgumentException("At least one shard path is required.", nameof(shardPaths));

-        _shardPaths = shardPaths;
+        _shardPaths = (string[])shardPaths.Clone();
         _options = options ?? new ShardedStreamingDatasetOptions();
     }

---

92-98: Duplicate Fisher-Yates shuffle logic could be extracted.

The same shuffle algorithm appears twice (lines 92-98 and 120-126). Extracting to a small helper would reduce duplication and make the main method easier to follow.

♻️ Extract shuffle helper

+    private static void ShuffleInPlace<T>(List<T> list, Random random)
+    {
+        for (int i = list.Count - 1; i > 0; i--)
+        {
+            int j = random.Next(i + 1);
+            (list[i], list[j]) = (list[j], list[i]);
+        }
+    }

Then replace both shuffle blocks with:

ShuffleInPlace(shuffleBuffer, shuffleRandom);

Also applies to: 120-126

[sonarqubecloud]

Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)
9.1% Duplication on New Code (required ≤ 3%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

[coderabbitai]

Actionable comments posted: 6

🤖 Fix all issues with AI agents

In `@src/Data/Formats/MemoryMappedDataset.cs`:
- Around line 99-114: ReadSample currently accesses _accessor without checking
disposal; add a guard at the start of the ReadSample method to throw
ObjectDisposedException when _disposed is true (e.g., use
ObjectDisposedException.ThrowIf(_disposed) if targeting .NET 7+, otherwise use
if (_disposed) throw new
ObjectDisposedException(nameof(MemoryMappedDataset<T>))); this ensures _accessor
and other resources aren't used after Dispose was called.
- Around line 67-92: In the MemoryMappedDataset constructor, opening the
MemoryMappedFile and creating the ViewAccessor (_mmf and _accessor) can leak
handles if the header validation (reading _numSamples and _elementsPerSample and
validating against HeaderSize/ElementSize) throws; wrap the CreateFromFile and
CreateViewAccessor calls plus the subsequent reads/validations in a try block
and ensure in the catch/finally that you Dispose/Close _accessor and _mmf if
they were created (but do not dispose them on successful construction), or
alternatively use safe temporary local variables for the mmap and accessor and
only assign to the fields after all validation succeeds so failed validation
triggers disposal of the temporaries.
- Around line 202-234: WriteDatasetFile does not validate that numSamples and
elementsPerSample are > 0, which causes files to be rejected by the reader;
update the method (WriteDatasetFile) to validate numSamples and
elementsPerSample after computing them and throw an ArgumentException with a
clear message if either is <= 0 (matching the constructor's checks) so
writer-reader symmetry is preserved.
- Around line 149-177: ReadBatch is not validating that the product of
sampleShape equals _elementsPerSample, which can cause overruns or uninitialized
data when copying samples; add a check in ReadBatch (before creating result)
that when sampleShape != null you compute the product of sampleShape and throw
an ArgumentException if product != _elementsPerSample (include a clear message
naming sampleShape and _elementsPerSample), then proceed with the existing logic
that uses ReadSample, result.Data.Span and dstOffset.

In `@src/Data/Formats/ShardedStreamingDataset.cs`:
- Around line 137-147: Move the XML documentation block so it immediately
precedes the ReadShard method declaration (ensuring the <summary> and
param/returns tags describe ReadShard), and then add or retain a separate XML
doc for the MaxRecordLength constant right above its declaration; specifically
ensure the ReadShard documentation is not placed above MaxRecordLength and that
both symbols (MaxRecordLength and ReadShard) have their own XML comments
directly above their declarations.

In `@tests/AiDotNet.Tests/UnitTests/Data/Sampling/DynamicBatchSamplerTests.cs`:
- Around line 77-78: Rename the test method GetBatchIndices_RespectesTokenBudget
to correct the typo to GetBatchIndices_RespectsTokenBudget in the
DynamicBatchSamplerTests class; update any references/usages (e.g., test runner
attributes like [Fact]) so the method name change is reflected and tests
continue to run.

🧹 Nitpick comments (3)

src/Data/Formats/MemoryMappedDataset.cs (1)

36-36: Consider making this class internal to follow the facade pattern.

Per the repository's architecture guidelines, implementation details like dataset format readers should be internal rather than public. Users should interact through the AiModelBuilder facade, not directly with format-specific classes.

If this needs to be user-facing, it should be exposed via methods on AiModelBuilder or a factory within the public API surface. As per coding guidelines: "Prefer internal over public for all classes, methods, and properties unless they are part of the facade API."

♻️ Suggested change

-public class MemoryMappedDataset<T> : IDisposable
+internal class MemoryMappedDataset<T> : IDisposable

src/Data/Sampling/DynamicBatchSampler.cs (1)

28-31: Mutable properties after construction could cause surprising behavior.

BatchSize and DropLast are publicly settable, meaning users could modify them between GetBatchIndices() calls or even during iteration. For a sampler whose behavior should be deterministic based on construction parameters, consider making these immutable.

♻️ Suggested: Use private setters or init-only properties

     /// <summary>
     /// Gets or sets the maximum number of samples per batch (upper bound even if token budget allows more).
     /// </summary>
-    public int BatchSize { get; set; }
+    public int BatchSize { get; }

     /// <inheritdoc/>
-    public bool DropLast { get; set; }
+    public bool DropLast { get; }

tests/AiDotNet.Tests/UnitTests/Data/Sampling/DynamicBatchSamplerTests.cs (1)

114-130: Consider strengthening the assertion for exact batch structure.

The comment documents the expected behavior precisely (Batch 1: [10], Batch 2: [200], Batch 3: [10]), but the assertion only verifies that index 1 exists somewhere. Since shuffle: false, the result is deterministic and you could assert the exact structure.

💡 Optional: Verify exact batch structure

             // Batch 1: [10] (first sample)
             // Batch 2: [200] (exceeds budget but gets its own batch)
             // Batch 3: [10] (third sample)
             Assert.Equal(3, batches.Count);
-            Assert.Contains(1, batches.SelectMany(b => b)); // The oversized sample is still included
+            Assert.Equal(new[] { 0 }, batches[0]);
+            Assert.Equal(new[] { 1 }, batches[1]); // The oversized sample gets its own batch
+            Assert.Equal(new[] { 2 }, batches[2]);

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@src/Data/Formats/MemoryMappedDataset.cs`:
- Around line 36-62: The MemoryMappedDataset<T> type and its public properties
are exposing internal implementation outside the facade; change the class
declaration from public to internal (internal class MemoryMappedDataset<T>) and
change the two public property accessors (NumSamples and ElementsPerSample) to
internal as well so the type and its surface are not public; retain IDisposable
and existing members but update any call sites (AiModelBuilder/AiModelResult) to
access the now-internal members or expose required data through the facade entry
points instead.

[Copilot]

Pull request overview

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

---

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

[coderabbitai]

Actionable comments posted: 8

🤖 Fix all issues with AI agents

In `@src/Data/Sampling/DistributedBucketSampler.cs`:
- Around line 26-31: BatchSize currently has a public setter that can be set to
0 or negative, causing the loop in GetBatchIndices() (which uses start +=
BatchSize) to never advance; add validation to prevent invalid values by either:
(a) validating in the BatchSize setter to throw an ArgumentOutOfRangeException
if value <= 0, or (b) at the start of GetBatchIndices() guard with if (BatchSize
<= 0) throw new InvalidOperationException("BatchSize must be > 0") (or sanitize
to 1), and ensure any related logic that depends on DropLast/BatchSize behaves
correctly when BatchSize is validated.

In `@src/Data/Sampling/DynamicBatchSampler.cs`:
- Around line 19-35: The DynamicBatchSampler class is declared public but
appears not to be part of the public facade; change its accessibility to
internal to follow the guideline preferring internal for non-facade types.
Locate the class declaration for DynamicBatchSampler and replace the public
modifier with internal; keep its existing members (BatchSize, DropLast, Length,
_sampleLengths, _maxTokensPerBatch, _shuffle) unchanged, and if this type must
be exposed to users later, provide access through the facade types
AiModelBuilder or AiModelResult instead.

In `@src/Data/Vision/Benchmarks/Cifar100DataLoader.cs`:
- Around line 17-43: Change the loader's visibility to internal to keep it
behind the facade: make the generic class Cifar100DataLoader<T> internal
(instead of public) and change its constructor Cifar100DataLoader(...) to
internal as well; leave the override members (Name, Description, TotalCount,
FeatureCount, OutputDimension) as-is but ensure no other public constructors or
factory methods remain that would expose the class directly—expose the loader
only via the AiModelBuilder facade.
- Around line 66-70: The code reads the CIFAR-100 file into data and computes
bytesPerSample and totalSamples but doesn't verify data.Length is an exact
multiple of bytesPerSample, which silently ignores trailing/corrupt bytes; in
Cifar100DataLoader (where dataFile, bytesPerSample and totalSamples are used)
add a sanity check after reading the file: if (data.Length % bytesPerSample !=
0) throw a descriptive exception (or log error and fail) referencing dataFile
and the expected bytesPerSample so callers know the binary is corrupt/partial
before proceeding to parse samples.

In `@src/Data/Vision/Benchmarks/Cifar10DataLoader.cs`:
- Around line 176-191: The current LoadBatchFile method silently returns when a
batch file is missing, causing silent partial datasets; update LoadBatchFile
(referencing parameters dataDir and fileName) to detect the missing file and
throw a descriptive exception (e.g., FileNotFoundException or an
ApplicationException with contextual message including filePath and fileName)
instead of returning, so callers fail fast and surface data integrity problems;
keep existing behavior for reading/iterating bytes intact but ensure callers can
catch or let the exception bubble up.
- Around line 18-50: The Cifar10DataLoader<T> class is currently public but
should be internal if it's not intended as part of the public API; change the
class accessibility from public to internal (class Cifar10DataLoader<T>) and
ensure it is exposed only through the facade (AiModelBuilder and AiModelResult)
by adding any necessary internal factory or registration points used by
AiModelBuilder to instantiate or access it, keeping its constructor and members
as internal or public as needed for AiModelBuilder to use them.

In `@src/Data/Vision/Benchmarks/FashionMnistDataLoader.cs`:
- Around line 86-103: The code currently only checks buffer lengths but not IDX
magic numbers or non-negative dimensions, which lets corrupt files report huge
counts; before reading imageCount/rows/cols use ReadBigEndianInt32(imageBytes,
0) to verify the image magic is 2051 (0x00000803) and
ReadBigEndianInt32(labelBytes, 0) to verify the label magic is 2049
(0x00000801); then validate imageCount, rows, cols and labelCount are >= 0 and
small enough that (long)imageCount * rows * cols cannot overflow and fits within
imageBytes.Length - 16, and similarly ensure 8 + labelCount fits
labelBytes.Length, returning/throwing InvalidDataException with clear messages
when any check fails (use the existing ReadBigEndianInt32, imageBytes,
labelBytes, imageCount, rows, cols, labelCount symbols to locate and implement).

In `@src/Data/Vision/Benchmarks/MnistDataLoader.cs`:
- Around line 28-67: MnistDataLoader<T> is declared public and leaks
implementation details; change its accessibility to internal (class
MnistDataLoader<T>) and keep internal members as needed (Name, Description,
TotalCount, FeatureCount, OutputDimension, NumClasses and the constructor
MnistDataLoader) so it is not part of the public API surface, and ensure any
callers outside the assembly obtain instances through the facade (factory or
interface exposed by AiModelBuilder/AiModelResult) rather than referencing
MnistDataLoader<T> directly.

🧹 Nitpick comments (5)

src/Data/Text/Benchmarks/Imdb50kDataLoader.cs (4)

19-19: Consider making this class internal to align with the facade pattern.

Per the coding guidelines, implementation classes should prefer internal over public unless they're part of the facade API (AiModelBuilder/AiModelResult). Since users access this through DataLoaders.Imdb50k<T>() factory method, exposing the class as public increases the API surface unnecessarily.

If the factory returns IInputOutputDataLoader<T, Tensor<T>, Tensor<T>>, users don't need direct access to the concrete type.

♻️ Suggested change

-public class Imdb50kDataLoader<T> : InputOutputDataLoaderBase<T, Tensor<T>, Tensor<T>>
+internal class Imdb50kDataLoader<T> : InputOutputDataLoaderBase<T, Tensor<T>, Tensor<T>>

As per coding guidelines: "Prefer internal over public for all classes, methods, and properties unless they are part of the facade API."

---

181-196: Synchronous file I/O in async context.

LoadReviews uses File.ReadAllText synchronously, but it's called from LoadDataCoreAsync. With 25k files per split, this blocks the thread pool thread for extended periods. Consider using File.ReadAllTextAsync for better scalability.

♻️ Suggested async version

-    private static void LoadReviews(string directory, int label,
+    private static async Task LoadReviewsAsync(string directory, int label,
         List<string> reviews, List<int> labels, CancellationToken cancellationToken)
     {
         if (!Directory.Exists(directory)) return;
 
         string[] files = Directory.GetFiles(directory, "*.txt");
         Array.Sort(files, StringComparer.OrdinalIgnoreCase);
 
         foreach (string file in files)
         {
             cancellationToken.ThrowIfCancellationRequested();
-            string text = File.ReadAllText(file);
+            string text = await File.ReadAllTextAsync(file, cancellationToken);
             reviews.Add(text);
             labels.Add(label);
         }
     }

---

150-178: Redundant array allocations and null checks in Split method.

Each set of indices is computed twice (once for features, once for labels), and the null checks are repeated despite EnsureLoaded() already being called. Extract the indices once per split:

♻️ Cleaner implementation

     {
         EnsureLoaded();
         ValidateSplitRatios(trainRatio, validationRatio);
         var (trainSize, valSize, _) = ComputeSplitSizes(_sampleCount, trainRatio, validationRatio);
         var random = seed.HasValue
             ? RandomHelper.CreateSeededRandom(seed.Value)
             : RandomHelper.CreateSecureRandom();
         var shuffled = Enumerable.Range(0, _sampleCount).OrderBy(_ => random.Next()).ToArray();
+
+        var features = LoadedFeatures ?? throw new InvalidOperationException("Features not loaded.");
+        var labels = LoadedLabels ?? throw new InvalidOperationException("Labels not loaded.");
+
+        var trainIndices = shuffled.Take(trainSize).ToArray();
+        var valIndices = shuffled.Skip(trainSize).Take(valSize).ToArray();
+        var testIndices = shuffled.Skip(trainSize + valSize).ToArray();
+
         return (
             new InMemoryDataLoader<T, Tensor<T>, Tensor<T>>(
-                ExtractTensorBatch(LoadedFeatures ?? throw new InvalidOperationException("Not loaded."),
-                    shuffled.Take(trainSize).ToArray()),
-                ExtractTensorBatch(LoadedLabels ?? throw new InvalidOperationException("Not loaded."),
-                    shuffled.Take(trainSize).ToArray())),
+                ExtractTensorBatch(features, trainIndices),
+                ExtractTensorBatch(labels, trainIndices)),
             new InMemoryDataLoader<T, Tensor<T>, Tensor<T>>(
-                ExtractTensorBatch(LoadedFeatures ?? throw new InvalidOperationException("Not loaded."),
-                    shuffled.Skip(trainSize).Take(valSize).ToArray()),
-                ExtractTensorBatch(LoadedLabels ?? throw new InvalidOperationException("Not loaded."),
-                    shuffled.Skip(trainSize).Take(valSize).ToArray())),
+                ExtractTensorBatch(features, valIndices),
+                ExtractTensorBatch(labels, valIndices)),
             new InMemoryDataLoader<T, Tensor<T>, Tensor<T>>(
-                ExtractTensorBatch(LoadedFeatures ?? throw new InvalidOperationException("Not loaded."),
-                    shuffled.Skip(trainSize + valSize).ToArray()),
-                ExtractTensorBatch(LoadedLabels ?? throw new InvalidOperationException("Not loaded."),
-                    shuffled.Skip(trainSize + valSize).ToArray()))
+                ExtractTensorBatch(features, testIndices),
+                ExtractTensorBatch(labels, testIndices))
         );
     }

---

230-246: Silent failure on regex timeout may hide data quality issues.

When RegexMatchTimeoutException is caught, the method returns an empty token list with no indication. This could produce unexpected results (reviews encoded as all-PAD). Consider at minimum tracking the count of failed tokenizations for diagnostics, or throwing if it exceeds a threshold.

src/Data/Sampling/DynamicBatchSampler.cs (1)

25-31: Keep BatchSize invariant after construction.

BatchSize has a public setter, but only the constructor validates it. Consider validating in GetBatchIndices() (or restricting the setter) so invalid runtime updates don’t undermine batching behavior.

Suggested change

     public IEnumerable<int[]> GetBatchIndices()
     {
+        if (BatchSize < 1)
+            throw new ArgumentOutOfRangeException(nameof(BatchSize), "BatchSize must be at least 1.");
         int[] indices = CreateSequentialIndices(_sampleLengths.Length);

Also applies to: 85-104

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

src/Interfaces/IStatefulDataLoader.cs:1

• DataLoaderCheckpoint exposes ShuffledIndices, but the provided StatefulDataLoader implementation does not populate or restore it (it only relies on a seed). Either remove ShuffledIndices from the checkpoint contract or implement saving/restoring the exact permutation to match the interface/docs and to support non-seed-based shuffles.
  src/Data/Vision/ImageFolderDataset.cs:1
• Using OrderBy(random.Next()) to shuffle is O(n log n) and can produce biased ordering when keys collide. Prefer an in-place Fisher–Yates shuffle (or a helper that does so) and then Take(maxSamples) for better performance and more uniform shuffling.

---

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