Use UnitConverter to do conversion functions and allow basic extensibility #1023

tmilnthorp · 2022-01-28T21:54:03Z

Here's a very-rough initial idea for supporting #865.

Rather than generating functions that are not extensible, we can reuse our existing UnitConverter class and be able to use it for conversions. Users could add/remove custom functions at-will.

Need to formalize this more as we should really use QuantityInfo for the parameters.

CodeGen/Generators/UnitsNetGen/UnitTestBaseClassGenerator.cs

angularsen · 2022-01-28T23:01:52Z

UnitsNet/GeneratedCode/Quantities/Angle.g.cs

+
+            var inBaseUnits = ToUnit(BaseUnit);
+
+            if(!unitConverter.TryGetConversionFunction<Angle>(inBaseUnits.Unit, unit, out var conversionFunction))


I like it.

As a very minor performance improvement, can we call the innermost method overload? I think it has to go through 2 additional TryGetConversionFunction methods to get to the most specific one.

Besides the minor detail of failing tests, I think this looks good?

So in order to extend this, we can simply call Acceleration.DefaultConversionFunctions.SetXXX() on startup?

Will there be a racing condition with the initialization of the global static UnitConverter.Default?

Need to formalize this more as we should really use QuantityInfo for the parameters.

What did you mean by this?

I like it.

As a very minor performance improvement, can we call the innermost method overload? I think it has to go through 2 additional TryGetConversionFunction methods to get to the most specific one.

Done!

Besides the minor detail of failing tests, I think this looks good?

So in order to extend this, we can simply call Acceleration.DefaultConversionFunctions.SetXXX() on startup?

Will there be a racing condition with the initialization of the global static UnitConverter.Default?

You certainly could. I do worry a bit though about that causing unintended consequences, especially since GetValueAs uses the DefaultConversionFunctions. This would affect:

operator +

operator -

operator <=

operator >=

operator <

operator >

CompareTo

Equals

As

That said, if you write a custom conversion function, arguably it should end up being used for * for example.

…o BaseUnits.

codecov · 2022-02-03T15:49:29Z

Codecov Report

Merging #1023 (c5be852) into master (8d2eeb6) will decrease coverage by 2.3%.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           master   #1023     +/-   ##
========================================
- Coverage    82.7%   80.4%   -2.2%     
========================================
  Files         309     309             
  Lines       47598   46126   -1472     
========================================
- Hits        39317   37081   -2236     
- Misses       8281    9045    +764

Impacted Files	Coverage Δ
...nitsNet/GeneratedCode/Quantities/Acceleration.g.cs	`81.3% <ø> (-2.2%)`	⬇️
...et/GeneratedCode/Quantities/AmountOfSubstance.g.cs	`81.7% <ø> (-2.3%)`	⬇️
...tsNet/GeneratedCode/Quantities/AmplitudeRatio.g.cs	`76.6% <ø> (-1.6%)`	⬇️
UnitsNet/GeneratedCode/Quantities/Angle.g.cs	`81.5% <ø> (-2.3%)`	⬇️
...tsNet/GeneratedCode/Quantities/ApparentEnergy.g.cs	`76.0% <ø> (-1.5%)`	⬇️
...itsNet/GeneratedCode/Quantities/ApparentPower.g.cs	`76.6% <ø> (-1.6%)`	⬇️
UnitsNet/GeneratedCode/Quantities/Area.g.cs	`83.2% <ø> (-2.2%)`	⬇️
UnitsNet/GeneratedCode/Quantities/AreaDensity.g.cs	`72.2% <ø> (-2.9%)`	⬇️
.../GeneratedCode/Quantities/AreaMomentOfInertia.g.cs	`78.2% <ø> (-1.8%)`	⬇️
UnitsNet/GeneratedCode/Quantities/BitRate.g.cs	`84.3% <ø> (-2.4%)`	⬇️
... and 216 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d2eeb6...c5be852. Read the comment docs.

tmilnthorp · 2022-02-03T16:46:54Z

Hmmm let me add some tests. Kinda shocked we don't have any tests for to base conversion in ToUnit!

tmilnthorp · 2022-02-04T14:35:26Z

UnitsNet.Tests/GeneratedCode/TestsBase/AccelerationTestsBase.g.cs

-        [Fact]
-        public void ToUnit()
+        [Theory]
+        [MemberData(nameof(UnitTypes))]


I parameterized a few tests to make them nicer

tmilnthorp · 2022-02-04T18:25:34Z

