Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Request: Implement IEquatable #299

Closed
steingran opened this issue Oct 17, 2017 · 9 comments
Closed

Request: Implement IEquatable #299

steingran opened this issue Oct 17, 2017 · 9 comments

Comments

@steingran
Copy link

Could classes like Force and Torque implement IEquatable, in addition to IComparable which is already implemented? This will make life easier when verifying that two Force or Torque objects are equal in automated tests.

Example: Can then write: Assert.AreEqual(force1, force2);
Instead of: Assert.AreEqual(0, force1.CompareTo(force2)); or Assert.IsTrue(force1.CompareTo(force2) == 0);

@angularsen
Copy link
Owner

angularsen commented Oct 17, 2017

I intentionally left out IEquatable, because the underlying representation is double and it's too easy to shoot oneself in the foot when comparing floating point values. In addition, the fact that this is a floating point value comparison is kind of hidden to the consumer.

Now that I browse the code, I see we do have an override Equals(object) method, which do compare the double values, so how about this:

  • Let's [Obsolete] and later remove the existing Equals(object) method, it is not safe in my opinion and we should not encourage using it (you can still do it on the double Meters property value yourself if you really want to)
  • Also [Obsolete] the operator overloads == and !=, but we can probably keep <= and >= although they do kind of bring the same risk
  • Add a bool Equals(Length other, Length maxDelta) method so that the consumer becomes aware that this is something he needs to consider when equating two quantities

To be clear, we would still not implement the IEquatable interface in this proposal.
I thought of adding a sane default value for the maxDelta, but really we have such large variation in the scale of units (nanogram vs megatonne) that I'm not sure how safe that would be either.

Just an example of the problem:

Console.WriteLine(Length.FromMeters(1.39).Equals(Length.FromMeters(1.39))); // true
Console.WriteLine(Length.FromMeters(1.39).Equals(Length.FromMeters(1 + 0.39))); // false

cc @eriove @JKSnd

@angularsen
Copy link
Owner

angularsen commented Oct 17, 2017

On a side-note, we have discussed adding an alternative version of the nuget using decimal as representation type. This is fixed-point and should be safe to use with IEqutabable and the equality checks discussed.

@eriove
Copy link
Contributor

eriove commented Oct 17, 2017

We are always using Assert.InRange() when verifying results on UnitsNet types. That way the error messages gets meaningful. With bool Equals(Length other, Length maxDelta) the error message from the assertion would still be meaningless and you would have to run the test in the debugger. By using InRange() the likelihood of understanding what's wrong without debugging the test is much higher.

@steingran
Copy link
Author

Thanks for the extensive reply :-) Then I know why IEquatable is not implemented.
Great idea to use InRange()

@angularsen
Copy link
Owner

angularsen commented Oct 17, 2017

@eriove Ah, yes, I was actually not thinking in the context of unit tests. For those cases I agree a InRange type of assert is better, but I was thinking of whether we should provide a method to compare (approximate) equality of quantities outside the context of testing?

@eriove
Copy link
Contributor

eriove commented Oct 17, 2017

@angularsen Then it makes sense, I would personally prefer if the delta was a relative difference instead, but I'm guessing that is dependent of the application. Since the relative difference would be a double and the absolute difference would be a quantity it would be possible to add both.

How useful is it outside testing though? I'm currently working on libraries without any GUI so I'm very focused on unit test as use cases :-)

@angularsen
Copy link
Owner

I don't know if this is in demand and whether obsoleting the existing equality methods will cause a furious uproar, I haven't used the equality myself in any cases I can think of. Maybe we should start by obsoleting them and then see if someone actually comes around to ask about it?

@angularsen
Copy link
Owner

angularsen commented Nov 9, 2017

Equals(T other), == and != are now obsoleted in 471d2fc for quantities with double backing type. Only Power and Information uses decimal backing type today.
Added a new Equals(T other, T maxError) that can be used instead.

@angularsen
Copy link
Owner

IEquatable is being added back in v5, with strict equality on Value + Unit.
#1100

We still recommend using the Equals overloads that provide arguments to specify absolute or relative error tolerance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants