Move default unit converter registration to quantity types #1018

tmilnthorp · 2022-01-25T14:18:14Z

Move default unit converter registration to unit's static constructor.

Attempt to resolve #1016

tmilnthorp · 2022-01-25T14:20:36Z

I am hoping this fixes #1016, but even if not, I think it's a bit easier to find what's going on in the static constructors.

Hopefully this would also bring down our "startup" time a bit.

codecov · 2022-01-25T14:31:11Z

Codecov Report

Merging #1018 (67db99e) into master (28f9dc8) will increase coverage by 0.1%.
The diff coverage is 100.0%.

@@           Coverage Diff            @@
##           master   #1018     +/-   ##
========================================
+ Coverage    82.8%   82.9%   +0.1%     
========================================
  Files         310     309      -1     
  Lines       46695   46928    +233     
========================================
+ Hits        38635   38867    +232     
- Misses       8060    8061      +1

Impacted Files	Coverage Δ
UnitsNet/UnitConverter.cs	`83.0% <91.0%> (+0.7%)`	⬆️
...nitsNet/GeneratedCode/Quantities/Acceleration.g.cs	`83.9% <100.0%> (+1.3%)`	⬆️
...et/GeneratedCode/Quantities/AmountOfSubstance.g.cs	`84.2% <100.0%> (+1.4%)`	⬆️
...tsNet/GeneratedCode/Quantities/AmplitudeRatio.g.cs	`78.5% <100.0%> (+0.7%)`	⬆️
UnitsNet/GeneratedCode/Quantities/Angle.g.cs	`84.1% <100.0%> (+1.4%)`	⬆️
...tsNet/GeneratedCode/Quantities/ApparentEnergy.g.cs	`77.8% <100.0%> (+0.6%)`	⬆️
...itsNet/GeneratedCode/Quantities/ApparentPower.g.cs	`78.5% <100.0%> (+0.7%)`	⬆️
UnitsNet/GeneratedCode/Quantities/Area.g.cs	`85.7% <100.0%> (+1.2%)`	⬆️
UnitsNet/GeneratedCode/Quantities/AreaDensity.g.cs	`75.3% <100.0%> (+0.3%)`	⬆️
.../GeneratedCode/Quantities/AreaMomentOfInertia.g.cs	`80.3% <100.0%> (+0.9%)`	⬆️
... and 103 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 28f9dc8...67db99e. Read the comment docs.

tmilnthorp · 2022-01-25T21:04:36Z

@angularsen any idea what the issue is with codecov project?

angularsen

Just skimmed it, one question

angularsen · 2022-01-25T23:41:56Z

UnitsNet/GeneratedCode/Quantities/Acceleration.g.cs

@@ -72,6 +72,39 @@ static Acceleration()
                    new UnitInfo<AccelerationUnit>(AccelerationUnit.StandardGravity, "StandardGravity", new BaseUnits(length: LengthUnit.Meter, time: DurationUnit.Second)),
                },
                BaseUnit, Zero, BaseDimensions, QuantityType.Acceleration);
+
+            // Register in default unit converter: BaseUnit -> AccelerationUnit
+            UnitConverter.Default.SetConversionFunction<Acceleration>(AccelerationUnit.MeterPerSecondSquared, AccelerationUnit.CentimeterPerSecondSquared, quantity => quantity.ToUnit(AccelerationUnit.CentimeterPerSecondSquared));


Ok, interesting idea to break things up while not requiring the client to call some init method of sorts. It could help with both the issue and startup time.

I am wondering, can this result in conversions failing in some way? That they are not initialized yet, because the Acceleration static ctor just wasn't called yet in that particular call path?

Aha, yes. If you call the following:

var func = UnitConverter.Default.GetConversionFunction<Acceleration>( AccelerationUnit.MeterPerSecondSquared, AccelerationUnit.KilometerPerSecondSquared );

It fails since it has not been registered yet, despite having the Acceleration type utilized. Let me see if I can fix this.

