feat: add comprehensive quantization framework with PTQ and QAT support - #816
Conversation
…rt (#278) Implements a complete model quantization framework supporting both Post-Training Quantization (PTQ) and Quantization-Aware Training (QAT). PTQ Strategies: - GPTQ: Hessian-based quantization with ActOrder optimization - AWQ: Activation-aware weight quantization for important weight protection - SmoothQuant: Per-channel smoothing for W8A8 quantization - SpinQuant: Rotation-based quantization using Cayley parameterization - QuIP#: 2-bit quantization with Hadamard transforms and lattice codebooks Numeric Formats: - INT8: Standard 8-bit integer quantization - Float16: Half-precision floating point - FP8: E4M3 and E5M2 formats for training/inference - NF4: 4-bit NormalFloat optimal for normally distributed weights (QLoRA) - MXFP4: Microscaling FP4 with shared exponents (OCP standard) QAT Support: - QATTrainingHook: Fake quantization during training with STE - EfficientQATOptimizer: Block-wise quantization optimization - Warmup epochs for gradual quantization introduction Calibration: - CalibrationHelper: Collects activation statistics via real forward passes - Supports INeuralNetworkModel, INeuralNetwork, and generic IFullModel - Falls back to parameter-based estimation when forward passes unavailable Integration: - Quantization wired into AiModelBuilder training pipeline - QuantizationInfo included in AiModelResult - 62 comprehensive integration tests Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary by CodeRabbit
WalkthroughIntegrates a full quantization subsystem: public config/enums, calibration/activation collectors, many quantizers/formats/strategies, QAT training hooks/optimizers, AiModelBuilder training integration with ApplyQuantizationIfConfigured, result-level QuantizationInfo, and comprehensive integration tests. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,220,255,0.5)
participant Builder as AiModelBuilder
participant Trainer as Trainer
participant Model as IFullModel
end
rect rgba(200,255,200,0.5)
participant Calibrator as CalibrationHelper
participant Quantizer as IQuantizer (selected)
end
rect rgba(255,230,200,0.5)
participant Result as AiModelResult
end
Builder->>Trainer: Start training → produce BestSolution (model)
Trainer->>Calibrator: CollectActivationStatistics(if configured)
Calibrator-->>Trainer: ActivationStatistics
Trainer->>Quantizer: Calibrate(model, data) / Quantize(model, config)
alt quantization succeeds
Quantizer-->>Trainer: QuantizedModel + QuantizationInfo
Trainer->>Model: replace BestSolution with QuantizedModel
Trainer->>Result: set QuantizationInfo in AiModelResultOptions
else quantization fails
Quantizer-->>Trainer: error/warnings
Trainer->>Result: leave BestSolution unchanged, QuantizationInfo = null/None
end
Trainer->>Result: return AiModelResult (includes QuantizationInfo)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR implements a comprehensive quantization framework for model compression, supporting both Post-Training Quantization (PTQ) and Quantization-Aware Training (QAT). The implementation addresses issue #278 with advanced quantization strategies (GPTQ, AWQ, SmoothQuant, SpinQuant, QuIP#), multiple numeric formats (INT8, FP8, NF4, MXFP4), QAT support with multiple methods, and calibration infrastructure for collecting activation statistics.
Changes:
- Adds 5 PTQ strategies (GPTQ, AWQ, SmoothQuant, SpinQuant, QuIP#) with Hessian-based optimization, activation-aware scaling, and learned rotations
- Implements 4 numeric formats (INT8, FP8 E4M3/E5M2, NF4, MXFP4) for different compression scenarios
- Integrates QAT support with fake quantization hooks and memory-efficient training methods
- Adds calibration infrastructure using real forward passes when available, with fallback to parameter estimation
- Wires quantization into AiModelBuilder training pipeline with proper result tracking
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Models/Results/QuantizationInfo.cs | Result class tracking quantization metrics (compression ratio, bit width, parameters) |
| src/Enums/QuantizationStrategy.cs | Enum defining PTQ strategies with comprehensive documentation |
| src/Enums/QuantizationGranularity.cs | Enum for quantization granularity levels (PerTensor, PerChannel, PerGroup) |
| src/Enums/QATMethod.cs | Enum for QAT training methods (Standard, EfficientQAT, ZeroQAT, ParetoQ) |
| src/Deployment/Optimization/Quantization/Training/QATTrainingHook.cs | Fake quantization application during training with STE |
| src/Deployment/Optimization/Quantization/Training/EfficientQATOptimizer.cs | Memory-efficient QAT with block-wise quantization |
| src/Deployment/Optimization/Quantization/Strategies/GPTQQuantizer.cs | GPTQ implementation with Hessian-based error compensation |
| src/Deployment/Optimization/Quantization/Strategies/AWQQuantizer.cs | AWQ implementation with activation-aware weight protection |
| src/Deployment/Optimization/Quantization/Strategies/SmoothQuantQuantizer.cs | SmoothQuant for W8A8 quantization with outlier smoothing |
| src/Deployment/Optimization/Quantization/Strategies/SpinQuantQuantizer.cs | SpinQuant with learned rotation matrices via Cayley parameterization |
| src/Deployment/Optimization/Quantization/Strategies/QuIPSharpQuantizer.cs | QuIP# for extreme 2-bit quantization using Hadamard transforms |
| src/Deployment/Optimization/Quantization/Formats/NF4Quantizer.cs | NF4 format implementation for QLoRA-style quantization |
| src/Deployment/Optimization/Quantization/Formats/MXFP4Quantizer.cs | MXFP4 microscaling format with shared exponents |
| src/Deployment/Optimization/Quantization/Formats/FP8Quantizer.cs | FP8 quantizer supporting E4M3 and E5M2 formats |
| src/Deployment/Optimization/Quantization/Calibration/CalibrationHelper.cs | Collects activation statistics via forward passes |
| src/Deployment/Optimization/Quantization/Calibration/ActivationStatistics.cs | Data structures for storing activation statistics |
| src/Deployment/Optimization/Quantization/QuantizationConfiguration.cs | Internal configuration with extensive options and factory methods |
| src/Deployment/Configuration/QuantizationConfig.cs | User-facing configuration with helper methods |
| src/AiModelBuilder.cs | Integration of quantization into training pipeline |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| catch (Exception ex) | ||
| { | ||
| Console.WriteLine($"Warning: QAT simulation failed: {ex.Message}. Proceeding with standard PTQ."); | ||
| } |
Check notice
Code scanning / CodeQL
Generic catch clause Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
In general, to fix a generic catch clause you identify the specific, expected exception types that can reasonably occur in the protected block and catch those explicitly. You then either rethrow or do not catch unexpected exceptions, so programming errors and fatal conditions are not silently swallowed. This preserves robustness for expected issues while maintaining debuggability and correctness.
For this specific block (lines 7214–7234), the code is performing QAT “simulation” (instantiating a hook, calling OnEpochStart, GetParameters, ApplyFakeQuantization, and WithParameters). Expected problems here are likely configuration- or argument-related, such as ArgumentException, InvalidOperationException, or possibly NotSupportedException, if the model or configuration does not support the requested QAT method. We should catch these non-fatal, predictable exceptions and keep the existing behavior (log and fall back to standard PTQ). Severe or unexpected exceptions (e.g., OutOfMemoryException, NullReferenceException) should not be swallowed.
Concretely, in src/AiModelBuilder.cs, replace the single catch (Exception ex) at line 7231 with multiple, more specific catch blocks, such as catch (ArgumentException ex), catch (InvalidOperationException ex), and a final catch (NotSupportedException ex). Each block can reuse the existing log message. We should not introduce any new dependencies or imports, since these exception types are in System and already available. No changes are needed elsewhere.
| @@ -7228,10 +7228,18 @@ | ||
|
|
||
| Console.WriteLine($"QAT simulation applied using {internalConfig.QATMethod} method"); | ||
| } | ||
| catch (Exception ex) | ||
| catch (ArgumentException ex) | ||
| { | ||
| Console.WriteLine($"Warning: QAT simulation failed: {ex.Message}. Proceeding with standard PTQ."); | ||
| } | ||
| catch (InvalidOperationException ex) | ||
| { | ||
| Console.WriteLine($"Warning: QAT simulation failed: {ex.Message}. Proceeding with standard PTQ."); | ||
| } | ||
| catch (NotSupportedException ex) | ||
| { | ||
| Console.WriteLine($"Warning: QAT simulation failed: {ex.Message}. Proceeding with standard PTQ."); | ||
| } | ||
| } | ||
|
|
||
| // Apply quantization - this returns a NEW model with quantized parameters |
| catch (Exception ex) | ||
| { | ||
| Console.WriteLine($"Warning: Quantization failed: {ex.Message}. Model will use original precision."); | ||
| } |
Check notice
Code scanning / CodeQL
Generic catch clause Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
General approach: replace the single broad catch (Exception) with more specific catch blocks for expected failures (e.g., ArgumentException, InvalidOperationException, potentially a domain‑specific quantization exception if available), and then add a final generic catch only if we still want a “last resort” safety net. This keeps robustness but avoids treating all failures as equivalent and lets us give more diagnostic information.
Best fix for this snippet without changing behavior:
- Keep the surrounding
tryblock intact. - Add explicit catch blocks for common runtime issues that might happen during quantization (
ArgumentException,InvalidOperationException, andNotSupportedExceptionare typical for configuration or operation‑not‑supported paths). - Adjust the existing generic catch to be last, and keep the same user‑visible behavior (warning and fallback), but expand the logged information slightly (e.g., include the exception type). This does not change functional behavior (still falls back to original precision) but helps diagnosis and makes it clear that we didn’t intend to swallow every exception type the same way.
- No new imports are needed, as these exception types are in
System.
Concretely, in src/AiModelBuilder.cs, around lines 2359–2363, replace the single catch (Exception ex) block with several specific catch blocks followed by a final generic catch. The rest of the method remains unchanged.
| @@ -2357,9 +2357,25 @@ | ||
| $"{quantizationInfo.BitWidth}-bit, compression ratio: {quantizationInfo.CompressionRatio:F2}x"); | ||
| } | ||
| } | ||
| catch (ArgumentException ex) | ||
| { | ||
| // Likely caused by invalid quantization configuration or incompatible model parameters | ||
| Console.WriteLine($"Warning: Quantization failed due to invalid argument: {ex.Message}. Model will use original precision."); | ||
| } | ||
| catch (InvalidOperationException ex) | ||
| { | ||
| // Likely caused by an unsupported operation during quantization | ||
| Console.WriteLine($"Warning: Quantization failed due to invalid operation: {ex.Message}. Model will use original precision."); | ||
| } | ||
| catch (NotSupportedException ex) | ||
| { | ||
| // Likely caused by unsupported quantization mode or target | ||
| Console.WriteLine($"Warning: Quantization is not supported in this configuration: {ex.Message}. Model will use original precision."); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Console.WriteLine($"Warning: Quantization failed: {ex.Message}. Model will use original precision."); | ||
| // Last-resort handler to avoid training failure when quantization unexpectedly breaks | ||
| Console.WriteLine($"Warning: Quantization failed due to unexpected error ({ex.GetType().Name}): {ex.Message}. Model will use original precision."); | ||
| } | ||
| } | ||
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Models/Results/AiModelResult.cs (1)
2340-2390:⚠️ Potential issue | 🟡 MinorPreserve
QuantizationInfoacross clone/deep-copy/deserialization.
Right now the new metadata is dropped when creating copies or rehydrating from JSON, so quantized models can appear unquantized in metadata.🔧 Proposed fix
// WithParameters options object DeploymentConfiguration = DeploymentConfiguration, // JIT compilation is parameter-specific, don't copy InferenceOptimizationConfig = InferenceOptimizationConfig, + QuantizationInfo = QuantizationInfo, ReasoningConfig = ReasoningConfig,// DeepCopy options object DeploymentConfiguration = DeploymentConfiguration, // JIT compilation is model-specific, don't copy InferenceOptimizationConfig = InferenceOptimizationConfig, + QuantizationInfo = QuantizationInfo, ReasoningConfig = ReasoningConfig,// Deserialize property assignments DeploymentConfiguration = deserializedObject.DeploymentConfiguration; + QuantizationInfo = deserializedObject.QuantizationInfo;Also applies to: 4173-4221, 4366-4389
🤖 Fix all issues with AI agents
In `@src/AiModelBuilder.cs`:
- Around line 2356-2370: Calibration is currently sampled from the raw x only
when x is TInput[]/IEnumerable<TInput>, causing skipped or mis-scaled
calibration for preprocessed tensors; change the source of calibrationData to
use the preprocessed data (e.g., preprocessedX or XTrain) and ensure
ApplyQuantizationIfConfigured(optimizationResult.BestSolution,
_quantizationConfig, calibrationData) always receives samples by: if
preprocessedX is an array or IEnumerable<TInput> take up to 128 items, else
create a single-element IEnumerable<TInput> containing a representative
preprocessed batch (or the first batch extracted from a Matrix/Tensor) as a safe
fallback so calibration is never null and uses preprocessed inputs.
In
`@src/Deployment/Optimization/Quantization/Calibration/ActivationStatistics.cs`:
- Around line 59-63: Move the public generic class LayerActivationStats<T> into
its own file: create a new file named LayerActivationStats.cs (same namespace as
ActivationStatistics.cs), copy the full class declaration and its usings into
it, remove the class from the original ActivationStatistics.cs so only one
public class remains per file, and ensure any references to
LayerActivationStats<T> still resolve (adjust namespace/usings if needed); if
your project requires explicit file listing, add the new file to the project
file.
- Around line 109-156: The Update and UpdatePerChannelStats methods in
ActivationStatistics<T> currently convert elements with Convert.ToDouble and use
doubles
(MinValue/MaxValue/MaxAbsValue/Mean/Variance/SampleCount/PerChannelMaxAbs);
replace those conversions and double arithmetic with the generic
INumericOperations<T> API: obtain a local var ops =
INumericOperations<T>.Instance, use ops.Abs, ops.Compare,
ops.Add/Subtract/Divide/Multiply to update
MinValue/MaxValue/MaxAbsValue/Mean/Variance/SampleCount in T-space (or use
T-typed accumulators where appropriate) and only convert to double when
storing/exporting a double result; similarly update PerChannelMaxAbs by
comparing ops.Abs(values) per channel and convert to double only at the boundary
where PerChannelMaxAbs must be double. Ensure references to Update,
UpdatePerChannelStats, PerChannelMaxAbs, Mean, Variance, SampleCount, MinValue,
MaxValue, MaxAbsValue are updated to use the INumericOperations<T> methods
rather than Convert.ToDouble/hardcoded double math.
In `@src/Deployment/Optimization/Quantization/Formats/MXFP4Quantizer.cs`:
- Around line 172-213: QuantizeWithMXFP4 currently recomputes and overwrites
block scales even when Calibrate/ComputeBlockScales already populated
_scaleFactors; modify QuantizeWithMXFP4 to first check the IsCalibrated flag
and/or presence of a scale for $"block_{b}" and use that existing value instead
of recomputing it, only computing and storing a scale (using the current
maxAbs/maxMXFP4 logic) if no calibrated scale exists; ensure you do not
overwrite _scaleFactors["block_{b}"] when it already exists, and keep the rest
of the loop (FindNearestMXFP4Value, NumOps.FromDouble, _blockSize handling)
unchanged so quantization uses the calibrated scales when available.
In `@src/Deployment/Optimization/Quantization/Training/EfficientQATOptimizer.cs`:
- Around line 162-172: The loop in EfficientQATOptimizer (the block iterating
from start to end that uses gradOutput, result and NumOps.FromDouble) reads a
`scale` value but never applies it; either apply the block scale to the gradient
(e.g., multiply grad by the retrieved `scale` before calling NumOps.FromDouble
and assigning into `result`) so the "Scale gradient by block scale" comment is
implemented, or remove the unused `scale` variable and update the comment to
indicate STE/no-scaling is intentional; update the code around the loop in
EfficientQATOptimizer.cs accordingly.
In `@src/Deployment/Optimization/Quantization/Training/QATTrainingHook.cs`:
- Around line 205-223: The asymmetric branch in QATTrainingHook.cs computes
zeroPoint using scale that can be zero when maxVal == minVal; to fix, ensure
scale is clamped to a small positive value before computing zeroPoint (e.g.,
max((maxVal - minVal) / qMax, epsilon)) and then clamp zeroPoint to the valid
quant range [qMin, qMax] (use the existing qMin/qMax computed from bitWidth);
apply the same guard and clamping logic wherever activation stats are updated
(the same asymmetric computation used elsewhere) and reference
_config.UseSymmetricQuantization, scale, zeroPoint, bitWidth, qMin, qMax when
making the change.
In `@src/Models/Results/QuantizationInfo.cs`:
- Line 174: The Warnings property initializer uses the C# 12 collection
expression "[]", which fails on older target frameworks; update the initializer
for the property IReadOnlyList<string> Warnings in QuantizationInfo (property
name: Warnings) to use a compatible allocation such as new List<string>() or
Array.Empty<string>()/new string[0], or alternatively confirm and update the
project targets in the .csproj to require .NET 8+ so C# 12 is supported.
🟡 Minor comments (17)
src/Deployment/Optimization/Quantization/Strategies/SmoothQuantQuantizer.cs-81-84 (1)
81-84:⚠️ Potential issue | 🟡 MinorMutating configuration object may cause unexpected behavior.
Similar to QuIPSharpQuantizer, this constructor modifies the
_configobject'sStrategyproperty if it doesn't match. Consider creating a defensive copy instead.tests/AiDotNet.Tests/IntegrationTests/Quantization/QuantizationIntegrationTests.cs-85-93 (1)
85-93:⚠️ Potential issue | 🟡 MinorUnused variable
uniqueValuesin test assertion.The
HashSet<double> uniqueValuesis declared but never populated or used. The test appears incomplete - it should verify that quantized values have limited unique values (as the comment suggests) but the actual assertion only checks if values are on the quantization grid.🔧 Proposed fix to remove unused variable
// Verify parameters are quantized (should have limited unique values) - var uniqueValues = new HashSet<double>(); for (int i = 0; i < quantizedParams.Length; i++) { // Scale factor determines the quantization step sizetests/AiDotNet.Tests/IntegrationTests/Quantization/QuantizationIntegrationTests.cs-1537-1541 (1)
1537-1541:⚠️ Potential issue | 🟡 MinorAssertion logic may be confusing and potentially incorrect.
The assertion at line 1537-1538 uses a double-negative logical condition that's hard to follow. The condition
hook.IsQuantizationEnabled == false || duringWarmup[0] != weights[0]doesn't clearly express the intended behavior. During warmup, quantization should be disabled and values should pass through unchanged.🔧 Proposed fix for clearer assertions
// Assert - Assert.False(hook.IsQuantizationEnabled == false || duringWarmup[0] != weights[0], - "Quantization should be disabled during warmup (epoch 0 < 2)"); + Assert.False(hook.IsQuantizationEnabled, + "Quantization should be disabled during warmup (epoch 0 < 2)"); + Assert.Equal(weights[0], duringWarmup[0], 10); Assert.True(hook.IsQuantizationEnabled, "Quantization should be enabled after warmup");src/Deployment/Optimization/Quantization/Strategies/QuIPSharpQuantizer.cs-80-83 (1)
80-83:⚠️ Potential issue | 🟡 MinorMutating configuration object may cause unexpected behavior.
The constructor modifies the passed-in
_configobject'sStrategyproperty if it doesn't matchQuIPSharp. This mutation of a configuration object that may be shared could cause unexpected behavior. Consider creating a copy or throwing an exception instead.🔧 Proposed fix to avoid config mutation
+ // Make a copy to avoid mutating shared config + if (config != null && config.Strategy != QuantizationStrategy.QuIPSharp) + { + _config = new QuantizationConfiguration + { + Mode = config.Mode, + Strategy = QuantizationStrategy.QuIPSharp, + TargetBitWidth = config.TargetBitWidth ?? 2, + Granularity = config.Granularity, + GroupSize = config.GroupSize, + UseSymmetricQuantization = config.UseSymmetricQuantization + }; + } + else + { _config = config ?? new QuantizationConfiguration { Mode = QuantizationMode.Int8, Strategy = QuantizationStrategy.QuIPSharp, TargetBitWidth = 2, Granularity = QuantizationGranularity.PerGroup, GroupSize = groupSize, UseSymmetricQuantization = true }; - - if (_config.Strategy != QuantizationStrategy.QuIPSharp) - { - _config.Strategy = QuantizationStrategy.QuIPSharp; }src/Deployment/Optimization/Quantization/Strategies/QuIPSharpQuantizer.cs-47-47 (1)
47-47:⚠️ Potential issue | 🟡 MinorUnused
HadamardCachedictionary.The
HadamardCachestatic dictionary is declared but never populated or used. The current implementation uses the fast in-place Walsh-Hadamard transform instead. Remove this unused field to avoid confusion.🧹 Proposed fix to remove unused field
private readonly int _groupSize; - // Pre-computed Hadamard matrices for common sizes - private static readonly Dictionary<int, double[,]> HadamardCache = new(); - // E8 lattice codebook values for 2-bit quantization (4 levels) private static readonly double[] LatticeCodebook2Bit = { -1.0, -0.333, 0.333, 1.0 };src/Deployment/Optimization/Quantization/Formats/NF4Quantizer.cs-108-152 (1)
108-152:⚠️ Potential issue | 🟡 MinorCalibration is effectively a no‑op right now.
Calibratecomputes per‑block scales but the quantization path recomputes and overwrites them, andcalibrationDatais never used. Either use the calibration data to derive scales or reuse the cached scales whenIsCalibratedto make calibration meaningful.🔧 Make calibration scales take effect
- // Compute block scale (absmax) - double maxAbs = 0; - for (int i = start; i < end; i++) - { - maxAbs = Math.Max(maxAbs, Math.Abs(Convert.ToDouble(parameters[i]))); - } - - double scale = maxAbs > 0 ? maxAbs : 1.0; - _scaleFactors[$"block_{b}"] = scale; + double scale; + if (IsCalibrated && _scaleFactors.TryGetValue($"block_{b}", out var cachedScale)) + { + scale = cachedScale; + } + else + { + double maxAbs = 0; + for (int i = start; i < end; i++) + { + maxAbs = Math.Max(maxAbs, Math.Abs(Convert.ToDouble(parameters[i]))); + } + + scale = maxAbs > 0 ? maxAbs : 1.0; + _scaleFactors[$"block_{b}"] = scale; + }Also applies to: 168-176
src/Deployment/Optimization/Quantization/Strategies/AWQQuantizer.cs-138-239 (1)
138-239:⚠️ Potential issue | 🟡 MinorClear per‑run state to avoid stale scales/zero‑points.
_scaleFactorsand_zeroPointsare never cleared, so repeated quantizations can include stale values and corrupt the “global” averages or per‑group lookup. Clear these at the start of quantization to keep state consistent.🧹 Proposed fix
private Vector<T> QuantizeWithAWQ(Vector<T> parameters, QuantizationConfiguration config) { + _scaleFactors.Clear(); + _zeroPoints.Clear(); + int n = parameters.Length; int groupSize = config.Granularity == QuantizationGranularity.PerGroup ? config.GroupSize : n;src/Deployment/Optimization/Quantization/QuantizationConfiguration.cs-123-124 (1)
123-124:⚠️ Potential issue | 🟡 MinorDeprecation warning is helpful, but
BitWidthstill returnsDefaultBitWidth, notEffectiveBitWidth.The
[Obsolete]message tells users to useEffectiveBitWidth, butBitWidthreturnsDefaultBitWidthwhich ignoresTargetBitWidth. This could cause confusion. Consider either:
- Having
BitWidthreturnEffectiveBitWidthfor backward compatibility, or- Making it clearer that the values differ
src/Deployment/Optimization/Quantization/Strategies/SpinQuantQuantizer.cs-71-94 (1)
71-94:⚠️ Potential issue | 🟡 MinorConstructor silently mutates the passed configuration object.
Same issue as GPTQQuantizer: if a non-null config with different strategy is passed, the constructor mutates it at line 88. Consider throwing instead or creating a defensive copy.
src/Deployment/Optimization/Quantization/Formats/MXFP4Quantizer.cs-101-110 (1)
101-110:⚠️ Potential issue | 🟡 Minor
Quantizeignores theconfigparameter.The method signature accepts a
QuantizationConfiguration configparameter but uses_config(from constructor) and_blockSizethroughout. Either use the passed config or remove the parameter from the interface if it's not needed.src/Deployment/Optimization/Quantization/Training/EfficientQATOptimizer.cs-55-61 (1)
55-61:⚠️ Potential issue | 🟡 MinorStarting bit width logic may produce unexpected values.
Line 59 sets
_currentBitWidth = Math.Min(32, _config.EffectiveBitWidth * 2). IfEffectiveBitWidthis 4, this starts at 8, butUpdateProgressiveQuantization(line 267) usesMath.Min(16, ...)as the start. This inconsistency means the constructor's initial value may never be used ifQATMethod == EfficientQAT.src/Deployment/Optimization/Quantization/Training/EfficientQATOptimizer.cs-99-102 (1)
99-102:⚠️ Potential issue | 🟡 MinorInteger overflow risk with bit shift operations.
Same issue as GPTQQuantizer:
1 << (effectiveBitWidth - 1)overflows foreffectiveBitWidth >= 32. Use1L <<for safety.🛠️ Suggested fix
-double qMin = _config.UseSymmetricQuantization ? -(1 << (effectiveBitWidth - 1)) : 0; -double qMax = _config.UseSymmetricQuantization ? (1 << (effectiveBitWidth - 1)) - 1 : (1 << effectiveBitWidth) - 1; +double qMin = _config.UseSymmetricQuantization ? -(1L << (effectiveBitWidth - 1)) : 0; +double qMax = _config.UseSymmetricQuantization ? (1L << (effectiveBitWidth - 1)) - 1 : (1L << effectiveBitWidth) - 1;src/Deployment/Optimization/Quantization/Strategies/GPTQQuantizer.cs-151-153 (1)
151-153:⚠️ Potential issue | 🟡 MinorInteger overflow risk for high bit widths.
The expressions
1 << (bitWidth - 1)and1 << bitWidthwill overflow whenbitWidth >= 32. Although typical use is 4-8 bits, the API accepts anyEffectiveBitWidth. Consider using1L << ...or clamping bit width.🛠️ Suggested fix
-double qMin = config.UseSymmetricQuantization ? -(1 << (bitWidth - 1)) : 0; -double qMax = config.UseSymmetricQuantization ? (1 << (bitWidth - 1)) - 1 : (1 << bitWidth) - 1; +double qMin = config.UseSymmetricQuantization ? -(1L << (bitWidth - 1)) : 0; +double qMax = config.UseSymmetricQuantization ? (1L << (bitWidth - 1)) - 1 : (1L << bitWidth) - 1;src/Deployment/Optimization/Quantization/Strategies/GPTQQuantizer.cs-70-78 (1)
70-78:⚠️ Potential issue | 🟡 MinorConstructor silently mutates the passed configuration object.
If a non-null
configis provided with a differentStrategy, the constructor modifies_config.Strategydirectly. Since_configholds a reference to the caller's object, this mutates state outside the class unexpectedly.Consider creating a defensive copy or throwing if the strategy doesn't match:
🛠️ Suggested fix
public GPTQQuantizer(QuantizationConfiguration? config = null) { - _config = config ?? QuantizationConfiguration.ForGPTQ(); - - if (_config.Strategy != QuantizationStrategy.GPTQ) - { - _config.Strategy = QuantizationStrategy.GPTQ; - } + if (config != null && config.Strategy != QuantizationStrategy.GPTQ) + { + throw new ArgumentException( + $"Configuration must use GPTQ strategy, but was {config.Strategy}", nameof(config)); + } + _config = config ?? QuantizationConfiguration.ForGPTQ(); }src/Deployment/Optimization/Quantization/Strategies/SpinQuantQuantizer.cs-475-504 (1)
475-504:⚠️ Potential issue | 🟡 Minor
QuantizeSymmetricoverwrites the global scale factor.Both
ComputeScaleFactors(called during Calibrate) andQuantizeSymmetric(called during Quantize) write to_scaleFactors["global"]. The calibration scale is overwritten during quantization, which may not be intentional.src/Deployment/Optimization/Quantization/Strategies/GPTQQuantizer.cs-252-257 (1)
252-257:⚠️ Potential issue | 🟡 MinorGlobal scale averaging may produce misleading values.
The global scale is computed as the average of all group scales, but
_scaleFactorsalso contains non-group keys (like"global"itself after the first run). This could skew the average on subsequent quantizations if the cache isn't cleared.🛠️ Suggested fix
// Store global scale (average) -if (_scaleFactors.Count > 0) +var groupScales = _scaleFactors.Where(kv => kv.Key.StartsWith("group_")).Select(kv => kv.Value).ToList(); +if (groupScales.Count > 0) { - _scaleFactors["global"] = _scaleFactors.Values.Average(); + _scaleFactors["global"] = groupScales.Average(); _zeroPoints["global"] = 0; }src/Deployment/Optimization/Quantization/Strategies/SpinQuantQuantizer.cs-174-197 (1)
174-197:⚠️ Potential issue | 🟡 MinorOptimization loop has numerical stability issues.
- The Cayley parameter update at lines 180-193 enforces skew-symmetry by setting
cayleyParam[j, i] = -cayleyParam[i, j]only wheni < j, but gradient descent updates both[i,j]and[j,i]independently before this correction, causing inconsistent states during the inner loop.- Setting diagonal to zero at line 192 happens inside the
jloop, overwriting values multiple times unnecessarily.🛠️ Suggested restructure
// Update Cayley parameter for (int i = 0; i < rotationSize; i++) { - for (int j = 0; j < rotationSize; j++) + for (int j = i + 1; j < rotationSize; j++) // Only update upper triangle { - cayleyParam[i, j] -= _learningRate * gradient[i, j]; - - // Enforce skew-symmetry: A[i,j] = -A[j,i] - if (i < j) - { - cayleyParam[j, i] = -cayleyParam[i, j]; - } + double update = _learningRate * (gradient[i, j] - gradient[j, i]) / 2; + cayleyParam[i, j] -= update; + cayleyParam[j, i] = -cayleyParam[i, j]; // Enforce skew-symmetry } - cayleyParam[i, i] = 0; // Diagonal must be zero + // Diagonal is already zero for skew-symmetric matrices }
🧹 Nitpick comments (17)
tests/AiDotNet.Tests/IntegrationTests/Quantization/QuantizationIntegrationTests.cs (2)
23-23: Unused constantTolerance.The constant
Toleranceis declared but never used in any of the test assertions. Consider removing it or using it in tests that compare floating-point values.
45-57: Consider returningIEnumerable<double[]>eagerly to avoid repeated enumeration.The
CreateCalibrationDatamethod usesyield return, which is lazy. Several tests call.ToList()on the result, which is correct. However, since most consumers materialize the collection anyway, consider returningList<double[]>directly to avoid potential multiple-enumeration issues if a caller forgets.ToList().src/Deployment/Optimization/Quantization/Formats/FP8Quantizer.cs (2)
191-192: NaN handling returns 0, which may silently mask data issues.When the input value is NaN, the method returns 0 without any warning or logging. This could mask data corruption or numerical instability issues in the model weights. Consider tracking or logging NaN occurrences during quantization.
430-446: Consider moving FP8Format enum to a separate file.Per PR objectives stating "one class per file and namespaces matching folders," the
FP8Formatenum should ideally be in its own file. However, since it's tightly coupled with the quantizer and relatively small, this is a minor organizational concern.src/Deployment/Optimization/Quantization/Calibration/CalibrationHelper.cs (4)
135-139: Silent catch blocks may hide legitimate errors.The empty catch blocks that silently continue to the next sample could mask important errors like out-of-memory conditions or type mismatches. Consider at least logging or counting these failures for diagnostics.
🔧 Proposed fix to track failures
+ int failedSamples = 0; foreach (var sample in samples) { try { // ... existing code ... } catch { - // Skip samples that fail - continue with remaining - continue; + // Skip samples that fail - continue with remaining + failedSamples++; } } + + if (failedSamples > 0 && stats.SampleCount == 0) + { + throw new InvalidOperationException($"All {failedSamples} calibration samples failed processing"); + }
372-383:CanRunPredictionsmethod provides no real validation.The method always returns
truefor non-null models and catches any exception to returnfalse, but it doesn't actually test if predictions can run. This provides a false sense of safety. Consider removing the try-catch or actually attempting a prediction.🔧 Proposed simplification
private bool CanRunPredictions(IFullModel<T, TInput, TOutput> model) { - // Check if model has been trained and can make predictions - try - { - return model != null; - } - catch - { - return false; - } + return model != null; }
410-426: Reflection-based conversion may have performance implications.The fallback to reflection-based
ToArray()method invocation could be slow when processing many calibration samples. This is acceptable as a last-resort fallback, but consider documenting this performance characteristic.
456-467: Potential boxing overhead for scalar output.The pattern
if (output is T scalar)will always match whenTOutputisT, which could cause unintended behavior. For value types, this also involves boxing. Consider checking this case more explicitly or reordering the type checks.src/Deployment/Optimization/Quantization/Strategies/SmoothQuantQuantizer.cs (2)
259-262: Per-channel estimation uses heuristic that may not match actual layer structure.The per-channel quantization estimates the number of channels as
sqrt(n), which assumes a square-ish parameter distribution. This heuristic may not accurately reflect the actual layer structure, potentially leading to suboptimal quantization. Consider accepting layer structure information from the model metadata if available.
306-309: Computing average of all scale factors may be misleading.Setting the global scale factor as the average of all channel/group scale factors may not be meaningful and could be confusing when retrieved via
GetScaleFactor("global"). Consider using max, median, or documenting this behavior clearly.src/Deployment/Optimization/Quantization/Formats/NF4Quantizer.cs (1)
97-106: Validate or honor theconfigargument to avoid silent mismatches.
Quantizeignores the passed configuration, so callers can supply incompatible settings without any signal. Consider validating bit‑width and group size to prevent surprising results.🔧 Possible guard
public IFullModel<T, TInput, TOutput> Quantize(IFullModel<T, TInput, TOutput> model, QuantizationConfiguration config) { if (model == null) throw new ArgumentNullException(nameof(model)); + if (config.TargetBitWidth != 4) + throw new ArgumentException("NF4 requires TargetBitWidth = 4.", nameof(config)); + if (config.Granularity == QuantizationGranularity.PerGroup && config.GroupSize != _blockSize) + throw new ArgumentException("NF4 block size must match config.GroupSize.", nameof(config)); var parameters = model.GetParameters(); var quantizedParams = QuantizeWithNF4(parameters);src/Deployment/Optimization/Quantization/Strategies/AWQQuantizer.cs (1)
251-331: Preserve activation scales even when length < parameter count.When
_activationScales["global"]is shorter thann, the current logic falls back to uniform scales, effectively disabling AWQ. Consider expanding/tiling the activation scales instead of dropping them entirely.♻️ Possible expansion strategy
private double[] GetActivationScales(int n) { - if (_activationScales.TryGetValue("global", out var scales) && scales.Length >= n) - { - return scales; - } + if (_activationScales.TryGetValue("global", out var scales) && scales.Length > 0) + { + if (scales.Length >= n) return scales; + var expanded = new double[n]; + for (int i = 0; i < n; i++) + { + expanded[i] = scales[i % scales.Length]; + } + return expanded; + }src/Deployment/Optimization/Quantization/Strategies/GPTQQuantizer.cs (2)
387-404: Condition at lines 391-392 is always true and redundant.The modulo calculation
i - (i / hessianDiag.Length * hessianDiag.Length)equalsi % hessianDiag.Length, which is always< hessianDiag.Length. The check is tautological and the subsequent check at line 397 is also always true sinceiLocalandjLocalare computed via modulo.♻️ Simplified version
private double GetHessianCrossElement(int i, int j, double[] hessianDiag) { - // Simplified: use geometric mean of diagonals for off-diagonal elements - // Full GPTQ would use actual H^-1 elements from Cholesky decomposition - if (i - (i / hessianDiag.Length * hessianDiag.Length) < hessianDiag.Length && - j - (j / hessianDiag.Length * hessianDiag.Length) < hessianDiag.Length) - { - int iLocal = i % hessianDiag.Length; - int jLocal = j % hessianDiag.Length; - - if (iLocal < hessianDiag.Length && jLocal < hessianDiag.Length) - { - return Math.Sqrt(hessianDiag[iLocal] * hessianDiag[jLocal]) * 0.1; - } - } - - return _config.GPTQDampingFactor; + // Simplified: use geometric mean of diagonals for off-diagonal elements + // Full GPTQ would use actual H^-1 elements from Cholesky decomposition + int iLocal = i % hessianDiag.Length; + int jLocal = j % hessianDiag.Length; + return Math.Sqrt(hessianDiag[iLocal] * hessianDiag[jLocal]) * 0.1; }
406-413: Nested classHessianInfocould userequiredor constructor for strict initialization.Per PR objectives, strict property initialization is required (no
default!). TheDiagonalproperty is nullable butSizedefaults to 0, which may be unintentional. Consider using a constructor orrequiredmodifier.src/Deployment/Optimization/Quantization/Training/EfficientQATOptimizer.cs (1)
335-359:BlockQuantizationStateclass should be in its own file.Per PR objectives: "one class per file and namespaces matching folders." Move
BlockQuantizationStateto a separate file.src/Deployment/Optimization/Quantization/Strategies/SpinQuantQuantizer.cs (1)
386-394: Singular matrix handling returns identity without warning.When the matrix is singular (pivot < 1e-10), the method silently returns an identity matrix. This could mask numerical issues during rotation learning. Consider logging a warning or throwing.
src/Deployment/Optimization/Quantization/Formats/MXFP4Quantizer.cs (1)
245-266:EncodeToMXFP4andDecodeFromMXFP4are public but appear to be internal helpers.These static methods are exposed publicly but seem intended for internal use. If they're part of the public API for serialization/deserialization, consider documenting this use case. Otherwise, mark them
internalorprivate.
- EfficientQATOptimizer: make _totalEpochs readonly, use scale in gradient computation - CalibrationHelper: remove unused avgMaxAbs, use explicit Exception catches - FP8Quantizer: remove unused fp8MinSubnormal, use ternary for format selection - MXFP4Quantizer: reuse calibrated scale factors if available - GPTQQuantizer: use ternary for Hessian diagonal assignment - SpinQuantQuantizer: actually use calibration data for activation statistics - QATTrainingHook: guard zero-point computation after scale clamping - QuantizationInfo: replace [] with Array.Empty for net471 compatibility - AiModelBuilder: use explicit Exception catches for generic catch clauses Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Use XTrain (preprocessed) instead of raw x for calibration data - Handle non-enumerable datasets by wrapping single items - Estimate parameter size based on actual type T instead of assuming double Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- SmoothQuantQuantizer: use log-space computation for numerical stability - QuantizationConfiguration: add configurable AWQScaleSearchOptions - AWQQuantizer: use configurable scale search options instead of hardcoded values Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
IEEE FP8 E4M3 can represent NaN but we map to 0 for safer ML inference Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Fix all issues with AI agents
In `@src/AiModelBuilder.cs`:
- Around line 7207-7215: The size calculations use int multiplication and can
overflow for large models; change the multiplications to use long by casting
operands before multiplying (e.g., when computing originalSizeBytes use
(long)parameters.Length * bytesPerParameter instead of parameters.Length *
bytesPerParameter) and likewise cast before computing quantized size (e.g.,
(long)quantizedParameters.Length * EffectiveBitWidth). Update the expressions in
the block around model.GetParameters() and the similar block that computes
quantizedParameters.Length * EffectiveBitWidth so the product is evaluated in
64-bit arithmetic.
- Around line 7190-7205: The MinMax/Dynamic branch currently silently defaults
to Int8 for any QuantizationMode not Int8/Float16, which causes FP8/NF4/MXFP4
configs to be mis-applied; update the branch in AiModelBuilder.cs where
internalConfig.Mode is checked so that FP8/NF4/MXFP4 modes are explicitly
handled (instantiate the correct quantizer types if available, e.g.,
FP8Quantizer/NF4Quantizer/MXFP4Quantizer in
Deployment.Optimization.Quantization) or throw a clear exception listing
unsupported modes, instead of returning new
Deployment.Optimization.Quantization.Int8Quantizer<T, TInput, TOutput>(); ensure
you reference internalConfig.Mode, QuantizationMode, and the quantizer classes
(e.g., Int8Quantizer, Float16Quantizer, FP8Quantizer) when making the change so
the code fails fast for unknown/unsupported modes.
In `@src/Deployment/Optimization/Quantization/Calibration/CalibrationHelper.cs`:
- Around line 151-184: The code sets the model into inference mode with
model.SetTrainingMode(false) and never restores the prior training mode; capture
the original mode before changing it, wrap the sampling loop in try/finally, and
call model.SetTrainingMode(originalMode) in the finally block so the model's
training state is always restored after the loop (reference
model.SetTrainingMode, samples, and the sampling loop that updates
stats.LayerStats and stats.SampleCount).
- Around line 395-401: The code uses C# 12 collection expressions for tensor
shapes in CalibrationHelper (the Tensor<T> constructors in the method that
handles Matrix<T> and T[] samples); replace the `[matrix.Rows, matrix.Columns]`
and `[1, array.Length]` expressions with backward-compatible array initializers
(e.g., new int[] { ... } or new[] { ... }) so the project compiles under net471,
or alternatively set LangVersion to 12.0 in the project file if you intend to
keep the C#12 syntax; update the two Tensor<T> calls accordingly.
In `@src/Deployment/Optimization/Quantization/Formats/MXFP4Quantizer.cs`:
- Around line 101-110: The Quantize method currently ignores the incoming
QuantizationConfiguration; update IFullModel<T, TInput, TOutput>.Quantize
implementation to respect the config by validating it and passing its settings
into the quantization routine instead of using constructor defaults: check for
null config and validate fields (e.g., group size, bit width), throw
ArgumentException for unsupported values, then call QuantizeWithMXFP4 (or an
overload) with the config-derived parameters and return
model.WithParameters(quantizedParams); reference the Quantize method and the
QuantizeWithMXFP4 helper to locate changes.
- Around line 175-223: The QuantizeWithMXFP4 method currently reuses
_scaleFactors entries keyed only by block index (block_{b}), which can leak
scales across different models; update the logic so _scaleFactors cannot be
reused across models—either incorporate a model-specific prefix into the
blockKey (e.g., include a modelId or weightBufferId when constructing blockKey)
or ensure the quantizer clears or re-initializes _scaleFactors when a new model
is loaded; modify the code paths that set or read _scaleFactors in
QuantizeWithMXFP4 to use the new model-scoped key or to check a modelVersion
flag so stale scales are never applied to a different model.
In `@src/Deployment/Optimization/Quantization/QuantizationConfiguration.cs`:
- Line 71: Granularity defaults to PerChannel while UsePerChannelQuantization
defaults to false causing inconsistent/overlapping behavior; reconcile by
deprecating UsePerChannelQuantization and mapping it to Granularity (or choose a
single source of truth): mark UsePerChannelQuantization as obsolete, implement
its getter/setter to return/set Granularity ==
QuantizationGranularity.PerChannel so both stay consistent, and ensure the
default value is driven from Granularity (keep Granularity default PerChannel
and remove independent defaulting for UsePerChannelQuantization) or
alternatively remove UsePerChannelQuantization entirely and update callers to
use Granularity.
- Around line 251-260: The AWQProtectionPercentage property is ambiguous about
whether it uses a 0–1 ratio or 0–100 percentage; decide on a single
representation, then update the property and docs accordingly: either (A) keep a
percentage name and set default to 1.0 with doc stating "1.0 = 1%" and validate
range 0–100, or (B) switch to a ratio by renaming AWQProtectionPercentage to
AWQProtectionRatio (or change default to 0.01) and update the XML remarks to
"0.0–1.0, default 0.01 = 1%"; update all usages of AWQProtectionPercentage/
AWQProtectionRatio, adjust unit wording in the summary/remarks, and add a guard
(e.g., range check) in any setter/validation logic to enforce the chosen scale.
In `@src/Deployment/Optimization/Quantization/Strategies/AWQQuantizer.cs`:
- Around line 317-331: GetActivationScales currently drops calibration when the
stored scales array is shorter than the parameter count (n); update
GetActivationScales to map/expand shorter calibration arrays instead of
returning a uniform default: when _activationScales contains an entry (e.g.,
"global") with scales.Length < n, produce a result of length n by
repeating/tiling or mapping per-channel/per-layer (e.g., use modulo indexing or
a per-group mapping strategy) so calibrated values are applied across the full
parameter vector rather than replaced by defaultScales; keep the existing return
when no scales entry exists but avoid discarding existing calibration when
scales.Length > 0 and < n.
In `@src/Deployment/Optimization/Quantization/Strategies/GPTQQuantizer.cs`:
- Around line 164-234: The Hessian diagonal returned by
GetHessianDiagonal(groupStart, actualGroupSize) is indexed as if group indices
are contiguous, but when ActOrder is enabled the processing order[] permutes
indices so hessianDiag is misaligned; fix by fetching or mapping the Hessian per
the ordered index (use idx = order[i]) before using it in the OBS update: either
call GetHessianDiagonal for each ordered index or build hessianDiagMapped[k] =
originalHessian[order[groupStart+k]] and then use hDiag = hessianDiagMapped[i -
groupStart] where hDiag is currently read; update references in the quantization
loop (symbols: GetHessianDiagonal, order, hessianDiag, idx, groupStart/groupEnd)
so error compensation uses the correct curvature entries.
In `@src/Deployment/Optimization/Quantization/Strategies/SmoothQuantQuantizer.cs`:
- Around line 239-244: In SmoothQuantQuantizer, after computing the asymmetric
zeroPoint (the branches that set scale and zeroPoint using bitWidth, min/max and
config.MinScaleFactor), clamp zeroPoint to the quant range [qMin, qMax] (e.g.
zeroPoint = Math.Max(qMin, Math.Min(qMax, zeroPoint))) so it cannot fall outside
the representable quantized range; apply this clamp in the same pattern for the
other asymmetric zeroPoint computations referenced (the blocks around the other
occurrences).
In `@src/Deployment/Optimization/Quantization/Strategies/SpinQuantQuantizer.cs`:
- Around line 103-117: The returned model currently contains
rotated-and-quantized weights but never applies the inverse rotation (R^T),
which changes semantics; after QuantizeSymmetric you must undo the learned
rotation before returning or ensure inference applies R^T. Fix by computing the
inverse/transpose of the rotation produced by LearnRotationMatrix (use
rotation.Transpose() or an ApplyInverseRotation(parameters, rotation) helper),
apply that inverse to quantizedRotated to restore weights to the original space,
and then return model.WithParameters(restoredParams); alternatively, persist
_rotationMatrices["global"] and wrap affected layers so their forward passes
multiply by R^T, but do not return rotated weights directly from
QuantizeSymmetric without restoring them.
In `@src/Deployment/Optimization/Quantization/Training/EfficientQATOptimizer.cs`:
- Around line 281-332: The asymmetric path currently computes blockZeroPoints[b]
before clamping blockScales[b], which can divide by zero when maxVal==minVal;
fix InitializeBlockState by computing the raw scale first (e.g., scaleRaw =
(maxVal - minVal) / ((1 << effectiveBitWidth) - 1)), clamp it with
blockScales[b] = Math.Max(scaleRaw, _config.MinScaleFactor), then compute
blockZeroPoints[b] = (int)Math.Round(-minVal / blockScales[b]) and finally clamp
that zero point into the valid quant range [0, qMax]; keep the symmetric branch
unchanged and ensure you reference blockScales[b] and blockZeroPoints[b] when
assigning.
In `@src/Deployment/Optimization/Quantization/Training/QATTrainingHook.cs`:
- Around line 114-120: The activation state is initialized with default (weight)
bit-width then overwritten, so QuantMin/QuantMax/Scale are computed using the
wrong bit-width; change InitializeLayerState to accept a bit-width parameter and
call it with _config.ActivationBitWidth when creating activation state
(activationKey path) so the returned LayerState.BitWidth,
QuantMin/QuantMax/Scale are computed from the activation bit width; update all
other activation-initialization sites (the same pattern around the block
referenced at lines ~190-243) to pass the activation bit width as well and
remove the post-assignment of BitWidth.
🧹 Nitpick comments (2)
src/Deployment/Optimization/Quantization/Formats/MXFP4Quantizer.cs (1)
147-170: Persist a global scale for metadata consumers.
GetScaleFactorwill usually return1.0because onlyblock_{b}keys are stored. Consider saving an aggregate (global) scale during computation so metadata isn’t misleading.♻️ Suggested addition
private void ComputeBlockScales(Vector<T> parameters) { int n = parameters.Length; int numBlocks = (n + _blockSize - 1) / _blockSize; + double scaleSum = 0; for (int b = 0; b < numBlocks; b++) { int start = b * _blockSize; int end = Math.Min(start + _blockSize, n); ... _scaleFactors[$"block_{b}"] = scale; + scaleSum += scale; } + + if (numBlocks > 0) + { + _scaleFactors["global"] = scaleSum / numBlocks; + } }src/Deployment/Optimization/Quantization/QuantizationConfiguration.cs (1)
262-270: Consider usingIReadOnlyList<double>or defensive copying forAWQScaleSearchOptions.Exposing a mutable
double[]allows callers to accidentally mutate the configuration's internal state. For a configuration object, this could lead to subtle bugs.♻️ Proposed refactor to use immutable collection
- public double[] AWQScaleSearchOptions { get; set; } = [0.0, 0.1, 0.2, 0.3, 0.5, 0.7, 1.0, 1.5, 2.0]; + public IReadOnlyList<double> AWQScaleSearchOptions { get; set; } = [0.0, 0.1, 0.2, 0.3, 0.5, 0.7, 1.0, 1.5, 2.0];
- Fix int overflow in quantizedSizeBytes calculation (AiModelBuilder.cs) - Fix floating-point equality checks using epsilon comparison (FP8Quantizer.cs) - Improve NF4 codebook documentation with QLoRA reference (NF4Quantizer.cs) - Use adaptive epsilon for gradient computation (SpinQuantQuantizer.cs) - Add calibration failure tracking with warnings (CalibrationHelper.cs) - Add CalibrationWarnings property to ActivationStatistics Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Fix more floating-point equality checks in FP8Quantizer (ToE4M3, ToE5M2) - Add detailed QAT simulation documentation explaining it's post-training only - Improve exception logging to include exception type and inner exception - Clarify that true QAT requires training loop integration Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add comprehensive documentation for ConvertToTensor supported types - Add return type validation for reflection-based ToArray conversion - Simplify and document GetHessianCrossElement index mapping logic - Add zero-length array guard in Hessian cross-element calculation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Deployment/Optimization/Quantization/Calibration/CalibrationHelper.cs`:
- Around line 498-507: The assignability check in the calibration helper is
inverted: instead of checking returnType.IsAssignableFrom(typeof(T[])), change
the condition to check whether typeof(T[]).IsAssignableFrom(returnType) (or
simply compare returnType == typeof(T[]) ||
typeof(T[]).IsAssignableFrom(returnType)) so you correctly detect when the
method's ReturnType can be assigned to T[] before invoking toArrayMethod; keep
the rest of the logic that invokes toArrayMethod and constructs the Tensor<T>
(using toArrayMethod, returnType, Tensor<T>, Vector<T>) unchanged.
In `@src/Deployment/Optimization/Quantization/Training/EfficientQATOptimizer.cs`:
- Around line 371-377: The property initializers BlockScales and BlockZeroPoints
use C# 12 collection expressions (= []) which break net471 compatibility;
replace those initializers with Array.Empty<double>() for BlockScales and
Array.Empty<int>() for BlockZeroPoints so the properties become
backward-compatible while preserving empty-array semantics (update the
initializers on the BlockScales and BlockZeroPoints properties).
🧹 Nitpick comments (5)
src/Deployment/Optimization/Quantization/Calibration/CalibrationHelper.cs (2)
439-451:CanRunPredictionsalways returnstruefor non-null models.The
try-catchblock is ineffective becausemodel != nullnever throwsInvalidOperationException. The method always returnstruefor any non-null model, making the exception handling dead code.If the intent is to verify the model is ready for predictions, consider checking a specific property or method that indicates training completion.
♻️ Simplified implementation
private static bool CanRunPredictions(IFullModel<T, TInput, TOutput> model) { - // Check if model has been trained and can make predictions - try - { - return model != null; - } - catch (InvalidOperationException) - { - // Model not trained or not ready for predictions - return false; - } + // Model is non-null at this point due to caller validation + return true; }
551-554: Patternoutput is T scalarmay incorrectly match non-scalar outputs.When
Tis a value type (e.g.,double),output is Twill match any boxedTvalue. However, sinceTOutputis a generic type parameter, ifTOutputhappens to beTitself, this branch catches it correctly. But ifTOutputis a collection type that also happens to beT, the pattern could match unexpectedly. Consider moving this branch after theT[]check or adding explicit type exclusions.src/Deployment/Optimization/Quantization/Formats/FP8Quantizer.cs (1)
61-65:Modeproperty returnsFloat16but this is an FP8 quantizer.The
Modeproperty returnsQuantizationMode.Float16with a comment "Closest mode", but this could be misleading to callers expecting accurate mode reporting. Consider adding a dedicatedFP8mode to the enum or documenting this limitation more prominently.src/Deployment/Optimization/Quantization/Strategies/SmoothQuantQuantizer.cs (1)
89-97: Mutating shared configuration object may cause unexpected side effects.The constructor mutates
_config.Strategyif it doesn't matchSmoothQuant. If the caller passes a shared configuration instance, this mutation affects all users of that config. Consider creating a defensive copy or throwing an exception for mismatched strategies.♻️ Defensive copy approach
public SmoothQuantQuantizer(QuantizationConfiguration? config = null) { - _config = config ?? QuantizationConfiguration.ForSmoothQuant(); + var effectiveConfig = config ?? QuantizationConfiguration.ForSmoothQuant(); - if (_config.Strategy != QuantizationStrategy.SmoothQuant) + if (effectiveConfig.Strategy != QuantizationStrategy.SmoothQuant) { - _config.Strategy = QuantizationStrategy.SmoothQuant; + // Create a copy to avoid mutating the caller's config + effectiveConfig = effectiveConfig.Clone(); // or create new instance with modified strategy + effectiveConfig.Strategy = QuantizationStrategy.SmoothQuant; } + _config = effectiveConfig; }src/Deployment/Optimization/Quantization/Strategies/GPTQQuantizer.cs (1)
70-78: Mutating shared configuration object may cause unexpected side effects.Same concern as SmoothQuantQuantizer: the constructor mutates
_config.Strategyif it doesn't match GPTQ. If the caller passes a shared configuration instance, this mutation affects all users. Consider creating a defensive copy.
- CalibrationHelper.cs: fix inverted type assignability check - EfficientQATOptimizer.cs: use Array.Empty<T>() for net471 compatibility - SpinQuantQuantizer.cs: refactor to use generic Matrix<T>/Vector<T> types instead of hardcoded double and double[,] Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Deployment/Optimization/Quantization/Strategies/SpinQuantQuantizer.cs`:
- Around line 92-95: The code in SpinQuantQuantizer currently mutates the
incoming _config by setting _config.Strategy = QuantizationStrategy.SpinQuant;
instead validate that _config.Strategy equals QuantizationStrategy.SpinQuant and
if it does not, throw a clear ArgumentException/InvalidOperationException
instead of modifying the passed object; update the check in the
SpinQuantQuantizer constructor or initialization method (the block referencing
_config.Strategy and QuantizationStrategy.SpinQuant) to perform validation and
throw to avoid surprising side effects on the caller's config.
🧹 Nitpick comments (1)
src/Deployment/Optimization/Quantization/Strategies/SpinQuantQuantizer.cs (1)
45-45: Unused_zeroPointsdictionary wastes memory.The
_zeroPointsdictionary is allocated but never populated.GetZeroPointalways returns0for symmetric quantization. Consider removing this field.♻️ Proposed fix
private readonly Dictionary<string, T> _scaleFactors = new(); - private readonly Dictionary<string, int> _zeroPoints = new(); private readonly Dictionary<string, Matrix<T>> _rotationMatrices = new();
| { | ||
| // Find pivot | ||
| int maxRow = col; | ||
| for (int row = col + 1; row < n; row++) | ||
| { | ||
| if (NumOps.Compare(NumOps.Abs(augmented[row, col]), NumOps.Abs(augmented[maxRow, col])) > 0) | ||
| { | ||
| maxRow = row; | ||
| } | ||
| } | ||
|
|
||
| // Swap rows | ||
| for (int j = 0; j < 2 * n; j++) | ||
| { | ||
| (augmented[col, j], augmented[maxRow, j]) = (augmented[maxRow, j], augmented[col, j]); | ||
| } | ||
|
|
||
| // Scale pivot row | ||
| T pivot = augmented[col, col]; | ||
| T pivotThreshold = NumOps.FromDouble(1e-10); | ||
| if (NumOps.Compare(NumOps.Abs(pivot), pivotThreshold) < 0) | ||
| { | ||
| // Matrix is singular - add warning and return identity as fallback | ||
| _calibrationWarnings.Add($"Warning: Singular matrix detected at column {col} during Cayley transform, falling back to identity rotation."); | ||
| return Matrix<T>.CreateIdentity(n); | ||
| } | ||
|
|
||
| for (int j = 0; j < 2 * n; j++) | ||
| { | ||
| augmented[col, j] = NumOps.Divide(augmented[col, j], pivot); | ||
| } | ||
|
|
||
| // Eliminate column | ||
| for (int row = 0; row < n; row++) | ||
| { | ||
| if (row != col) | ||
| { | ||
| T factor = augmented[row, col]; | ||
| for (int j = 0; j < 2 * n; j++) | ||
| { | ||
| augmented[row, j] = NumOps.Subtract(augmented[row, j], NumOps.Multiply(factor, augmented[col, j])); | ||
| } | ||
| } | ||
| } | ||
| } |
Check notice
Code scanning / CodeQL
Block with too many statements Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
In general, this kind of issue is best fixed by decomposing the complex loop body into smaller, focused methods. Each helper method should encapsulate a logical sub-step of the algorithm (e.g., finding the pivot row, swapping rows, normalizing the pivot row, and eliminating the current column), so that the main loop becomes a high-level description of the algorithm with fewer complex statements.
For this specific InvertMatrix method in SpinQuantQuantizer.cs, the best approach is to refactor the body of the for (int col = 0; col < n; col++) loop into separate private methods:
FindPivotRow(Matrix<T> augmented, int n, int col)– encapsulates the innerforloop that findsmaxRow.SwapRows(Matrix<T> augmented, int n, int rowA, int rowB)– encapsulates the row-swappingforloop.bool NormalizePivotRow(Matrix<T> augmented, int n, int col)– encapsulates pivot extraction, singularity check, logging the warning, and scaling the pivot row. It returnsfalseif the matrix is singular so the caller can return the identity fallback.EliminateColumn(Matrix<T> augmented, int n, int col)– encapsulates the elimination double loop (overrowandj).
Then, the for (int col = 0; col < n; col++) loop can call these helpers in sequence, leaving only a small number of complex constructs in the loop body (one if and several method calls instead of nested loops and conditionals). The behavior stays the same: we still use partial pivoting, log the same warning message, and fall back to the identity matrix on singularity.
Concretely:
- Above
InvertMatrix, add the four new private helper methods using the existing fields (NumOps,_calibrationWarnings). - Inside the
for (int col = 0; col < n; col++)loop, replace the current inlined pivot-finding, swapping, singularity check, scaling, and elimination logic with calls to these helpers. - Keep the rest of
InvertMatrix(matrix augmentation and inverse extraction) unchanged.
No new imports or external libraries are necessary; everything can be expressed in terms of existing types (Matrix<T>, INumericOperations<T>, _calibrationWarnings).
…ating config SpinQuantQuantizer now throws ArgumentException if Strategy is not SpinQuant, instead of silently modifying the caller's config object. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Models/Results/AiModelResult.cs (1)
1162-1167:⚠️ Potential issue | 🟠 MajorQuantizationInfo is dropped on deserialize/clone paths.
Line 1166 assigns from options, butDeserialize,WithParameters, andDeepCopydon’t propagate it, so loaded/cloned results lose quantization metadata. Please carry it through those paths.🛠️ Proposed fix
@@ public void Deserialize(byte[] data) - InferenceOptimizationConfig = deserializedObject.InferenceOptimizationConfig; + InferenceOptimizationConfig = deserializedObject.InferenceOptimizationConfig; + QuantizationInfo = deserializedObject.QuantizationInfo; SerializedModelData = deserializedObject.SerializedModelData;@@ public IFullModel<T, TInput, TOutput> WithParameters(Vector<T> parameters) // JIT compilation is parameter-specific, don't copy InferenceOptimizationConfig = InferenceOptimizationConfig, + QuantizationInfo = QuantizationInfo, ReasoningConfig = ReasoningConfig,@@ public IFullModel<T, TInput, TOutput> DeepCopy() // JIT compilation is model-specific, don't copy InferenceOptimizationConfig = InferenceOptimizationConfig, + QuantizationInfo = QuantizationInfo, ReasoningConfig = ReasoningConfig,
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private static (double scale, int zeroPoint) ComputeQuantizationParameters( | ||
| double maxAbs, double minVal, double maxVal, int bitWidth, double minScaleFactor, bool symmetric) | ||
| { | ||
| double qMax = symmetric ? (1 << (bitWidth - 1)) - 1 : (1 << bitWidth) - 1; | ||
| double scale; | ||
| int zeroPoint; | ||
|
|
||
| if (symmetric) | ||
| { | ||
| scale = maxAbs / qMax; | ||
| zeroPoint = 0; | ||
| } | ||
| else | ||
| { | ||
| scale = (maxVal - minVal) / ((1 << bitWidth) - 1); | ||
| scale = Math.Max(scale, minScaleFactor); | ||
| zeroPoint = (int)MathHelper.Clamp(Math.Round(-minVal / scale), 0, qMax); | ||
| } | ||
|
|
||
| // Ensure minimum scale factor | ||
| scale = Math.Max(scale, minScaleFactor); | ||
| return (scale, zeroPoint); | ||
| } |
There was a problem hiding this comment.
In the ComputeQuantizationParameters method, the symmetric parameter is declared but when calculating the zero point for asymmetric quantization, the code uses MathHelper.Clamp(Math.Round(-minVal / scale), 0, qMax). However, in asymmetric mode, qMax is calculated as (1 << bitWidth) - 1, which means the valid range should be [0, qMax]. The clamping is correct, but the calculation of qMax in the symmetric branch uses (1 << (bitWidth - 1)) - 1 which is different. This inconsistency could lead to incorrect quantization ranges. The symmetric qMax calculation should match the pattern used in other quantizers for consistency.
| public static double ByteToE4M3(byte b) | ||
| { | ||
| if (b == 0) return 0; | ||
|
|
||
| bool sign = (b & 0x80) != 0; | ||
| int exp = (b >> 3) & 0x0F; | ||
| int mantissa = b & 0x07; | ||
|
|
||
| double value; | ||
| if (exp == 0) | ||
| { | ||
| // Subnormal | ||
| value = mantissa / 8.0 * Math.Pow(2, -6); | ||
| } | ||
| else if (exp == 15) | ||
| { | ||
| // E4M3 supports NaN but not Infinity per NVIDIA spec | ||
| // NaN is encoded when exp=15 and mantissa=7 (0x7F or 0xFF) | ||
| if (mantissa == 7) | ||
| { | ||
| return double.NaN; | ||
| } | ||
| // Other exp=15 patterns represent max value | ||
| value = E4M3_MAX_VALUE; | ||
| } | ||
| else | ||
| { | ||
| // Normal | ||
| value = (1.0 + mantissa / 8.0) * Math.Pow(2, exp - E4M3_EXPONENT_BIAS); | ||
| } | ||
|
|
||
| return sign ? -value : value; | ||
| } |
There was a problem hiding this comment.
The ByteToE4M3 method handles NaN by checking if mantissa == 7 when exp == 15. However, according to the NVIDIA E4M3 spec, there are multiple NaN encodings (all patterns with exp=15 and mantissa != 0 should be NaN). The current implementation only recognizes 0x7F and 0xFF as NaN, but patterns like 0x79-0x7E and 0xF9-0xFE should also be treated as NaN. This could lead to incorrect interpretation of E4M3 values.
| // NF4 codebook: 16 values used in QLoRA for N(0,1)-distributed weights. | ||
| // These are empirically optimized representative values for equal-probability bins of N(0,1), | ||
| // not exact analytical quantiles. The interval comments below show approximate quantile ranges | ||
| // for each bin. See Dettmers et al., "QLoRA: Efficient Finetuning of Quantized LLMs" (2023), | ||
| // Appendix / NF4 codebook definition, for the original table of values. |
There was a problem hiding this comment.
The comment on lines 47-50 describes the NF4 codebook as "empirically optimized representative values" but then says the interval comments show "approximate quantile ranges". The actual NF4 codebook values from the QLoRA paper are quantile centers for a standard normal distribution N(0,1), not empirically optimized values. The comment should accurately reflect that these are analytically derived quantile centers, not empirical optimizations. This is an important distinction for users who want to understand the theoretical foundation of NF4.
| // NF4 codebook: 16 values used in QLoRA for N(0,1)-distributed weights. | |
| // These are empirically optimized representative values for equal-probability bins of N(0,1), | |
| // not exact analytical quantiles. The interval comments below show approximate quantile ranges | |
| // for each bin. See Dettmers et al., "QLoRA: Efficient Finetuning of Quantized LLMs" (2023), | |
| // Appendix / NF4 codebook definition, for the original table of values. | |
| // NF4 codebook: 16 quantile-center values for N(0,1) used in QLoRA for normally distributed weights. | |
| // These are analytically derived centers of equal-probability quantization bins of a standard normal | |
| // distribution, not empirically optimized values. The interval comments below show approximate bin | |
| // boundaries (quantile ranges) for each level. See Dettmers et al., "QLoRA: Efficient Finetuning of | |
| // Quantized LLMs" (2023), Appendix / NF4 codebook definition, for the original table of values. |
| /// <summary> | ||
| /// Gets the processing order for columns based on activation importance (ActOrder optimization). | ||
| /// </summary> | ||
| private int[] GetProcessingOrder(double[] weights, int n, bool useActOrder) | ||
| { | ||
| int[] order = new int[n]; | ||
|
|
||
| if (useActOrder && _activationStats?.GlobalActivationMagnitudes != null && | ||
| _activationStats.GlobalActivationMagnitudes.Length > 0) | ||
| { | ||
| // Compute activation importance scores from calibration statistics | ||
| var importance = new (int index, double score)[n]; | ||
| var magnitudes = _activationStats.GlobalActivationMagnitudes; | ||
|
|
||
| for (int i = 0; i < n; i++) | ||
| { | ||
| double score = i < magnitudes.Length ? magnitudes[i] : 0; | ||
| importance[i] = (i, score); | ||
| } | ||
|
|
||
| // Sort by importance (descending) - process most important first | ||
| var sorted = importance.OrderByDescending(x => x.score).ToArray(); | ||
| for (int i = 0; i < n; i++) | ||
| { | ||
| order[i] = sorted[i].index; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| // Default order | ||
| for (int i = 0; i < n; i++) | ||
| { | ||
| order[i] = i; | ||
| } | ||
| } | ||
|
|
||
| return order; | ||
| } |
There was a problem hiding this comment.
In the GetProcessingOrder method, when useActOrder is true and activation statistics are available, the code sorts by importance descending and then stores the sorted indices in the order array. However, the comment says "process most important first", but the actual logic in QuantizeWithGPTQ processes columns in the order specified by the order array from i = groupStart to i < groupEnd, using idx = order[i]. This means if order[0] contains the most important index, it will be processed first, which is correct. However, the variable naming could be clearer - calling it order suggests it's a permutation, but it's actually an array where order[i] gives the parameter index to process at step i.
| // s_j = max(|X_j|)^α / max(|W_j|)^(1-α) | ||
| // This balances the quantization difficulty between X and W | ||
|
|
||
| double actMax = Math.Max(activationMax[i], 1e-6); | ||
| double wMax = Math.Max(weightMax[i], 1e-6); | ||
|
|
||
| // Use log-space computation for numerical stability when values are very small | ||
| // s = exp(alpha * log(actMax) - (1-alpha) * log(wMax)) | ||
| double logAct = Math.Log(actMax); | ||
| double logW = Math.Log(wMax); | ||
| double logS = alpha * logAct - (1.0 - alpha) * logW; | ||
| double s = Math.Exp(logS); |
There was a problem hiding this comment.
In the ComputeSmoothingScales method, the log-space computation uses logS = alpha * logAct - (1.0 - alpha) * logW to compute the smoothing scale. However, the formula comment states s_j = max(|X_j|)^α / max(|W_j|)^(1-α). Taking logs: log(s) = α*log(act) - (1-α)*log(w). The current implementation matches this, but it's actually computing s = act^α / w^(1-α), which should be log(s) = α*log(act) - (1-α)*log(w). However, the exponent on w should be (1-α), so the log should be - (1-α)*log(w) = -(log(w) - α*log(w)). The current formula is correct, but the comment could be clearer about the log-space transformation.
| /// <item><description>Any type with ToArray() method returning T[] (via reflection)</description></item> | ||
| /// </list> | ||
| /// <para>The reflection fallback is expensive and should be avoided for performance-critical code. | ||
| /// If the ToArray() method exists but returns a different type (not T[]), conversion will fail silently.</para> |
There was a problem hiding this comment.
The ConvertToTensor method uses reflection as a fallback to call ToArray() on unknown types. The comment on lines 467-468 states "This fallback is expensive and should be avoided for performance-critical code" and "If the ToArray() method exists but returns a different type (not T[]), conversion will fail silently." However, the actual code on lines 500-509 does check the return type before invoking: if (returnType == typeof(T[]) || typeof(T[]).IsAssignableFrom(returnType)). This check prevents the silent failure mentioned in the comment. The comment should be updated to reflect that the method validates the return type before invoking, and only fails silently if the method throws or returns null.
| /// If the ToArray() method exists but returns a different type (not T[]), conversion will fail silently.</para> | |
| /// The method validates that ToArray() returns a compatible T[] before invoking; conversion will only | |
| /// fail silently if ToArray() throws or returns null, in which case this method simply returns null.</para> |
| // Map global indices to local indices within the hessianDiag array | ||
| if (hessianDiag.Length == 0) return _config.GPTQDampingFactor; | ||
|
|
||
| int iLocal = i % hessianDiag.Length; | ||
| int jLocal = j % hessianDiag.Length; | ||
|
|
||
| // Both indices are guaranteed to be in range after modulo | ||
| if (iLocal >= 0 && jLocal >= 0) | ||
| { | ||
| return Math.Sqrt(hessianDiag[iLocal] * hessianDiag[jLocal]) * 0.1; | ||
| } | ||
|
|
||
| // Fallback for out-of-range indices: use damping factor | ||
| return _config.GPTQDampingFactor; |
There was a problem hiding this comment.
The GetHessianCrossElement method uses modulo operation to map global indices to local indices: int iLocal = i % hessianDiag.Length and int jLocal = j % hessianDiag.Length. This assumes that indices wrap around, which may not accurately represent the cross-element structure of the Hessian matrix. For a proper GPTQ implementation, cross-elements should come from the inverse Hessian matrix computed via Cholesky decomposition, but this simplified version uses a geometric mean approximation. The modulo mapping could cause incorrect cross-element values when processing large groups with ActOrder, potentially degrading quantization quality.
| // Map global indices to local indices within the hessianDiag array | |
| if (hessianDiag.Length == 0) return _config.GPTQDampingFactor; | |
| int iLocal = i % hessianDiag.Length; | |
| int jLocal = j % hessianDiag.Length; | |
| // Both indices are guaranteed to be in range after modulo | |
| if (iLocal >= 0 && jLocal >= 0) | |
| { | |
| return Math.Sqrt(hessianDiag[iLocal] * hessianDiag[jLocal]) * 0.1; | |
| } | |
| // Fallback for out-of-range indices: use damping factor | |
| return _config.GPTQDampingFactor; | |
| // Validate indices against the local hessianDiag array. If indices fall outside | |
| // the local group, treat them as weakly correlated and use the damping factor, | |
| // rather than wrapping via modulo which can mix unrelated parameters. | |
| if (hessianDiag.Length == 0) | |
| return _config.GPTQDampingFactor; | |
| if (i < 0 || i >= hessianDiag.Length || j < 0 || j >= hessianDiag.Length) | |
| return _config.GPTQDampingFactor; | |
| return Math.Sqrt(hessianDiag[i] * hessianDiag[j]) * 0.1; |
| // Grid search over scaling factors (configurable via AWQScaleSearchOptions) | ||
| double[] scaleOptions = config.AWQScaleSearchOptions.Length > 0 | ||
| ? config.AWQScaleSearchOptions | ||
| : [0.0, 0.1, 0.2, 0.3, 0.5, 0.7, 1.0, 1.5, 2.0]; |
There was a problem hiding this comment.
In the FindOptimalScale method, the grid search uses the configured AWQScaleSearchOptions with a default of [0.0, 0.1, 0.2, 0.3, 0.5, 0.7, 1.0, 1.5, 2.0]. However, scale values above 1.0 would actually amplify weights rather than protect them. The AWQ paper typically uses scale values in the range [0.0, 1.0] for the protection factor. Values like 1.5 and 2.0 seem incorrect - they would make protected weights MORE susceptible to quantization error, not less. This could lead to suboptimal quantization results.
| : [0.0, 0.1, 0.2, 0.3, 0.5, 0.7, 1.0, 1.5, 2.0]; | |
| : [0.0, 0.1, 0.2, 0.3, 0.5, 0.7, 1.0]; |
| /// Computes the zero-point for asymmetric quantization. | ||
| /// </summary> | ||
| /// <param name="min">The minimum value in the range.</param> | ||
| /// <param name="scale">The quantization scale factor.</param> | ||
| /// <param name="qMax">The maximum quantized value.</param> | ||
| /// <returns>The clamped zero-point value.</returns> | ||
| private static int ComputeAsymmetricZeroPoint(double min, double scale, double qMax) | ||
| { | ||
| return (int)MathHelper.Clamp(Math.Round(-min / scale), 0, qMax); | ||
| } |
There was a problem hiding this comment.
The ComputeAsymmetricZeroPoint method is defined as a static local function, but it's only called twice within the same file. While this is not incorrect, the method could be made an instance method or a private static method of the class for better discoverability and reusability. The current placement as a static local function at the top of the class (line 75-83) is unusual - static local functions are typically defined closer to their usage point. Consider moving this to be a private static method of the class.
| model.SetTrainingMode(false); | ||
| try | ||
| { | ||
| foreach (var inputTensor in samples) | ||
| { | ||
| totalSamples++; | ||
| try | ||
| { | ||
| // Run forward pass with memory to capture intermediate activations | ||
| var output = model.ForwardWithMemory(inputTensor); | ||
|
|
||
| // Store output activation stats | ||
| if (!stats.LayerStats.TryGetValue("output", out var outputStats)) | ||
| { | ||
| outputStats = new LayerActivationStats<T> { LayerName = "output" }; | ||
| stats.LayerStats["output"] = outputStats; | ||
| } | ||
| outputStats.Update(output); | ||
|
|
||
| // Store input activation stats | ||
| if (!stats.LayerStats.TryGetValue("input", out var inputStats)) | ||
| { | ||
| inputStats = new LayerActivationStats<T> { LayerName = "input" }; | ||
| stats.LayerStats["input"] = inputStats; | ||
| } | ||
| inputStats.Update(inputTensor); | ||
|
|
||
| stats.SampleCount++; | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| failedSamples++; | ||
| // Log first few failure reasons for debugging | ||
| if (failedSamples <= 3) | ||
| { | ||
| stats.CalibrationWarnings.Add($"Forward pass sample {totalSamples} failed: {ex.Message}"); | ||
| } | ||
| continue; | ||
| } | ||
| } | ||
| } | ||
| finally | ||
| { | ||
| // Restore training mode | ||
| model.SetTrainingMode(true); | ||
| } |
There was a problem hiding this comment.
The CollectTensorBasedActivations method calls model.SetTrainingMode(false) to set inference mode for calibration, then uses a try-finally block to restore training mode with model.SetTrainingMode(true) in the finally block (lines 188-233). However, this assumes the model was in training mode before calibration started. If the model was already in inference mode, this would incorrectly switch it to training mode after calibration. The code should save the current training mode state before changing it and restore the original state in the finally block.
| { | ||
| calibrationData = xTrainArray.Take(Math.Min(calibrationSampleCount, xTrainArray.Length)); | ||
| } | ||
| else if (XTrain is IEnumerable<TInput> xTrainEnumerable) |
Check warning
Code scanning / CodeQL
Constant condition Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
In general, to fix this type of issue you should avoid using null-conditional operators or other null checks inside a scope that is already guarded by a non-null check on the same variable. Doing so keeps the control flow clear and prevents static analyzers from reporting constant-condition issues.
Concretely here, the if statement on line 2323:
if (_quantizationConfig != null && _quantizationConfig.Mode != QuantizationMode.None && optimizationResult.BestSolution != null)guarantees _quantizationConfig is non-null in the body. Therefore, on line 2329 we can safely replace:
int calibrationSampleCount = _quantizationConfig?.CalibrationSamples ?? 100;with a direct access:
int calibrationSampleCount = _quantizationConfig.CalibrationSamples ?? 100;This preserves existing functionality: if CalibrationSamples is a nullable int?, the null-coalescing ?? 100 still applies; if it is a non-nullable int, the ?? 100 is redundant but harmless (and you may optionally keep it to avoid changing behavior). No new methods or imports are required; the change is entirely local to the quantization block in src/AiModelBuilder.cs.
| @@ -2326,7 +2326,7 @@ | ||
| { | ||
| // Use preprocessed training data as calibration data for consistent quantization | ||
| // This ensures calibration sees the same data distribution as during training | ||
| int calibrationSampleCount = _quantizationConfig?.CalibrationSamples ?? 100; | ||
| int calibrationSampleCount = _quantizationConfig.CalibrationSamples ?? 100; | ||
| IEnumerable<TInput>? calibrationData = null; | ||
| if (XTrain is TInput[] xTrainArray) | ||
| { |
| { | ||
| calibrationData = xTrainArray.Take(Math.Min(calibrationSampleCount, xTrainArray.Length)); | ||
| } | ||
| else if (XTrain is IEnumerable<TInput> xTrainEnumerable) |
Check warning
Code scanning / CodeQL
Constant condition Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
In general, to fix a constant condition arising from a redundant null‑conditional access after a prior null check, you remove the unnecessary null‑conditional operator and treat the value as non‑null within the guarded block. This both simplifies the logic and aligns with what the earlier null check already guarantees.
Concretely, in src/AiModelBuilder.cs, at line 2323, there is an if block guarded by _quantizationConfig != null. Inside that block, line 2329 uses _quantizationConfig?.CalibrationSamples ?? 100;. Because _quantizationConfig cannot be null within this block, the ?. is unnecessary and leads to a constant condition warning. Replace _quantizationConfig?.CalibrationSamples with _quantizationConfig.CalibrationSamples. No new methods or imports are needed; this is a local expression change that does not alter functionality, because the ?? 100 already handles the case where CalibrationSamples itself may be nullable or has a default value.
| @@ -2326,7 +2326,7 @@ | ||
| { | ||
| // Use preprocessed training data as calibration data for consistent quantization | ||
| // This ensures calibration sees the same data distribution as during training | ||
| int calibrationSampleCount = _quantizationConfig?.CalibrationSamples ?? 100; | ||
| int calibrationSampleCount = _quantizationConfig.CalibrationSamples ?? 100; | ||
| IEnumerable<TInput>? calibrationData = null; | ||
| if (XTrain is TInput[] xTrainArray) | ||
| { |
|
…0.117.0) Bumps the AiDotNet.Tensors pin 0.116.0 -> 0.117.0, which ships the optional attention-logit soft-cap on ScaledDotProductAttention (CPU + all six GPU backends, Tensors #816), and wires the Gemma-2 attn_logit_softcapping path through the facade import: - GroupedQueryAttentionLayer takes an optional attnLogitSoftcap and forwards it into the fused Engine.ScaledDotProductAttention(..., softcap); exposed via the AttnLogitSoftcap property. The (never-faithful) softcap + ALiBi combination throws rather than silently dropping the cap. - HuggingFaceConfig parses attn_logit_softcapping (alongside the existing final_logit_softcapping). - Gemma2ModelBuilder threads config.AttnLogitSoftcapping into every decoder block's attention. Also fixes a pre-existing net471 compile break in the pretrained/GGUF test files: TensorShape does not convert to IEnumerable<int> on net471, so Assert.Equal(int[], x.Shape) failed to compile there (these files only ever ran on net10.0). Comparing x.Shape.ToArray() builds on both TFMs. Test: Builder_Gemma2_AttnLogitSoftcap_ParsesAndAltersAttention asserts the config parses, the builder threads the cap into the layer (structural), and a bite-sized cap actually changes the forward output (functional). Pretrained + GGUF suites 36/36 green; both TFMs build clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…0.117.0) Bumps the AiDotNet.Tensors pin 0.116.0 -> 0.117.0, which ships the optional attention-logit soft-cap on ScaledDotProductAttention (CPU + all six GPU backends, Tensors #816), and wires the Gemma-2 attn_logit_softcapping path through the facade import: - GroupedQueryAttentionLayer takes an optional attnLogitSoftcap and forwards it into the fused Engine.ScaledDotProductAttention(..., softcap); exposed via the AttnLogitSoftcap property. The (never-faithful) softcap + ALiBi combination throws rather than silently dropping the cap. - HuggingFaceConfig parses attn_logit_softcapping (alongside the existing final_logit_softcapping). - Gemma2ModelBuilder threads config.AttnLogitSoftcapping into every decoder block's attention. Also fixes a pre-existing net471 compile break in the pretrained/GGUF test files: TensorShape does not convert to IEnumerable<int> on net471, so Assert.Equal(int[], x.Shape) failed to compile there (these files only ever ran on net10.0). Comparing x.Shape.ToArray() builds on both TFMs. Test: Builder_Gemma2_AttnLogitSoftcap_ParsesAndAltersAttention asserts the config parses, the builder threads the cap into the layer (structural), and a bite-sized cap actually changes the forward output (functional). Pretrained + GGUF suites 36/36 green; both TFMs build clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary
Implements a complete model quantization framework supporting both Post-Training Quantization (PTQ) and Quantization-Aware Training (QAT), addressing issue #278.
PTQ Strategies
Numeric Formats
QAT Support
QATTrainingHook: Applies fake quantization during training with Straight-Through EstimatorEfficientQATOptimizer: Block-wise quantization optimizationCalibration Infrastructure
CalibrationHelper: Collects activation statistics via real forward passes when possibleINeuralNetworkModel,INeuralNetwork, and genericIFullModelIntegration
AiModelBuildertraining pipelineQuantizationInfoincluded inAiModelResultTest plan
Closes #278
🤖 Generated with Claude Code