Skip to content

Commit 9dfa5f2

Browse files
angularsenclaude
andcommitted
Update PR 1544 review with empirical findings from both options
Option A (implicit cast): 1 line library change, 101 mechanical test fixes, 0 new test failures across ~51,900 tests. Verified working. Option B (double public): 608 files changed, 0 library build errors, but 2,509 test failures (5%) due to precision loss from double storage. Roundtrip conversions that were lossless with QuantityValue now lose precision. This defeats a core value proposition of the PR. Recommendation: Option A is clearly superior — simpler, preserves full precision, and empirically verified. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 2f960fe commit 9dfa5f2

File tree

1 file changed

+20
-9
lines changed

1 file changed

+20
-9
lines changed

review_pr1544.md

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -147,18 +147,29 @@ The precision benefit is preserved: conversion factors like `1250/381` (feet-to-
147147
- **Precision only in conversion pipeline** — consumer arithmetic (`length1 + length2`) uses `double`, not rationals
148148
- **~10% of PR value lost** — the `QuantityValue`-as-public-type feature and its test coverage
149149

150-
### Implementation scope (assessed, not implemented)
150+
### Verified empirically
151+
152+
Option B was fully implemented on branch `option-b-double-public` (608 files changed).
153+
154+
**Changes made:**
155+
- ~10 manual source files: `IQuantity.cs`, `IArithmeticQuantity.cs`, `ILogarithmicQuantity.cs`, `QuantityExtensions.cs`, `QuantityParser.cs`, `Quantity.cs`, `UnitConverter.cs`, `Comparison.cs`, `QuantityInfo.cs`, plus 10 custom quantity extra files
156+
- CodeGen templates: `QuantityGenerator.cs` (~15 locations), `UnitTestBaseClassGenerator.cs`, `NumberExtensionsGenerator.cs`, `NumberExtensionsCS14Generator.cs`, `QuantityRelationsParser.cs`
157+
- `UnitRelations.json` type reference
158+
- All 131 generated quantity files + test base classes regenerated
159+
- Serialization adapters updated
160+
- Benchmark and test code updated (decimal literals → double, QuantityValue references → double)
161+
162+
**Build: 0 library errors, 0 warnings.** All 4 library projects compile cleanly.
151163

152-
Option B was scoped but not implemented due to the scale of changes required:
164+
**Tests: 2,509 failures out of 51,899.** The failures are NOT test bugs — they reveal a fundamental trade-off:
165+
- **1,215 `ToUnit_FromNonBaseUnit` failures**: Converting a quantity to another unit and back loses precision because `double` storage introduces floating-point rounding at each step. With `QuantityValue`, this roundtrip was lossless.
166+
- **491 `ToUnit_FromIQuantity` failures**: Same root cause.
167+
- **256 `ToString` failures**: Precision-sensitive formatting.
168+
- **68 `ConversionRoundTrip` failures**: Direct roundtrip precision tests.
153169

154-
- **~10 manual source files**: `IQuantity.cs`, `IArithmeticQuantity.cs`, `ILogarithmicQuantity.cs`, `QuantityExtensions.cs`, `QuantityParser.cs`, `Quantity.cs`, `UnitConverter.cs` (public signatures), `Comparison.cs`, plus custom quantity extras
155-
- **CodeGen templates**: ~15 locations in `QuantityGenerator.cs`, plus `NumberExtensionsGenerator.cs`, `NumberExtensionsCS14Generator.cs`, `QuantityRelationsParser.cs`
156-
- **Data files**: `UnitRelations.json` type reference
157-
- **Regeneration**: All 131 quantity files + number extensions + test base classes
158-
- **Serialization layer rework**: Serializers currently read/write `QuantityValue` from `IQuantity.Value` — changing to `double` fundamentally changes the wire format
159-
- **Estimated effort**: Multi-day, with significant risk of cascading issues in serialization
170+
**Key finding**: Option B fundamentally degrades conversion precision. The exact rational conversion pipeline (`QuantityValue × rational_coefficient`) helps, but the `double` input/output introduces rounding that compounds across multi-step conversions. This defeats a core value proposition of the PR.
160171

161-
Given that Option A achieves the same consumer-facing goal with 1 line of library code changed, Option B is not recommended unless Option A proves unworkable in practice.
172+
Given that Option A achieves the same consumer-facing goal with 1 line of library code, 0 new test failures, and preserves full precision, **Option B is not recommended.**
162173

163174
---
164175

0 commit comments

Comments
 (0)