Skip to content

QuantityValue implemented as a fractional number 🐲 #1544

Open
lipchev wants to merge 80 commits intoangularsen:masterfrom
lipchev:fractional-quantity-value
Open

QuantityValue implemented as a fractional number 🐲 #1544
lipchev wants to merge 80 commits intoangularsen:masterfrom
lipchev:fractional-quantity-value

Conversation

@lipchev
Copy link
Collaborator

@lipchev lipchev commented Apr 13, 2025

  • QuantityValue implemented as a fractional number
  • IQuantity interfaces optimized (some methods refactored as extensions)
  • UnitInfo: introduced two new properties: ConversionFromBase and ConversionToBase which are used instead of the switch(Unit) conversion
  • UnitsNetSetup: introduced helper methods for adding external quantities, or re-configuring one or more of the existing ones
  • UntAbbreviationsCache: introduced additional factory methods (using a configuration delegate)
  • UnitParser: introduced additional factory methods (using a configuration delegate)
  • UnitConverter: re-implemented (multiple versions)
  • Inverse relationship mapping implemented as a type of implicit conversion
  • updated the JsonNet converters
  • introducing the SystemTextJson project
  • added a new UnitsNetConfiguration project to the Samples, showcasing the new configuration options
  • many more tests and benchmarks (perhaps too many)

- IQuantity interfaces optimized (some methods refactored as extensions)
- QuantityInfo/UnitInfo hierachy re-implemented (same properties, different constructors)
- QuantityInfoLookup is now public
- UntAbbreviationsCache, UnitParser, QuantityParser optimized
- UnitConverter: re-implemented (multiple versions)
- removed the IConvertible interface
- updated the JsonNet converters
- introducing the SystemTextJson project
- added a new UnitsNetConfiguration to the Samples project showcasing the new configuration options
- many more tests and benchmarks (perhaps too many)
@lipchev
Copy link
Collaborator Author

lipchev commented Apr 13, 2025

@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.

lipchev added 2 commits April 18, 2025 00:27
…lection constructors with IEnumerable

- `UnitAbbreviationsCacheInitializationBenchmarks`: replaced some obsolete usages
@angularsen
Copy link
Owner

100k lines removed, 100k lines added 🙈

image

@lipchev
Copy link
Collaborator Author

lipchev commented Apr 18, 2025

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" 😄

@angularsen
Copy link
Owner

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?

@lipchev
Copy link
Collaborator Author

lipchev commented Apr 18, 2025

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 😄

@angularsen
Copy link
Owner

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.

@github-actions
Copy link
Contributor

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.

@github-actions
Copy link
Contributor

This PR was automatically closed due to inactivity.

@github-actions github-actions bot closed this Jun 28, 2025
- updating all packages to their latest versions
- UnitsNet version bumped to 6.0.0-pre100
…Sharper debugger failure

Move the configuration lookup out of the QuantityDebugProxy constructor into the Configuration property getter in UnitsNet/CustomCode/Debug/QuantityDebugProxy.cs so the ReSharper inline evaluator no longer shows "Cannot evaluate expression" when expanding the proxy in the debugger. Also replace debug-formatting static fields with properties (DefaultFormatSpecifier, DefaultFormatProvider) and tidy namespaces/XML docs. Mitigates ReSharper issues RSRP-499956 and RSRP-502262.
…constructor logic and improving the xml comments regarding the equality contract
@lipchev
Copy link
Collaborator Author

lipchev commented Nov 16, 2025

@angularsen I expected the build would fail, but regardless went on and pushed the target framework to net10.0. It all works fine (on my machine 😄 ), with the exception of the NanoFramework build which I wasn't able to test (Microsoft boasts that every extension would be compatible, but who knows 🤷 ).

I ran a couple of the benchmarks - not everything is faster, but there is a good amount of improvement around the Parse/Format stuff. Anyway, I've already updated the package references in my own solutions (multi targeting net472 and net10.0 😄 ) and ran a few tests - everything looks good.

PS I'm still experiencing issues with the resharper extension not being able to properly evaluate some expressions in the debugger (no issues with the native VS tooltips)- I've got 2 issues open on their tracker, please check that the debug-proxies are all still working fine for you in raider.

