@angularsen Clearly, I don't expect this to get merged in the Gitty up! fashion, but at least we have the whole picture, with sources that I can reference.

If you want, send me an e-mail, we could do a quick walk-through / discussion.

100k lines removed, 100k lines added 🙈

image

100k lines removed, 100k lines added 🙈

I tried to create this PR twice before (many months ago), while the changes to the unit definitions were still not merged- and the web interface was giving me an error when trying to browse the files changed.. Something like "Too many files to display" 😄

Ok, I'm not going to get through a review of this many files anytime soon.
Maybe we should have a screen sharing session and go through it together. I may have some time this weekend, what about you? What timezone are you in?

On the surface though, it seems like this could be split up into chunks. I know it's tedious and extra work, but it will be way faster to review. Do you see any chunks of changes to easily split off into separate PRs?

Ok, I'm not going to get through a review of this many files anytime soon. Maybe we should have a screen sharing session and go through it together. I may have some time this weekend, what about you? What timezone are you in?

Sofia (GMT+3), but time zones are not relevant to my sleep schedule - so basically any time you want.

On the surface though, it seems like this could be split up into chunks. I know it's tedious and extra work, but it will be way faster to review. Do you see any chunks of changes to easily split off into separate PRs?

Yes, I do have some ideas:

1. UnitAbbreviationsCache and the UnitParser should be more or less free of changes once UnitAbbreviationsCache.CreateEmpty should use the default QuantityInfoLookup #1548 is merged
2. I plan to remove the IConvertible interface tonight (lots of red points there)
3. The QuantityParser has just a few minor changes which I was going to try to push as well (other than that it's mostly just double changing to QuantityValue)
4. QuantityFormatter - there was an issue that I created earlier that should (mostly) solve the differences
5. Refactoring the QuantityInfo can theoretically be done without the ConversionExpressions (which would open the way for the changes to the IQuantity interface and some of the extension methods).
6. UnitParser: introduce two new method: GetUnitFromAbbreviation and TryGetUnitFromAbbreviation returning a UnitInfo (which could be used for constructing an instance of the quantity)
7. Introduce the changes to the IQuantity interface and some of the extension methods
8. Move the exceptions in their own folder and replace the usages of the NotImplementedException with the appropriate UnitNotFoundException
9. Update the UnitTestBaseClassGenerator - I've refactored the Parse/TryParse tests (completing the test coverage) - having a look at the diff on the MassTestsBase.g.cs, it looks like these account for about half of all diffs in the PR 😄
10. Make the QuantityInfoLookup public: apart from the extra constructors, there doesn't appear to be any other differences- and I don't see any reason to keep it internal
11. Introduce the CodeGen/Helpers/ExpressionEvaluationHelpers.cs + CodeGen/Helpers/ExpressionAnalyzer and replace the unit conversion expressions such as (_value / 1e3) * 1e-2d) with the simplified expression (unless we actually plan to use the the rest of this PR- this would probably be an overkill).
12. The JsonQuantityConverter stuff from SystemText could theoretically come with it's double versions first (but we do need to have a discussion about it)

Hopefully by the time we get to 5) you'd be up to speed (and fed up with PRs) and we can turn back to reviewing / working on the rest of it as a whole 😄

Ok, sounds good. Just send PRs my way and I'll try to get to them. I have a little bit of extra time this weekend.

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days.

This PR was automatically closed due to inactivity.

@angularsen I've synced the changes from upstream, however instead of bringing down the diffs, this has added another ~200 file changes (the new NumberExtensions).

I've updated the task list (completing the IQuantity interface refactoring). The next task on the list is

Introduce the CodeGen/Helpers/ExpressionEvaluationHelpers.cs + CodeGen/Helpers/ExpressionAnalyzer and replace the unit conversion expressions such as (_value / 1e3) * 1e-2d) with the simplified expression (unless we actually plan to use the the rest of this PR- this would probably be an overkill).