Ok, I created a separate internal static RegisterDefaultConversions method in each of our quantities. I use reflection on the first call to the UnitConverter (as before) to register them all. This makes sure everything is initialized as-before, but without a giant method with all the types hard-coded into it.

Startup time won't be improved now, but hopefully the original problem will be resolved. It is nicer to look at also :)

Nice 🐷💄

angularsen · 2022-01-25T23:44:52Z

@angularsen any idea what the issue is with codecov project?

It seems the PR diff is fine, but not the merged PR. It seems to stumble on -0.0%. Migth be a configuration error. I know I had to upgrade the codecov uploader script recently, so it might be related. I think we can just ignore it for now.

tmilnthorp · 2022-01-26T13:27:25Z

@angularsen any idea what the issue is with codecov project?

It seems the PR diff is fine, but not the merged PR. It seems to stumble on -0.0%. Migth be a configuration error. I know I had to upgrade the codecov uploader script recently, so it might be related. I think we can just ignore it for now.

Ok sounds good.

…ection from UnitConverter

angularsen · 2022-01-26T20:29:19Z

@tmilnthorp A merge conflict after some recent PRs merged

…nRegistration

tmilnthorp · 2022-01-26T20:33:01Z

@tmilnthorp A merge conflict after some recent PRs merged

@angularsen fixed

angularsen

I think this looks great, just the merge conflict and some minor suggestions you can consider.

angularsen · 2022-01-26T20:30:35Z

UnitsNet/GeneratedCode/Quantities/Acceleration.g.cs

+            unitConverter.SetConversionFunction<Acceleration>(AccelerationUnit.MillistandardGravity, AccelerationUnit.MeterPerSecondSquared, quantity => quantity.ToBaseUnit());
+            unitConverter.SetConversionFunction<Acceleration>(AccelerationUnit.NanometerPerSecondSquared, AccelerationUnit.MeterPerSecondSquared, quantity => quantity.ToBaseUnit());
+            unitConverter.SetConversionFunction<Acceleration>(AccelerationUnit.StandardGravity, AccelerationUnit.MeterPerSecondSquared, quantity => quantity.ToBaseUnit());
+        }


I like the new structure ♥

A minor idea, but since we are just about always adding two-way conversions, could SetConversionFunction have an overload method to define both ways in one call?

Pretty much just an extra conversion function parameter if I am reading it right.

Could be named SetTwoWayConversionFunctions

A minor idea, but since we are just about always adding two-way conversions, could SetConversionFunction have an overload method to define both ways in one call?

Pretty much just an extra conversion function parameter if I am reading it right.

I tried adding an overload, but IMHO the lines became a bit more "mentally taxing" (even more so than it already is). It is certainly shorter in height though:

public void SetConversionFunction<TQuantity>(Enum from, Enum to, ConversionFunction<TQuantity> conversionFunction, ConversionFunction<TQuantity> reverseConversionFunction) where TQuantity : IQuantity { }