I think this is ready. I wonder why the new CodeCov report isn't coming through?

angularsen

Mostly nitpicks, which means the PR is good.

CodeGen/Generators/UnitsNetGen/UnitTestBaseClassGenerator.cs

UnitsNet.Tests/GeneratedCode/TestsBase/AccelerationTestsBase.g.cs

UnitsNet/GeneratedCode/Quantities/ElectricChargeDensity.g.cs

angularsen · 2022-02-04T23:01:44Z

I have no idea how the codecov is triggered. It almost seems random.

angularsen · 2022-02-04T23:07:52Z

It seems codecov failed to process the last uploaded coverage reports. Can't see any error codes or any details on what though.

https://codecov.io/github/angularsen/UnitsNet/commit/b20c987ade7301039ed68e63b16137101e966ac3

angularsen · 2022-02-08T21:31:32Z

Release UnitsNet/4.117.0 · angularsen/UnitsNet

lipchev · 2022-02-14T17:17:49Z

Hey guys, I've just updated the release note- indicating that this is a breaking change for implementors of the IQuantity interface.

lipchev · 2022-02-14T17:29:45Z

And by the way- I haven't looked into this closely but - couldn't this be made into an extension method? We could still have the GetValueAs function call it by default, while people extending the main interface can choose to completely ignore it for their 'custom quantities'.
I wasn't really affected (only had 2-3 spots needed the extra implementation) - but one would have to copy-paste the full body for each custom type which is not ideal.
@angularsen @tmilnthorp

tmilnthorp · 2022-02-15T11:20:50Z

Hey guys, I've just updated the release note- indicating that this is a breaking change for implementors of the IQuantity interface.

Oops. Got a bit carried away there 😄

In retrospect, I think I should just remove the UnitConverter overloads. How the conversion / extensibility is implemented is just an implementation detail. Interfaces should be as small as possible to provide the required functionality and no more.

Would you guys prefer removing them now (also a breaking change, although arguably for fewer consumers between minor versions), or making them obsolete for v5 removal?

lipchev · 2022-02-15T11:36:44Z

I think the impact of doing it now would be smaller (probably just me and you).

tmilnthorp · 2022-02-15T13:45:16Z

I think the impact of doing it now would be smaller (probably just me and you).

I would tend to agree. I made #1047 to remove this overload. I will think about your other proposal in another PR, just wanted to get that break out (if @angularsen agrees this is the best action).

angularsen · 2022-02-15T20:46:13Z

I agree to remove for now to avoid breaking change in v4.

I was thinking if we could maybe make use of default interface methods, but this seems limited to netstandard2.1 / netcore3 and I believe we should still support nestandard2.0.

tmilnthorp · 2022-02-15T21:15:49Z

I agree to remove for now to avoid breaking change in v4.

I was thinking if we could maybe make use of default interface methods, but this seems limited to netstandard2.1 / netcore3 and I believe we should still support nestandard2.0.

I had thought of that also, but I wouldn't be able to provide a default implementation anyways, since DefaultConverter lives on the individual quantity implementations, not interface definition.

@lipchev

Co-authored-by: Andreas Gullberg Larsen <[email protected]> Fixes #1069 Per @lipchev's comments in #1023, I decided to add the local conversion method back (as private ```TryToUnit```). This will allow better performance for auto-generated conversions before falling back to the extensibility points.

tmilnthorp added 6 commits January 28, 2022 15:08

Use UnitConverter to register conversion functions

6123b66

Use static instance

c85f156

Optimize

980b73e

Use ToUnit(Base) rather than a specialized function

9e3cc61

Make ToUnit the actual converter

d12806a

Add overloaded method for user-supplied converter

476a447

tmilnthorp commented Jan 28, 2022

View reviewed changes

CodeGen/Generators/UnitsNetGen/UnitTestBaseClassGenerator.cs Outdated Show resolved Hide resolved

tmilnthorp requested review from angularsen and lipchev January 28, 2022 21:56

angularsen approved these changes Jan 28, 2022

View reviewed changes

tmilnthorp changed the title ~~Unitconversion~~ Use UnitConverter to do conversion functions and allow basic extensibility Jan 31, 2022

tmilnthorp added 4 commits January 31, 2022 09:32

Bypass overloads

767dd9f

Add copy constructor for UnitConverter

4fd823b

Merge branch 'master' into unitconversion

414ddff

Check for direct conversions first. Avoid stack overflow when going t…

4ff6060

…o BaseUnits.

tmilnthorp marked this pull request as ready for review February 3, 2022 15:48

