Relative and absolute equality #451

tmilnthorp · 2018-06-21T16:22:18Z

Initial go at #391

tmilnthorp · 2018-06-21T16:30:34Z

UnitsNet/UnitDefinitions/Energy.json

@@ -136,8 +136,8 @@
    {
      "SingularName": "ThermImperial",
      "PluralName": "ThermsImperial",
-      "FromUnitToBaseFunc": "x*1.05505585257348e+14",
-      "FromBaseToUnitFunc": "x/1.05505585257348e+14",
+      "FromUnitToBaseFunc": "x*1.05967e8",


There are actually 4 definitions: Therm EC/US/UK and BTU

https://en.wikipedia.org/wiki/Therm#Definitions

ThermImperial may be misleading in name, but refers to Therm UK.
There is already a unit for BritishThermalUnit (BTU) further up here.

So I think it is already correct, as is. Do you agree?

I did assume it was therm(UK). I thought that was therm_39, but looks like I'm completely wrong there. It caught my eye immediately though because the scale is completely off (e14).

If you look at Wikipedia, it should be 1.05505585257348e8, not 1.05505585257348e14. Looks like the decimal on Wikipedia got overlooked :)

So just to make sure I'm not missing anything, 1.05505585257348e8 seems correct, do you agree?

Ah I missed that part :-D Yup, should be e8!

I will change it to that then!

angularsen

A couple of things on the work so far, but I like where this is headed.

angularsen · 2018-06-21T17:11:58Z

UnitsNet.Tests/AssertEx.cs

+                double difference = Math.Abs(expected - actual);
+                double percentDifference = (difference / actual) * 100.0d;
+
+                Assert.True( areEqual, $"Values are not equal within relative tolerance: {tolerance * 100.0d}%\nExpected: {expected}\nActual: {actual}\nDiff: {percentDifference}%" );


You can use format P2 as in {0.0354321:P2} to get "3.54%" string including the % symbol. It also uses the right symbol for the current culture.

I would perhaps argue that percentage should be relative to expected and not actual, since you don't really know what actual is, but expected is .. well.. what you expect :-) If expected is 10 and actual is 0.000001, then a difference of ~10 becomes a huge percentage instead of having an error of ~100%.

I didn't know you could do the P2 formatting outside of a ToString. Nice! I'll update that. And I agree that it should be against the expected.

angularsen · 2018-06-21T17:19:53Z

UnitsNet/Comparison.cs