/// <summary> /// Registers the default conversion functions in the given <see cref="UnitConverter"/> instance. /// </summary> /// <param name="unitConverter">The <see cref="UnitConverter"/> to register the default conversion functions in.</param> internal static void RegisterDefaultConversions(UnitConverter unitConverter) { // Register in unit converter: BaseUnit <-> BaseUnit unitConverter.SetConversionFunction<Acceleration>(AccelerationUnit.MeterPerSecondSquared, AccelerationUnit.MeterPerSecondSquared, quantity => quantity); // Register in unit converter: BaseUnit -> AccelerationUnit unitConverter.SetConversionFunction<Acceleration>(AccelerationUnit.MeterPerSecondSquared, AccelerationUnit.CentimeterPerSecondSquared, quantity => quantity.ToUnit(AccelerationUnit.CentimeterPerSecondSquared), quantity => quantity.ToBaseUnit()); unitConverter.SetConversionFunction<Acceleration>(AccelerationUnit.MeterPerSecondSquared, AccelerationUnit.DecimeterPerSecondSquared, quantity => quantity.ToUnit(AccelerationUnit.DecimeterPerSecondSquared), quantity => quantity.ToBaseUnit()); unitConverter.SetConversionFunction<Acceleration>(AccelerationUnit.MeterPerSecondSquared, AccelerationUnit.FootPerSecondSquared, quantity => quantity.ToUnit(AccelerationUnit.FootPerSecondSquared), quantity => quantity.ToBaseUnit()); unitConverter.SetConversionFunction<Acceleration>(AccelerationUnit.MeterPerSecondSquared, AccelerationUnit.InchPerSecondSquared, quantity => quantity.ToUnit(AccelerationUnit.InchPerSecondSquared), quantity => quantity.ToBaseUnit()); unitConverter.SetConversionFunction<Acceleration>(AccelerationUnit.MeterPerSecondSquared, AccelerationUnit.KilometerPerSecondSquared, quantity => quantity.ToUnit(AccelerationUnit.KilometerPerSecondSquared), quantity => quantity.ToBaseUnit()); unitConverter.SetConversionFunction<Acceleration>(AccelerationUnit.MeterPerSecondSquared, AccelerationUnit.KnotPerHour, quantity => quantity.ToUnit(AccelerationUnit.KnotPerHour), quantity => quantity.ToBaseUnit()); unitConverter.SetConversionFunction<Acceleration>(AccelerationUnit.MeterPerSecondSquared, AccelerationUnit.KnotPerMinute, quantity => quantity.ToUnit(AccelerationUnit.KnotPerMinute), quantity => quantity.ToBaseUnit()); unitConverter.SetConversionFunction<Acceleration>(AccelerationUnit.MeterPerSecondSquared, AccelerationUnit.KnotPerSecond, quantity => quantity.ToUnit(AccelerationUnit.KnotPerSecond), quantity => quantity.ToBaseUnit()); unitConverter.SetConversionFunction<Acceleration>(AccelerationUnit.MeterPerSecondSquared, AccelerationUnit.MicrometerPerSecondSquared, quantity => quantity.ToUnit(AccelerationUnit.MicrometerPerSecondSquared), quantity => quantity.ToBaseUnit()); unitConverter.SetConversionFunction<Acceleration>(AccelerationUnit.MeterPerSecondSquared, AccelerationUnit.MillimeterPerSecondSquared, quantity => quantity.ToUnit(AccelerationUnit.MillimeterPerSecondSquared), quantity => quantity.ToBaseUnit()); unitConverter.SetConversionFunction<Acceleration>(AccelerationUnit.MeterPerSecondSquared, AccelerationUnit.MillistandardGravity, quantity => quantity.ToUnit(AccelerationUnit.MillistandardGravity), quantity => quantity.ToBaseUnit()); unitConverter.SetConversionFunction<Acceleration>(AccelerationUnit.MeterPerSecondSquared, AccelerationUnit.NanometerPerSecondSquared, quantity => quantity.ToUnit(AccelerationUnit.NanometerPerSecondSquared), quantity => quantity.ToBaseUnit()); unitConverter.SetConversionFunction<Acceleration>(AccelerationUnit.MeterPerSecondSquared, AccelerationUnit.StandardGravity, quantity => quantity.ToUnit(AccelerationUnit.StandardGravity), quantity => quantity.ToBaseUnit()); }

Breaking it into multiple lines gets even worse:

