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

Constrained the GenericMathExtensions to IQuantity #1448

Merged
merged 1 commit into from
Dec 15, 2024

Conversation

lipchev
Copy link
Collaborator

@lipchev lipchev commented Dec 10, 2024

  • replaced the aggregation with an iterator based version (slightly faster)
  • the Average extension now throws an InvalidOperationException when the source is empty

- replaced the aggregation with an iterator based version (slightly faster)
- the Average extension now throws an InvalidOperationException when the source is empty
{
IEnumerable<Length> values = [Length.FromCentimeters(100), Length.FromCentimeters(200)];

Length sumOfQuantities = GenericMathExtensions.Sum(values);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I'm not using values.Sum() is because when we add the IVectorQuantity interface, this is going to start testing the other extension method (which is a better/more specific match for the list of Length).

Comment on lines +66 to +69
if (!e.MoveNext())
{
throw new InvalidOperationException("Sequence contains no elements");
}
Copy link
Collaborator Author

@lipchev lipchev Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notice this: in v5 this case would have caused an exception to be thrown because of the NaN, however as we disabled the guard in v6- this would not have thrown an exception.
This is also the behavior of Average(IEnumerable<Double>).

@lipchev
Copy link
Collaborator Author

lipchev commented Dec 11, 2024

I think I already mentioned this in #1200 but these method are still applicable to the logarithmic quantities, which implement both IAdditionOperators<TQuantity, TQuantity, TQuantity> (rightly) and IAdditiveIdentity<TQuantity, TQuantity> (incorrectly).

        [Fact(Skip = "Fix in v6")]
        public void AdditiveIdentityIsIncorrect()
        {
            var expectedQuantity = PowerRatio.FromDecibelMilliwatts(24);
            AssertEx.EqualTolerance(expectedQuantity.DecibelMilliwatts, (expectedQuantity + PowerRatio.AdditiveIdentity).DecibelMilliwatts, DecibelMilliwattsTolerance);
        }

With the addition of the new interfaces, the logarithmic quantities are going to lose their AdditiveIdentity and become incompatible with the extensions from the GenericMath (they have their own Geomatric/Aritmethic mean extensions).

@lipchev
Copy link
Collaborator Author

lipchev commented Dec 13, 2024

Oh, and by the way- if you need to test the performance difference there are a few benchmarks available for the Sum:

@angularsen angularsen merged commit 17e3bfc into angularsen:release/v6 Dec 15, 2024
1 check failed
@lipchev lipchev deleted the generic-math-v6 branch December 16, 2024 15:17
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.

2 participants