Skip to content

✨ Add Equals() to IQuantity interfaces #1215

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

Merged
merged 4 commits into from
May 26, 2023
Merged

Conversation

angularsen
Copy link
Owner

@angularsen angularsen commented Feb 26, 2023

Ref #1193

In v5, the default equality implementation changed to strict equality and the existing methods to compare across units with a tolerance, but this was not available in IQuantity interfaces.

Changes

  • Add Equals(IQuantity? other, IQuantity tolerance) to IQuantity
  • Add Equals(TSelf? other, TSelf tolerance) to IQuantity<TSelf, TUnitType, TValueType> for strongly typed comparisons
  • Obsolete Equals(TQuantity other, double tolerance, ComparisonType comparisonType) method in quantity types

@angularsen angularsen changed the title ✨ Add IEquatableQuantity ✨ Add IQuantity.Equals() and IEquatableQuantity Feb 26, 2023
@angularsen angularsen changed the title ✨ Add IQuantity.Equals() and IEquatableQuantity ✨ Add Equals() to IQuantity interfaces Feb 26, 2023
@codecov
Copy link

codecov bot commented Feb 26, 2023

Codecov Report

Patch coverage has no change and project coverage change: -2 ⚠️

Comparison is base (5ecf8e9) 85% compared to head (4cdc1ba) 84%.

❗ Current head 4cdc1ba differs from pull request most recent head c7c0dfa. Consider uploading reports for the commit c7c0dfa to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #1215    +/-   ##
=======================================
- Coverage      85%     84%    -2%     
=======================================
  Files         328     328            
  Lines       33513   34227   +714     
=======================================
+ Hits        28800   28805     +5     
- Misses       4713    5422   +709     
Impacted Files Coverage Δ
UnitsNet/Comparison.cs 66% <0%> (ø)
...nitsNet/GeneratedCode/Quantities/Acceleration.g.cs 85% <0%> (-2%) ⬇️
...et/GeneratedCode/Quantities/AmountOfSubstance.g.cs 85% <0%> (-2%) ⬇️
...tsNet/GeneratedCode/Quantities/AmplitudeRatio.g.cs 77% <0%> (-3%) ⬇️
UnitsNet/GeneratedCode/Quantities/Angle.g.cs 85% <0%> (-2%) ⬇️
...tsNet/GeneratedCode/Quantities/ApparentEnergy.g.cs 76% <0%> (-3%) ⬇️
...itsNet/GeneratedCode/Quantities/ApparentPower.g.cs 79% <0%> (-3%) ⬇️
UnitsNet/GeneratedCode/Quantities/Area.g.cs 87% <0%> (-2%) ⬇️
UnitsNet/GeneratedCode/Quantities/AreaDensity.g.cs 77% <0%> (-3%) ⬇️
.../GeneratedCode/Quantities/AreaMomentOfInertia.g.cs 79% <0%> (-3%) ⬇️
... and 110 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@angularsen
Copy link
Owner Author

@tmilnthorp Would like your eyes on this whenever you have time.

@angularsen angularsen force-pushed the agl/equality-interface branch from f75cc0c to 72fa8af Compare March 4, 2023 12:46
@tmilnthorp
Copy link
Collaborator

@tmilnthorp Would like your eyes on this whenever you have time.

I have the same comment as before

@angularsen
Copy link
Owner Author

@tmilnthorp Would like your eyes on this whenever you have time.

I have the same comment as before

I must have missed it, what comment?

@tmilnthorp
Copy link
Collaborator

@angularsen:

Perhaps instead of using the Comparison parameter, we could have a single method:

bool Equals(TQuantity other, TQuantity tolerance);

Or if you want asymmetric bounds:

bool IsBetween(TQuantity other, TQuantity min, TQuantity max);

This gives a lot of flexibility to use the same method for handling both situations (as unit test frameworks do):

  • Use as-is for absolute tolerance
  • Multiply the expected result by a scaling factor for relative tolerance

This also lets us remove the Comparison enum entirely for the future.

@dmytro-gokun
Copy link

@angularsen What did you decide to do about this PR? Is it going to be merged at some moment or?

@angularsen
Copy link
Owner Author

This slipped off my radar honestly.

@tmilnthorp Hm, yes, I like this contract better. Providing the error margin as a quantity.

IsBetween could be an extension method, but it is just a minor convenience to an if-statement.

So, I propose:

  • Add bool Equals(TQuantity other, TQuantity tolerance) to IQuantity and implementations
  • Mark existing Equals(IQuantity, double, ComparisonType) methods as obsolete
  • Optional: Add extension method IsBetween.

@dmytro-gokun How does this work for you?

@tmilnthorp
Copy link
Collaborator

@angularsen sounds good to me. Users can also use min < value < max.

@dmytro-gokun
Copy link

@angularsen I just need this method (Equals/IsBetween/whatever) be on a some sort of a generic interface, so that it is possible to call in generic code. The rest is not important for my scenario

Ref #1193

In v5, equality changed to strict equality.
This meant it became more cumbersome to compare equality for `IQuantity` objects of different units, but similar value when converted to the same unit.

- Add interface `IEquatableQuantity` to help compare `IQuantity` objects with a tolerance for error

Merge IEquatableQuantity into IQuantity<TSelf, Unit, ValueType> interface
@angularsen angularsen force-pushed the agl/equality-interface branch from 72fa8af to 1e8c9c7 Compare May 26, 2023 23:13
@angularsen angularsen enabled auto-merge (squash) May 26, 2023 23:33
@angularsen angularsen merged commit ce42aa8 into master May 26, 2023
@angularsen angularsen deleted the agl/equality-interface branch May 26, 2023 23:37
@angularsen
Copy link
Owner Author

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

Successfully merging this pull request may close these issues.

3 participants