/// <summary> /// Registers the default conversion functions in the given <see cref="UnitConverter"/> instance. /// </summary> /// <param name="unitConverter">The <see cref="UnitConverter"/> to register the default conversion functions in.</param> internal static void RegisterDefaultConversions(UnitConverter unitConverter) { // Register in unit converter: BaseUnit <-> BaseUnit unitConverter.SetConversionFunction<Acceleration>(AccelerationUnit.MeterPerSecondSquared, AccelerationUnit.MeterPerSecondSquared, quantity => quantity); // Register in unit converter: BaseUnit -> AccelerationUnit unitConverter.SetConversionFunction<Acceleration>(AccelerationUnit.MeterPerSecondSquared, AccelerationUnit.CentimeterPerSecondSquared, quantity => quantity.ToUnit(AccelerationUnit.CentimeterPerSecondSquared), quantity => quantity.ToBaseUnit()); unitConverter.SetConversionFunction<Acceleration>(AccelerationUnit.MeterPerSecondSquared, AccelerationUnit.DecimeterPerSecondSquared, quantity => quantity.ToUnit(AccelerationUnit.DecimeterPerSecondSquared), quantity => quantity.ToBaseUnit()); unitConverter.SetConversionFunction<Acceleration>(AccelerationUnit.MeterPerSecondSquared, AccelerationUnit.FootPerSecondSquared, quantity => quantity.ToUnit(AccelerationUnit.FootPerSecondSquared), quantity => quantity.ToBaseUnit()); unitConverter.SetConversionFunction<Acceleration>(AccelerationUnit.MeterPerSecondSquared, AccelerationUnit.InchPerSecondSquared, quantity => quantity.ToUnit(AccelerationUnit.InchPerSecondSquared), quantity => quantity.ToBaseUnit()); unitConverter.SetConversionFunction<Acceleration>(AccelerationUnit.MeterPerSecondSquared, AccelerationUnit.KilometerPerSecondSquared, quantity => quantity.ToUnit(AccelerationUnit.KilometerPerSecondSquared), quantity => quantity.ToBaseUnit()); unitConverter.SetConversionFunction<Acceleration>(AccelerationUnit.MeterPerSecondSquared, AccelerationUnit.KnotPerHour, quantity => quantity.ToUnit(AccelerationUnit.KnotPerHour), quantity => quantity.ToBaseUnit()); unitConverter.SetConversionFunction<Acceleration>(AccelerationUnit.MeterPerSecondSquared, AccelerationUnit.KnotPerMinute, quantity => quantity.ToUnit(AccelerationUnit.KnotPerMinute), quantity => quantity.ToBaseUnit()); unitConverter.SetConversionFunction<Acceleration>(AccelerationUnit.MeterPerSecondSquared, AccelerationUnit.KnotPerSecond, quantity => quantity.ToUnit(AccelerationUnit.KnotPerSecond), quantity => quantity.ToBaseUnit()); unitConverter.SetConversionFunction<Acceleration>(AccelerationUnit.MeterPerSecondSquared, AccelerationUnit.MicrometerPerSecondSquared, quantity => quantity.ToUnit(AccelerationUnit.MicrometerPerSecondSquared), quantity => quantity.ToBaseUnit()); unitConverter.SetConversionFunction<Acceleration>(AccelerationUnit.MeterPerSecondSquared, AccelerationUnit.MillimeterPerSecondSquared, quantity => quantity.ToUnit(AccelerationUnit.MillimeterPerSecondSquared), quantity => quantity.ToBaseUnit()); unitConverter.SetConversionFunction<Acceleration>(AccelerationUnit.MeterPerSecondSquared, AccelerationUnit.MillistandardGravity, quantity => quantity.ToUnit(AccelerationUnit.MillistandardGravity), quantity => quantity.ToBaseUnit()); unitConverter.SetConversionFunction<Acceleration>(AccelerationUnit.MeterPerSecondSquared, AccelerationUnit.NanometerPerSecondSquared, quantity => quantity.ToUnit(AccelerationUnit.NanometerPerSecondSquared), quantity => quantity.ToBaseUnit()); unitConverter.SetConversionFunction<Acceleration>(AccelerationUnit.MeterPerSecondSquared, AccelerationUnit.StandardGravity, quantity => quantity.ToUnit(AccelerationUnit.StandardGravity), quantity => quantity.ToBaseUnit()); }

Got it. I agree, the line became tough to read. The best I could come up with was:

unitConverter.SetConversionFunction<Acceleration>(AccelerationUnit.MeterPerSecondSquared, AccelerationUnit.CentimeterPerSecondSquared, quantity => quantity.ToUnit(AccelerationUnit.CentimeterPerSecondSquared), quantity => quantity.ToBaseUnit());

Doesn't save vertical space though, so I don't know. I don't think it helps on neither the issue we are trying to fix nor readability. The code is also generated anyhow. LGTM!