+    {
+        public static bool Equals( double value1, double value2, double tolerance = 0.00001, ComparisonType comparisonType = ComparisonType.Relative)
+        {
+            if(comparisonType == ComparisonType.Relative)


A switch can be slightly more readable when comparing the same value multiple times and returning a value based on it.

angularsen · 2018-06-21T17:21:53Z

UnitsNet/Comparison.cs

+                throw new InvalidOperationException("The ComparisonType is unsupported.");
+        }
+
+        public static bool EqualsRelative( double value1, double value2, double tolerance = 0.00001)


The xmldoc should explain that the tolerance is relative to value1, which is kind of important I think.
Consider renaming value1 to referenceValue, since we must choose a reference to compare relative to. The other could be named value maybe. Perhaps there are some naming conventions on this sort of thing.

I agree. I've updated this and added documentation that I hope is clear.

angularsen · 2018-06-21T17:22:28Z

UnitsNet/Comparison.cs

+
+        public static bool EqualsRelative( double value1, double value2, double tolerance = 0.00001)
+        {
+            double maxVariation = Math.Abs( value1 * tolerance );


We should throw if tolerance is < 0, even if it would work the same.

Agreed. I was going to initially say <= 0 but I think it's perfectly valid to request being exact if desired.

angularsen · 2018-06-21T17:24:18Z

UnitsNet/GeneratedCode/Extensions/Number/NumberToPowerDensityExtensions.g.cs

+        #region PetawattPerCubicMeter
+
+        /// <inheritdoc cref="PowerDensity.FromPetawattsPerCubicMeter(UnitsNet.QuantityValue)" />
+        public static PowerDensity PetawattsPerCubicMeter(this int value) => PowerDensity.FromPetawattsPerCubicMeter(value);


Consider moving the PowerDensity units to a separate PR, even if that is probably why you started working on the abs vs relative comparison to begin with.

That is indeed why I started :) I just forgot to remove it. Good catch

angularsen · 2018-06-21T17:33:34Z

UnitsNet/GeneratedCode/Quantities/VolumeFlow.g.cs

+        /// <param name="tolerance">The comparison tolerance.</param>
+        /// <param name="comparisonType">The comparison type: either relative or absolute.</param>
+        /// <returns>True if the difference between the two values is not greater than the specified relative or absolute tolerance.</returns>
+        public bool Equals(VolumeFlow other, double tolerance = 0.00001, ComparisonType comparisonType = ComparisonType.Relative)


Tolerance probably needs to be of type VolumeFlow, since the consumer may be working with units of entirely different orders of magnitude than the base unit - so the default of 0.00001 may be way too small or way too large.

A second option is to rely on the fact that we keep track of the Unit the quantity was constructed with, so we could pick one of the quantities' Unit and convert both quantities to a numeric value of that unit, then compare that with a default numeric tolerance. So when comparing a 1 nanometer with 2 nanometers, then the tolerance would 0.000001 nanometers by default rather than 0.000001 meters. The pitfall here is that it matters WHICH value you choose the unit for, as you get very different behavior when comparing 1 kilometer to 1e12 nanometers vs comparing 1e12 nanometers to 1 kilometer. In this particular example, I would expect this to be the reference - which I guess is intuitive.

I think I like option 2 better. Absolute/relative can be a bit confusing and keeping it a double makes it easier to think of as a "percentage". Being typed would make sense for absolute differences, but since this is a generic method I think it's better as a double.

I made this in the same thinking of String.Equals where you can pass in the comparison type (ignore case, etc.).

I agree the reference being [this] is intuitive.

…lacing manual percentage calculation with format strings.

…g isn't done when not using base units. [this] will be the reference for relative units.

…ter with name "value"

angularsen · 2018-06-21T20:35:27Z

UnitsNet/Comparison.cs

        /// <param name="tolerance">The absolute or relative tolerance value.</param>
        /// <param name="comparisonType">Whether to use tolerance as an absolute or relative tolerance.</param>
        /// <returns></returns>
-        public static bool Equals(double referenceValue, double value, double tolerance, ComparisonType comparisonType)
+        public static bool Equals(double referenceValue, double otherValue, double tolerance, ComparisonType comparisonType)


WRC target is a big pain in the ass, I've contemplated dropping the support for it, but we have a Microsoft package that depends on it which is kind of cool.

I have created an issue to generate a separate code base for WRC, to avoid all the #if defs, which I think will be an improvement since most people don't care about WRC anyway. Then hopefully one will meet less friction when adding new stuff, since adding extra code like operator overloads would by default not be included in WRC.

The downside being a sh*tload more files generated for every unit etc added.

#409

I did see that, and considered picking that up :)

angularsen · 2018-06-22T11:37:53Z

UnitsNet.Tests/CustomCode/MassTests.cs

@@ -42,11 +42,11 @@ public class MassTests : MassTestsBase

        protected override double KilotonnesInOneKilogram => 1E-6;

-        protected override double LongTonsInOneKilogram => 0.000984207;
+        protected override double LongTonsInOneKilogram => 9.842065276110606e-4;


This PR is getting a bit crowded and not so easy to review. Could you perhaps pull out the bits not directly related to absolute/relative functionality in a separate PR?

Unfortunately all of these changes are so the tests pass. These validation numbers were off quite a bit when I changed the tests to validate by % tolerance (not precise enough).

So in other words, they're unfortunately all related to absolute/relative functionality! Especially since I have switched the tests over to relative tolerance by default.

Without the mass tests change, mass tests fail with the following (MegatonnesInOneKilogram in this example):

Message: Values are not equal within relative tolerance: 0.0010%
Expected: 1E-06
Actual: 1E-09
Diff: 99.9000%

