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

Special values (NaN, Inf+, Inf-) for doubles #1268

Closed
ebfortin opened this issue Jun 18, 2023 · 8 comments
Closed

Special values (NaN, Inf+, Inf-) for doubles #1268

ebfortin opened this issue Jun 18, 2023 · 8 comments

Comments

@ebfortin
Copy link
Contributor

What is the rational behind throwing AwgumentException when the underlying double value for a unit is NaN, Inf+ or Inf-? With double we can sometimes use these special values to mean something. Inf+ or Inf- for example are "valid" in certain context. Shouldn't we just pass them along?

@angularsen
Copy link
Owner

Hi, I guess the rationale is consistency. 3 quantities (Power, Information, BitRate) use decimal as the value type, all other 100+ quantities use double. Since decimal does not support NaN/InF values, I wanted to avoid subtle bugs where some quantities throw and others happily accept these values.

The reason for Power was precision issues in earlier implementations, later fixed.
There is a PR to change Power back to double here: #1195

Information/BitRate was chosen to use decimal, because their smallest representation is 1 Bit so it kind of makes sense to have fixed precision and avoid rounding errors due to floating point arithmetic. It definitely could be changed to double, but a lot of effort has been made to support decimal to begin with, so a bit on the fence to throw all that out.

Some discussion here on Info/BitRate:
#1195 (comment)

So now what? A few ideas.

Option 1) Change Power to double, keep Info/BitRate as decimal, support NaN/InF for quantities with double value.
Option 2) Make everything double and support NaN/InF.

@angularsen angularsen mentioned this issue Jul 11, 2023
7 tasks
@ebfortin
Copy link
Contributor Author

I knew there was a good reason for not supporting special cases. I just couldn't figure it all by myself. Thanks for the explanation.

I would add another option to your two : go with generic maths and so give the possibility to use any value type as the underlying type holding the value.

We've talked about it some months / years ago here. At the time generic math was experimental. Now it's there in the framework. Also there wasn't a clear use case to put the time and effort. This could be one. Moreso that having the possibility to go with float, for example, would support use cases where precision is secondary to storage space taken. Someone could even go wild and use Half for scenarios that make sense, all with having the huge benefits of using real units.

For sure this would be a longer term option. For the short term I would go with Option 1.

@angularsen
Copy link
Owner

angularsen commented Jul 12, 2023

Generics may be a way forward, but we have tried it out a little bit and I find it a bit complicated to work with.

I think it's an interesting path, but someone needs to put in time and effort to experiment and figure out how it will all fit together. I don't have the time myself.

A few observations so far:

  1. We do support generic math in Generic math for UnitsNet in .NET 7 #1164 with new interfaces like IArithmeticQuantity
  2. As a proof of concept we implemented GenericMathExtensions.Sum<T>() and GenericMathExtensions.Average<T>.
  3. I was not able to create a truly generic Average() for both double and decimal, the compiler would complain about ambiguity, so I wound up with DecimalGenericMathExtensions.Average<T>. It doesn't mean it can't be done, but I could not figure it out.
  4. Quantities are not numbers, they are numbers + units, so we can't implement INumber<>, but we can implement some individual generic math interfaces like IAdditionOperators, IMultiplyOperators, IAdditiveIdentity.
  5. Units can trip up things like Average() using IAdditiveIdentity as the starting value, which typically maps to Length.Zero. Since addition picks the left hand side unit, the addition argument order must be so that the accumulated value is on the right hand side, to avoid getting the base unit instead of the first item's unit.
  6. Some quantities have units of different scale (Fahrenheit vs Celsius) and different zero level (Kelvin and Celsius), so you can't for example perform addition of two Temperature quantities, because 10 Celsius + 5 Celsius would in some contexts mean 15 Celsius, but it could also mean 283.15 Kelvin + 283.15 Kelvin. The operation is ambiguous. We introduced TemperatureDelta to make it more clear, but this is not compatible with generic math.

@angularsen
Copy link
Owner

Updated the wiki page on this topic, a place to describe the state of this experiment and known challenges.

https://github.com/angularsen/UnitsNet/wiki/Experimental:-Generic-Math-and-INumber

@angularsen
Copy link
Owner

If you want to help pursue option 1, you can help move #1195 forward.

Then create a separate PR for supporting NaN/InF for quantities with double.
I believe QuantityValue must be modified to not throw, and maybe some other places.

@senften
Copy link

senften commented Aug 15, 2023

supporting NaN, Inf+ or Inf-? would simplify things for us considerably (see Infinity error - Is it really that bad? · Issue #586 · angularsen/UnitsNet for earlier comments on this topic)

Additionally, the recommended use of a Nullable<> has run into further issues with regard to the use of IQuantity as a generic and it being nullable while the subclasses like Pressure are not so we have an issue generalizing some of our classes.

For instance

public interface ICurve<out T> where T: IQuantity
{
   T?[] GetValues(IReadOnlyList<DateTime> xvaluesToQuery);
}

public interface PressureCurve : ICurve<Pressure> {}

since IQuantity is nullable, the Nullable<> notation is considered redundant and the implementation of Pressure curve returns a Pressure[], not a Pressure?[] and so the previous suggestion of using a nullable to represent a NaN fails.

@angularsen
Copy link
Owner

Related work in progress:
#1289

@angularsen
Copy link
Owner

Fixed in v6 branch by #1289

#1200

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