... but that can only start after I've had a confirmation, that we're moving towards replacing the double (in v6).

@lipchev I really like the concept of the decimal fractions, it solves equality issues and precision, and if I recall correctly it remains fairly backwards compatible.

I want to say let's go, but if you could please help me out and give a short summary for a couple of things that would help a lot as I don't recall and I'm pressed for time.

• How does it affect performance? CPU time on creating a quantity, converting units and ToString are perhaps the big ones. Allocations are also relevant. I don't need full benchmarks, just a top level understanding of are we talking 2x, 10x, 100x orders of magnitude.
• Any significant breaking changes to be aware of?

* How does it affect performance? CPU time on creating a quantity, converting units and ToString are perhaps the big ones. Allocations are also relevant. I don't need full benchmarks, just a top level understanding of are we talking 2x, 10x, 100x orders of magnitude.

I spent a full Sunday back in February creating a comparison chart, but before I could finish up setting up all the sheets, I accidentally executed the build script which erased all of my benchmark results. 😠 I'll attach the file as it is, but the gist is that compared to v5 there should be a significant performance improvement, especially when it comes to the strings.
UnitsNet-Benchmarks.xlsx

The conversions are about the same, or faster (with units other than the BaseUnit). There are no allocations, as long as the Value is represented by a fraction, who's terms are smaller than int.MaxValue.

For example the value 42.24 is represented as 4224 / 100, which when multiplied by 1000 (some Kilo conversion) would become 4224000 / 100 which is still small enough. Note that I intentionally avoid the (possible) reduction, since keeping the original denominator generally makes the additions / subtractions more efficient: for example adding 1.23 to would result in 4224123 / 100 (simply adding the integers in the numerators). So, while dividing 4224000 / 100 by 1000 would revert back to 4224 / 100, doing a bunch of multiplications (without manually calling QuantityValue.Reduce) would sooner or later hit the magical threshold (int.MaxValue) which would trigger the allocation of the uint[]? _bits. From there on, the performance scales linearly with the size of the bits increasing by 1 for every power of int.MaxValue (my PC was still able to do square roots, even after something like 500K digits of precision). Anyway, I've got a ton of benchmarks of the implementation over at the Fractions repo (with loads of pretty charts) - when we factor in the occasional allocations, the raw arithmetic-performance was within the same order of magnitude as the decimal (it was 2x-6x slower back on Jul 1, 2024, and I expect the QuantityValue to be slightly faster).

The initialization is slower but no more than 10x, judging from these benchmarks but that can be reduced by loading a select subset of quantities (I imagine loading just 10 QuantityInfos would still at least 2x faster).