As you can see, it was a difference of 1e-6, which was inside the 1e-5 tolerance, but it was off by almost double!! 😲

Haha 😆 it's so good to finally fix this!
I understand about the precision changes, no problem!

Is this getting ready now? I might find some time tomorrow to review.

Yup! I'm done making changes.

Whitespace fix

angularsen

Done! Phew, this is a ton of good work! You really did a huge job going through all these conversions and finding so many discrepancies in the test cases. Please see my comments.

angularsen · 2018-06-23T13:20:29Z

UnitsNet.Tests/CustomCode/DurationTests.cs


-        protected override double YearsInOneSecond => 3.1689e-8;
+        protected override double YearsInOneSecond => 3.170979198376458e-8;


This value was different than Years365 and the average year instead of 365 days.

Are you saying it is supposed to be different? Both are defines in the json files as x36524*3600, so I am not sure how they could be?

angularsen · 2018-06-23T13:27:35Z

UnitsNet.Tests/CustomCode/MassTests.cs


        protected override double MegapoundsInOneKilogram => 2.2046226218487757e-6;

-        protected override double MegatonnesInOneKilogram => 1E-6;
+        protected override double MegatonnesInOneKilogram => 1E-9;


Wow.. Just wow!

angularsen · 2018-06-24T07:41:32Z

UnitsNet/Comparison.cs

+
+namespace UnitsNet
+{
+    public sealed class Comparison


Make this class static? Should it be internal?

I thought I tried and Windows Runtime Component yelled at me. I'll double check because it should definitely be static. Not so sure about internal. Can be used anywhere if someone wishes!

It seems to be fine with static. I'll leave it as public though if that's ok with you?

angularsen · 2018-06-24T07:55:54Z

UnitsNet/GeneratedCode/Quantities/Acceleration.g.cs

@@ -790,6 +790,29 @@ public override bool Equals(object obj)
            return AsBaseUnitMetersPerSecondSquared().Equals(((Acceleration) obj).AsBaseUnitMetersPerSecondSquared());
        }

+        /// <summary>
+        ///     Compare equality to another Acceleration within the given absolute or relative tolerance.
+        ///     Relative tolerance is when the difference between the two quantities is not greater than the scale of the values compared to the tolerance.


How about:

Relative tolerance is defined by a fraction of this quantity, so a value of 0.01 means +/- 1% tolerance.
Absolute tolerance is defined by the max difference between this quantity and in the of this quantity. So if this quantity was constructed with 100 cm, then a tolerance of 0.01 means a tolerance of +/- 1 cm.

When writing this, I realize that I'm not sure this behavior for absolute tolerance is intuitive enough and whether we should have a separate method for absolute equality that instead take an Acceleration as tolerance? The relative tolerance implementation is fine I think.

I think having an example is good here. Makes it easier to understand. I'll add that.

As for making the method typed I'm not so sure. When we want to implement an IQuantity interface, having the generic method that takes a double is the only way to make it reusable as an interface method.

Right, but that's the part I'm not sure about. Will it be intuitive enough to expose an EqualsAbsolute() method generically? You don't necessarily know what unit you are working with, something like this:

public void MakeImportantDecisionIfTheseTwoArePrettyMuchEqual(Length a, Length b) { // What exactly is the allowed error here? Are we working with Meters here? Nanometers? Lightyears? if (a.EqualsAbsolute(b, 0.001)) DoTheImportantThing(); }

It doesn't get better if you substitute Length with IQuantity, then it could as well be oranges and apples let alone how many of them.

That is true with the interface, but generally I'd expect a check, much like Equals(Object) does. If the types are different it would just always return false. But all debatable :)

Are you saying you'd like to see:

var a = Length.FromMeters( 5.0 ); var isEqual = a.EqualsAbsolute( Length.FromMeters( 10.0 ), Length.FromNauticalMiles( 100000 ) )

angularsen · 2018-06-24T07:58:24Z

UnitsNet/UnitDefinitions/Pressure.json

-      "FromUnitToBaseFunc": "x*98066501.9960652",
-      "FromBaseToUnitFunc": "x*1.01971619222242E-08",
+      "FromUnitToBaseFunc": "x*9.80665e7",
+      "FromBaseToUnitFunc": "x/9.80665e7",


Nice that you are making this consistent with the guideline!

When the tests failed due to relative checks, I couldn't know if the conversion was wrong or the test, so I did both :)

… other, $quantityName maxError)