angularsen · 2022-01-26T20:34:35Z

UnitsNet/UnitConverter.cs

+                .Wrap()
+                .Assembly
+                .GetExportedTypes()
+                .Where(t => typeof(IQuantity).Wrap().IsAssignableFrom(t));


This is so much better to read 🙌

angularsen · 2022-01-26T21:25:54Z

Nuget should be out shortly
Release UnitsNet/4.115.0 · angularsen/UnitsNet

tmilnthorp added 3 commits January 25, 2022 09:09

Move default conversion function registration to unit static constructor

2d58a47

Use ConcurrentDictionary

a91a032

Cleanup

9af6275

tmilnthorp requested a review from angularsen January 25, 2022 14:18

tmilnthorp self-assigned this Jan 25, 2022

tmilnthorp mentioned this pull request Jan 25, 2022

InvalidProgramException: locals size too big #1016

Closed

tmilnthorp changed the title ~~In unit conversion registration~~ Move default unit converter registration to unit's static constructor. Jan 25, 2022

No longer partial

f7a029f

tmilnthorp added 2 commits January 25, 2022 17:01

Make it beautiful

f3ba125

Use nice name

41184e2

angularsen reviewed Jan 25, 2022

View reviewed changes

Create seperate RegisterDefaultConversions function and call via refl…

1c6ae4c

…ection from UnitConverter

tmilnthorp changed the title ~~Move default unit converter registration to unit's static constructor.~~ Move default unit converter registration to static method in unit. Jan 26, 2022

tmilnthorp added 2 commits January 26, 2022 09:01

Go back to Dictionary since no longer in static constructors

d777af6

Don't need exception checking in internal method

fb8d63f

Merge remote-tracking branch 'angularsen/master' into InUnitConversio…

67db99e

…nRegistration

angularsen reviewed Jan 26, 2022

View reviewed changes

tmilnthorp mentioned this pull request Jan 26, 2022

The library does not compile when used with IL2CPP in Unity3D #1009

Closed

angularsen changed the title ~~Move default unit converter registration to static method in unit.~~ Move default unit converter registration to quantity types Jan 26, 2022

angularsen merged commit fa983e2 into angularsen:master Jan 26, 2022

tmilnthorp mentioned this pull request Feb 8, 2022

Move abbreviation mapping to individual quantity types #1031

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move default unit converter registration to quantity types #1018

Move default unit converter registration to quantity types #1018

tmilnthorp commented Jan 25, 2022

tmilnthorp commented Jan 25, 2022

codecov bot commented Jan 25, 2022 •

edited

Loading

tmilnthorp commented Jan 25, 2022

angularsen left a comment

angularsen Jan 25, 2022

tmilnthorp Jan 26, 2022

tmilnthorp Jan 26, 2022 •

edited

Loading

angularsen Jan 26, 2022

angularsen commented Jan 25, 2022

tmilnthorp commented Jan 26, 2022

angularsen commented Jan 26, 2022

tmilnthorp commented Jan 26, 2022

angularsen left a comment

angularsen Jan 26, 2022

angularsen Jan 26, 2022

angularsen Jan 26, 2022

tmilnthorp Jan 26, 2022

angularsen Jan 26, 2022

angularsen Jan 26, 2022

angularsen commented Jan 26, 2022

Move default unit converter registration to quantity types #1018

Move default unit converter registration to quantity types #1018

Conversation

tmilnthorp commented Jan 25, 2022

tmilnthorp commented Jan 25, 2022

codecov bot commented Jan 25, 2022 • edited Loading

Codecov Report

tmilnthorp commented Jan 25, 2022

angularsen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmilnthorp Jan 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angularsen commented Jan 25, 2022

tmilnthorp commented Jan 26, 2022

angularsen commented Jan 26, 2022

tmilnthorp commented Jan 26, 2022

angularsen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angularsen commented Jan 26, 2022

codecov bot commented Jan 25, 2022 •

edited

Loading

tmilnthorp Jan 26, 2022 •

edited

Loading