tmilnthorp added 2 commits February 3, 2022 11:19

Better if/else blocks

5c4ad4a

Better ToUnit tests for all units

60336db

tmilnthorp added 5 commits February 3, 2022 11:50

Merge remote-tracking branch 'angularsen/master' into unitconversion

aef45b5

Make test data reusable

382ef1c

Add test converting from non-base unit to each unit

4969026

Better conversion test

efe06a5

Parameterize ToUnit test

34ec675

tmilnthorp commented Feb 4, 2022

View reviewed changes

tmilnthorp added 2 commits February 4, 2022 10:05

Add new ToUnit overload to interfaces. Clean up doc.

8e338e1

Whitespace

b20c987

angularsen requested changes Feb 4, 2022

View reviewed changes

CodeGen/Generators/UnitsNetGen/UnitTestBaseClassGenerator.cs Outdated Show resolved Hide resolved

UnitsNet.Tests/GeneratedCode/TestsBase/AccelerationTestsBase.g.cs Outdated Show resolved Hide resolved

UnitsNet/GeneratedCode/Quantities/ElectricChargeDensity.g.cs Show resolved Hide resolved

tmilnthorp added 4 commits February 7, 2022 09:24

Whitespace fix

02700b9

More whitespace

2899527

Remove debugging test

bf77dda

Check converted unit

c5be852

angularsen merged commit e5aba5b into angularsen:master Feb 8, 2022

tmilnthorp mentioned this pull request Feb 16, 2022

⚡ Prefer built-in conversions over extensibility for perf #1049

Merged

gmkado mentioned this pull request Mar 4, 2024

v4.117.0 breaks quantity comparison between different units #1376

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use UnitConverter to do conversion functions and allow basic extensibility #1023

Use UnitConverter to do conversion functions and allow basic extensibility #1023

tmilnthorp commented Jan 28, 2022

angularsen Jan 28, 2022

angularsen Jan 28, 2022 •

edited

Loading

angularsen Jan 28, 2022

tmilnthorp Jan 31, 2022

tmilnthorp Jan 31, 2022

codecov bot commented Feb 3, 2022 •

edited

Loading

tmilnthorp commented Feb 3, 2022

tmilnthorp Feb 4, 2022

tmilnthorp commented Feb 4, 2022

angularsen left a comment

angularsen commented Feb 4, 2022 •

edited

Loading

angularsen commented Feb 4, 2022

angularsen commented Feb 8, 2022

lipchev commented Feb 14, 2022

lipchev commented Feb 14, 2022

tmilnthorp commented Feb 15, 2022

lipchev commented Feb 15, 2022

tmilnthorp commented Feb 15, 2022

angularsen commented Feb 15, 2022

tmilnthorp commented Feb 15, 2022


		var inBaseUnits = ToUnit(BaseUnit);

		if(!unitConverter.TryGetConversionFunction<Angle>(inBaseUnits.Unit, unit, out var conversionFunction))

Use UnitConverter to do conversion functions and allow basic extensibility #1023

Use UnitConverter to do conversion functions and allow basic extensibility #1023

Conversation

tmilnthorp commented Jan 28, 2022

angularsen Jan 28, 2022

Choose a reason for hiding this comment

angularsen Jan 28, 2022 • edited Loading

Choose a reason for hiding this comment

angularsen Jan 28, 2022

Choose a reason for hiding this comment

tmilnthorp Jan 31, 2022

Choose a reason for hiding this comment

tmilnthorp Jan 31, 2022

Choose a reason for hiding this comment

codecov bot commented Feb 3, 2022 • edited Loading

Codecov Report

tmilnthorp commented Feb 3, 2022

tmilnthorp Feb 4, 2022

Choose a reason for hiding this comment

tmilnthorp commented Feb 4, 2022

angularsen left a comment

Choose a reason for hiding this comment

angularsen commented Feb 4, 2022 • edited Loading

angularsen commented Feb 4, 2022

angularsen commented Feb 8, 2022

lipchev commented Feb 14, 2022

lipchev commented Feb 14, 2022

tmilnthorp commented Feb 15, 2022

lipchev commented Feb 15, 2022

tmilnthorp commented Feb 15, 2022

angularsen commented Feb 15, 2022

tmilnthorp commented Feb 15, 2022

angularsen Jan 28, 2022 •

edited

Loading

codecov bot commented Feb 3, 2022 •

edited

Loading

angularsen commented Feb 4, 2022 •

edited

Loading