tmilnthorp · 2018-06-25T13:24:26Z

Ok. I've updated the documentation. Hope that looks good to you now!

angularsen

Just a couple of minor things. I'm still a bit on the fence on the absolute equality though, it feels a bit weird passing in an absolute tolerance without a unit. I honestly don't know if it is the base unit or if it is the constructed unit, without looking in the source code. It might be fine if the xmldoc clearly states the behavior, though, but I think it will catch some people off guard whereas requiring the unit will make them aware that this is a thing to consider.

angularsen · 2018-06-25T20:09:59Z

UnitsNet/Comparison.cs

+        ///     </para>
+        ///     <para>
+        ///     Absolute tolerance is defined as the maximum allowable absolute difference between <paramref name="referenceValue"/> and
+        ///     <paramref name="otherValue"/> as a fixed number.


Fixed number, but in what unit?

In this case it's just a double, so there is no unit. But when you're looking at the Equals on a quantity, the reference value is "this" so it's a fixed number of "this" unit.

var a = Length.FromMeters( 100.0 ); var b = Length.FromInches( 5.0 ); a.Equals( b, 0.001 )

In this case it's absolute of a. So within 0.001 meters.

Okay sure, that is one way to look at it. I will still argue that there is a unit in play here and it is the constructed unit of a, which is the most sensible choice by all means! However, I would have to read the current xmldoc more than once to understand what will actually happen, since there is conversions going on here and you are naturally not comparing the values 100 and 5 head on. If the xmldoc rather said the tolerance was in the referenceValue's Unit then at least in my eyes it would be much more intuitive. I guess we just have different perspective on this and read it a bit differently.

So for me, a slight change in wording of xmldoc would make it a lot more intuitive.

I think changing the wording definitely makes sense regardless!

angularsen · 2018-06-25T20:11:04Z

UnitsNet/Comparison.cs

        /// </summary>
        /// <param name="referenceValue">The reference value which the tolerance will be calculated against.</param>
        /// <param name="otherValue">The value to compare to.</param>
-        /// <param name="tolerance"></param>
+        /// <param name="tolerance">The relative tolerance. Must be greater than 0.</param>


greater or equal

angularsen · 2018-06-25T20:11:38Z

UnitsNet/Comparison.cs

        /// <param name="otherValue">The value to compare to.</param>
-        /// <param name="tolerance">The absolute or relative tolerance value.</param>
-        /// <param name="comparisonType">Whether to use tolerance as an absolute or relative tolerance.</param>
+        /// <param name="tolerance">The absolute or relative tolerance value. Must be greater than 0.</param>


greater or equal

angularsen · 2018-06-25T20:12:16Z

UnitsNet/Comparison.cs