PS The Inverse method is also something like 2x slower (but that's due to the extra logic)...

PS2 Aside for the static benchmarks, I've also observed a visible improvement (in the performance logs) after migrating my code-base, but that's probably due to the improvements to the string handling stuff around the UnitParser, UnitAbbreviations etc. (from v5 to v6).

* Any significant breaking changes to be aware of?

Compared to the v5 the only significant change is that

• double value = Mass.Zero.Value is not allowed
• decimal value = Power.Zero.Value is not allowed
  both of these return a QuantityValue which does not support the implicit conversion to double or decimal:
• double value = (double)Mass.Zero.Value is allowed
• decimal value = Power.Zero.Value.ToDecimal() is also allowed

While inconvenient, this is the correct way to do it (implicit conversions from, and explicit conversion to any primitive number).

By the way, given that in v5 IQuantity.Value is still defined as QuantityValue (without an explicit cast as far as I remember), this shouldn't be that much of surprise 🤷

Thank you. I'll post some thoughts before reviewing the actual PR code changes.

Excel sheet

Nice overview, here is my read of it:

• Perf improvements for all but ToUnit
• The biggest absolute improvements:
  • Parse/TryParse is ~60% faster (49000 to 18000 ns)
  • ToString is ~75% faster (700 to 150 ns)
  • SI/UnitSystem overloads are 45% faster (200 to 120 ns)
• ToUnit is 60% slower, but in absolute terms negligible (+15 ns)

Normalizing

Regarding not normalizing and running into allocations when crossing int.MaxValue, this is useful to know (new to me) and something worth documenting in v6 release. We'll need to explain the pros/cons of decimal fractions anyway.
There are maybe ways to optimize when to normalize and not? Either way, manual control of normalize is nice for those who care about it.

Initialization

Can't we just lazy initialize the quantities actually used? Not sure how much work that would be.

Breaking changes

Good overview. What are the disadvantages of supporting implicit cast to double and decimal though? It seems really convenient to me.

I saw quite a bit of commented code here and there, so I asked Claude to identify all the commented out parts. Something to consider going over.

PR #1544 - Commented Code Review

PR Title: QuantityValue implemented as a fractional number 🐲
Author: @lipchev
Status: Open

Summary

This review identifies commented-out code in PR #1544 that should either be removed or have TODO comments explaining why it's commented out and what the plan is for uncommenting later.

🔴 High Priority - Remove Immediately

1. QuantitiesSelector.cs - Duplicate Commented Class (112 lines!)

File: UnitsNet/CustomCode/QuantityInfo/Builder/QuantitiesSelector.cs
Lines: 133-244
Issue: Entire duplicate class definition (~112 lines) commented out with no explanation

// /// <summary>
// /// Provides functionality to select and configure quantities for use within the UnitsNet library.
// /// </summary>
// ... [full duplicate class implementation - 112 lines]
// }

Recommendation: DELETE - This appears to be an old version of the same class that should have been removed. Git history preserves this for reference if needed.

2. ExpressionEvaluator.cs - "No Longer Necessary" Code

File: CodeGen/Helpers/ExpressionAnalyzer/ExpressionEvaluator.cs
Lines: 278-283

// these are no longer necessary
// var expressionEvaluator = new ExpressionEvaluator(parameter, constantExpressions,
// new SqrtFunctionEvaluator(),
// new PowFunctionEvaluator(),
// new SinFunctionEvaluator(),
// new AsinFunctionEvaluator());
var expressionEvaluator = new ExpressionEvaluator(parameter, constantExpressions);

Recommendation: DELETE - Explicitly marked as "no longer necessary". Git history preserves the old constructor call pattern.

🟡 Medium Priority - Remove or Add TODO

3. QuantitiesSelector.cs - Unused Property

File: UnitsNet/CustomCode/QuantityInfo/Builder/QuantitiesSelector.cs
Line: 30

// internal Lazy<IEnumerable<QuantityInfo>> QuantitiesSelected { get; }

Recommendation: DELETE - No explanation provided; appears to be leftover from refactoring.

4. QuantitiesSelector.cs - ToList() Alternative

File: UnitsNet/CustomCode/QuantityInfo/Builder/QuantitiesSelector.cs
Line: 98

return enumeration;
// return enumeration.ToList();

Recommendation: Either:

• DELETE if ToList() is definitely not needed, OR
• ADD TODO: // TODO: Consider materializing with ToList() if multiple enumerations cause performance issues

5. DynamicQuantityConverter.cs - Old Implementation (2 instances)

File: UnitsNet/CustomCode/QuantityConverters/DynamicQuantityConverter.cs
Line 58:

// TryGetUnitInfo(conversionKey.FromUnitKey with { UnitValue = conversionKey.ToUnitValue }, out UnitInfo? toUnitInfo))

Line 347:

// return targetQuantityInfo.Create(conversionFunction.Convert(value), conversionFunction.TargetUnit);
return targetQuantityInfo.From(conversionFunction.Convert(value), conversionFunction.TargetUnit.ToUnit<TTargetUnit>());

Recommendation: Either:

• DELETE if old implementations are not needed, OR
• ADD TODO explaining why preserved (e.g., performance comparison, alternative approach)

6. FrozenQuantityConverter.cs - Old Implementation

File: UnitsNet/CustomCode/QuantityConverters/FrozenQuantityConverter.cs
Line: 67

// TryGetUnitInfo(conversionKey.FromUnitKey with { UnitValue = conversionKey.ToUnitValue }, out UnitInfo? toUnitInfo))

Recommendation: Same as DynamicQuantityConverter - either DELETE or add TODO with explanation.

7. FractionExtensions.cs - Old Double-Based Approach

File: CodeGen/Helpers/ExpressionAnalyzer/Functions/Math/FractionExtensions.cs
Line: 35-36

// return FromDoubleRounded(System.Math.Pow(x.ToDouble(), power.ToDouble()));
return PowRational(x, power);

Recommendation: Either:

• DELETE if PowRational is proven stable, OR
• ADD TODO: // TODO: Old double-based approach for reference - remove once PowRational is proven stable in production

✅ Keep (Already Have Adequate Explanation)

ConversionExpression.cs - Inline Documentation

File: UnitsNet/CustomCode/QuantityInfo/Units/ConversionExpression.cs
Lines: 264, 268, 276, 282, 293, 297, 305, 311, 322, 328, 336, 342

These comments show simplified lambda forms that enhance readability. Keep as-is.

// scaleFunction = value => value;
// scaleFunction = value => value * coefficient;

UnitsNetSetup.cs - Future Work TODOs

File: UnitsNet/CustomCode/UnitsNetSetup.cs
Lines: 35-38

// TODO see about allowing eager loading
// private AbbreviationsCachingMode AbbreviationsCaching { get; set; } = AbbreviationsCachingMode.Lazy;
// TODO see about caching the regex associated with the UnitParser
// private UnitsCachingMode UnitParserCaching { get; set; } = UnitsCachingMode.Lazy;

Status: ✅ Properly documented future work. Keep as-is.

UnitTestBaseClassGenerator.cs - Documented Limitation

File: CodeGen/Generators/UnitsNetGen/UnitTestBaseClassGenerator.cs
Lines: 1138-1139

// note: it's currently not possible to test this due to the rounding error from (quantity - otherQuantity)
// Assert.True(quantity.Equals(otherQuantity, maxTolerance));

Status: ✅ Well-documented limitation. Keep as-is.

MainWindowVM.cs - Sample Code Alternative Approach

File: Samples/UnitConverter.Wpf/UnitConverter.Wpf/MainWindowVM.cs
Lines: 135-136

// note: starting from v6 it is possible to store (and invoke here) a conversion expression
// ConvertValueDelegate _convertValueToUnit = UnitsNet.UnitConverter.Default.GetConversionFunction(...);
// ToValue = _convertValueToUnit(FromValue);

Status: ✅ Sample/demo code showing users an alternative v6 feature. Keep as-is.

ExpressionEvaluationHelpers.cs - Implicit Constructor Note

File: CodeGen/Helpers/ExpressionEvaluationHelpers.cs
Line: 354

// return $"new ConversionExpression({coefficientTermFormat})";
return coefficientTermFormat; // using the implicit constructor from QuantityValue

Status: ✅ Has explanation, though could be slightly improved:

Optional improvement:

// Old explicit approach: return $"new ConversionExpression({coefficientTermFormat})";
return coefficientTermFormat; // using the implicit constructor from QuantityValue

Summary Statistics

• 🔴 High Priority Deletions: 2 instances (~118 lines)
• 🟡 Medium Priority (Remove or TODO): 7 instances
• ✅ Keep (Adequate documentation): 6 categories of comments

Recommended Actions

1. Delete the 112-line duplicate class in QuantitiesSelector.cs (lines 133-244)
2. Delete "no longer necessary" code in ExpressionEvaluator.cs (lines 278-283)
3. Review the 7 medium-priority items and either:
   • Delete if no longer needed, or
   • Add clear TODO comments explaining preservation reason

This will remove ~125+ lines of dead code and improve code maintainability.