MadsKirkFoged / EngineeringUnits

Working with units made easy with automatic unit-check and converting between units
MIT License
42 stars 11 forks source link

Add support for NaN #34

Closed mschmitz2 closed 1 year ago

mschmitz2 commented 1 year ago

Currently, constructing with a double.NaN value (e.g. Pressure.FromSI(double.NaN)) results in a value of Infinity.

Looking through the source code I found that this is done in BaseUnit:

if (IsValueOverDecimalMax(value))
{
    Inf = true;
    return;
}

where

private static bool IsValueOverDecimalMax(double value)
{
    return double.IsInfinity(value) || value > 7.9228162514264338E+28 || value < -7.9228162514264338E+28 || double.IsNaN(value);
}

I imagine this was implemented due to internally saving the value as decimal, where decimal does not support NaN.

But from what I can see it could be possible to handle this in exactly the same way as infinity, but side by side. I.e.

if (double.IsNaN(value))
{
    IsNaN = true;
    return;
}
if (double.IsInfinity(value) || value > 7.9228162514264338E+28 || value < -7.9228162514264338E+28)
{
    Inf = true;
    return;
}

and then also checking for IsNaN when getting a value as double, everywhere you currently already check for Inf. Hopefully this would not have too much of an impact on performance.

Ideally, to avoid having to use XXX.FromSI(double.NaN), there could also be a static property .NaN similar to .Zero.

The application where this would be of help to me is for plotting, where NaN is an empty point and values of Infinity causes issues and results in exceptions. I can of course get around it in my application by transforming every Infinity value back to NaN before passing to the plotting tool, so this is not super crucial, but might be a nice addition if easy.

MadsKirkFoged commented 1 year ago

I have added support for NaN! It was just a use-case I never stumbled upon myself but I agree it should be there!

Thanks for bringing it up and for taking your time to explain it!

Can you download the newest version and test it out?

mschmitz2 commented 1 year ago

Thanks! Confirming that the main issue is resolved as expected with the new version.

Is there any reason why you did not want to implement a <unit>.NaN static property similar to <unit>.Zero? E.g.

public static Pressure Zero => new Pressure(0, PressureUnit.SI);
public static Pressure NaN => new Pressure(double.NaN, PressureUnit.SI);

so one could do e.g.

var p = someTest ? Pressure.FromBar(2) : Pressure.NaN;

instead of currently needing to do

var p = someTest ? Pressure.FromBar(2) : Pressure.FromSI(double.NaN);

Happy to open a new issue for this if you prefer.

MadsKirkFoged commented 1 year ago

I just forgot to do it :-D

It should be pushed out now.

mschmitz2 commented 1 year ago

Is it just me or did you forget to add to for Pressure (which by chance was the only one I checked) and some others as well? Easy way to spot the missed ones is here https://github.com/MadsKirkFoged/EngineeringUnits/tree/master/EngineeringUnits/CombinedUnits

image

MadsKirkFoged commented 1 year ago

You are right Pressure got left out of the Codegenerator because it is slightly different and then I forgot to manual add the NaN. The others are some I need to go though some day to check if we still use them or not.

I have added NaN for pressure

mschmitz2 commented 1 year ago

Thanks! Issue closed as far as I'm concerned.