@@ -70,7 +81,8 @@ public static bool EqualsRelative(double referenceValue, double otherValue, doub

        /// <summary>
        ///     Checks if two values are equal with a given absolute tolerance.
-        ///     Absolute tolerance is when the absolute difference between the two values is not greater than the tolerance value.
+        ///     Absolute tolerance is defined as the maximum allowable absolute difference between <paramref name="referenceValue"/> and
+        ///     <paramref name="otherValue"/> as a fixed number.


Same, what unit?

angularsen · 2018-06-25T20:13:01Z

UnitsNet/Scripts/Include-GenerateQuantitySourceCode.ps1

        /// </summary>
        /// <param name="other">The other quantity to compare to.</param>
-        /// <param name="tolerance">The comparison tolerance.</param>
+        /// <param name="tolerance">The absolute or relative tolerance value. Must be greater than 0.</param>


greater or equal

angularsen · 2018-06-25T20:13:12Z

UnitsNet/Scripts/Include-GenerateQuantitySourceCode.ps1

+        ///     </para>
+        ///     <para>
+        ///     Absolute tolerance is defined as the maximum allowable absolute difference between this quantity's value and
+        ///     <paramref name="other"/> as a fixed number.


Same, what unit?

…tity types

tmilnthorp · 2018-06-25T21:56:22Z

Updating the documentation on the Equals method in quantity classes

angularsen · 2018-06-26T07:35:57Z

UnitsNet/Scripts/Include-GenerateQuantitySourceCode.ps1

+        ///     </para>
+        /// </summary>
+        /// <param name="other">The other quantity to compare to.</param>
+        /// <param name="tolerance">The absolute or relative tolerance value. Must be greater than 0.</param>


greater or equal to 0

Also update Comparison with the same documentation updates.

tmilnthorp · 2018-06-26T13:22:11Z

I will also obsolete Equals($quantityName other, $quantityName maxError) since we have a method that does relative and obsolete now. I'll also change the Equals(Object) implementation

…als methods that were scaling to base units when unnecessary. Whitespace fixes.

…teEquality

angularsen · 2018-06-26T16:25:42Z

Awesome! Looks good, nice touch on fixing the old equals methods too.
Nuget 3.101 on the way out.

tmilnthorp added 2 commits June 21, 2018 09:58

Adding ability to compare by relative and absolute values

ce4c327

Fixing units and tests due to new relative tolerance

4272747

tmilnthorp commented Jun 21, 2018

View reviewed changes

Removing default parameters for windows runtime component

877db67

angularsen reviewed Jun 21, 2018

View reviewed changes

tmilnthorp added 7 commits June 21, 2018 14:54

Fixing therm(UK) values

35a8d4c

Relative difference calculation should be against expected value. Rep…

74c3faa

…lacing manual percentage calculation with format strings.

Removing power density prefixes for another day

9fe2dbc

Adding documentation and parameter checks

2ac0600

Allow tolerance to be 0 (exact)

b2898c7

Make sure comparison is done to the current units of [this] so scalin…

882a4d7

…g isn't done when not using base units. [this] will be the reference for relative units.

Windows runtime component haunts me :). Fix for now allowing a parame…

490e5af

…ter with name "value"

angularsen reviewed Jun 21, 2018

View reviewed changes

angularsen reviewed Jun 22, 2018

View reviewed changes

2a50b72

Whitespace fix

angularsen requested changes Jun 24, 2018

View reviewed changes

tmilnthorp added 2 commits June 25, 2018 08:46

Making Comparison class static

241cabc

Updating documentation. Call EqualsAbsolute from Equals($quantityName…

06164a3

… other, $quantityName maxError)

Bit by decimal :)

94be718

angularsen requested changes Jun 25, 2018

View reviewed changes

Updating documentation to be more clear and provide examples for quan…

5c5db3c

…tity types

angularsen requested changes Jun 26, 2018

View reviewed changes

tmilnthorp added 5 commits June 26, 2018 09:46

Updating documentation for clarity

62e3424

Fix for compiler warning

2071848

Obsolete Equals($quantityName other, $quantityName maxError). Fix Equ…

760f17f

…als methods that were scaling to base units when unnecessary. Whitespace fixes.

Merge remote-tracking branch 'upstream/master' into RelativeAndAbsolu…

fb04723

…teEquality

Fix for WRC when value is always double.

d35958c

angularsen merged commit 0702d1c into angularsen:master Jun 26, 2018


		protected override double YearsInOneSecond => 3.1689e-8;
		protected override double YearsInOneSecond => 3.170979198376458e-8;

Relative and absolute equality #451

Relative and absolute equality #451

Conversation

tmilnthorp commented Jun 21, 2018

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

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

tmilnthorp Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

angularsen Jun 21, 2018 • edited Loading

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

Choose a reason for hiding this comment

angularsen left a comment • edited Loading

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

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

tmilnthorp commented Jun 25, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmilnthorp commented Jun 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmilnthorp commented Jun 26, 2018

angularsen commented Jun 26, 2018

tmilnthorp Jun 21, 2018 •

edited

Loading

angularsen Jun 21, 2018 •

edited

Loading

angularsen left a comment •

edited

Loading