<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFramework>net9.0</TargetFramework>
<TargetFramework>net10.0</TargetFramework>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might potentially want to keep the generator at net9.0 for a while longer, until VS2026 becomes more widely adopted (I haven't actually tried it but I think VS2022 should still be able to build this with Use previews of the .NET SDK option enabled).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do net10.0 as a separate PR, but we'll definitely bump

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was kinda hoping you would bump the build images before, or at the same time as the PR is created (targeting the main branch).

@angularsen
Copy link
Owner

Just to give a life sign, I'm still very busy with work and I expect this to continue well into the next year. However, I'm motivated to get this PR merged in the upcoming Christmas season, bump v6 to beta and start stabilizing and collecting feedback on the rather big changes for a little while before we fully release it.

I see some of my comments haven't been addressed yet, so I assume you are still working on this?
Let me know if you are waiting for me on anything. I'll try to help nudge this PR along in the next weeks, I believe it's close to ready to merge.

I realize I'm becoming a bottleneck in this project, we should discuss options to improve that. Maybe onboard more contributors, and I think you should have more autonomy to make decisions without having to wait on me. I'm open to ideas here.

- added AoT compatibility to the NumberExtensions.CS14 project
…ineQuantity`, `ClassOfLinearQuantity`, and `ClassOfLogarithmicQuantity`, implementing the respective interface defition (as a class).

- Completed the coverage of the `LinearQuantityExtensions`, `LogarithmicQuantityExtensions`, `AffineQuantityExtensions` (calling the concrete `Equals` overload with a null reference type)
- Completed the coverage of the `UnitMath` extensions (`Min`, `Max`)
- Cleaned up the legacy code in `IQuantityTests.cs`.
…PerKilogram * BrakeSpecificFuelConsumption.KilogramPerJoule expression (`double` - > `QuantityValue`)

- CodeGen: replaced all mentions of the `double` type
@github-actions
Copy link
Contributor

github-actions bot commented Feb 6, 2026

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.

@github-actions
Copy link
Contributor

This PR was automatically closed due to inactivity.

@angularsen
Copy link
Owner

Just a heads up that I have taken small steps to resume review of this PR, I have a local review with the help of AI that I'll be iterating on a little before I post it here.

@angularsen
Copy link
Owner

Alright, it's a bit rough and straight from the AI still but here are some new inputs on hopefully the latest changes. I haven't double checked if maybe some of these are already addressed.

The big one is the implicit conversion, I really do think we need to reduce the blast radius of this breaking change and not require everyone to explicitly cast to double, or to deal with QuantityValue in their codebase.

PR #1544 Review: QuantityValue as Fractional Number

Review date: 2026-03-09 | Branch: fractional-quantity-value


Changes Required Before Merge

P0 — Must do

  1. Add implicit conversion QuantityValuedouble
    Most consumers just want a double. Without this, ~60-70% of consumer code breaks on upgrade. With implicit conversion, that drops to ~20-30%. Those who want exact precision can still use QuantityValue directly.

  2. Revert EmitDefaultValue = false on DataMember
    Remove the EmitDefaultValue = false parameter from [DataMember] on _value and _unit fields in generated quantities. Length.Zero should always serialize with both Value and Unit present.

P1 — Should do

  1. Document DataContract serialization as a breaking change
    State in v6 release notes that DataContract serialization format changed (XML and JSON). XML surrogate exists and works. DataContractJsonSerializer surrogate is blocked by a .NET runtime bug — recommend migrating to JsonNet or System.Text.Json packages.

Deferred (Post-Merge)

Before stable release

  1. Create migration guide for breaking changes in this PR. Will need re-evaluation for full v6 migration guide later.
  2. Add test coverage for InterfaceQuantityWithUnitTypeConverter — currently appears unused/untested.

Low priority

  1. QuantityGenerator.cs — Add example code comments in generated code sections for readability.
  2. QuantityValueFormatOptions.cs — Align serialization/deserialization enum names (DecimalPrecision vs ExactNumber should use consistent naming).
  3. QuantityInfoBuilderExtensionsConfigure extension on UnitDefinition[] is awkward API; consider wrapper type or static method.

Won't do

  • Roslyn analyzer for migration assistance.

Reference: Breaking Changes Summary

Change Before (v5) After (v6)
IQuantity.Value double QuantityValue
Unit properties (e.g. .Meters) double QuantityValue
Length / Length double QuantityValue
As(), ToUnit() Interface methods Extension methods (some [Obsolete])
UnitConverter() constructor Public, parameterless Removed; use UnitConverter.Create(...)
SetConversionFunction / GetConversionFunction Available Removed
UnitsNetSetup constructor Public Private; use builder pattern
DataContract serialization double field QuantityValue struct (numerator/denominator)
AbbreviatedUnitsConverter (JsonNet) IReadOnlyDictionary constructor UnitParser + QuantityValueFormatOptions
Default JSON precision ~17 significant digits Up to 29 significant digits
MissingMemberHandling.Error Silently skipped unknowns Now correctly throws (bug fix)
Null IQuantity deserialization Returns .Zero Returns null
Conversion factors Floating-point approximations Exact rational fractions

@lipchev
Copy link
Collaborator Author

lipchev commented Mar 9, 2026

@angularsen I'm sorry, but P0 is a no-go. I have been very explicit about it since the very beginning (even before this PR).
You could try changing it (locally), just to get an idea of how many things would break (ambiguous assignments).

@angularsen
Copy link
Owner

I see, I'll try and change it locally to get a better feel for why adding implicit cast is challenging.
The way I understand this breaking change and how it will affect consumers' code, it currently feels too invasive of a change, but I'll need to map out those use cases to know for sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants