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

Formulas in which we divide by x throws an exception when x = 0 #707

Closed
aidbal opened this issue Sep 24, 2019 · 4 comments
Closed

Formulas in which we divide by x throws an exception when x = 0 #707

aidbal opened this issue Sep 24, 2019 · 4 comments
Labels

Comments

@aidbal
Copy link
Contributor

aidbal commented Sep 24, 2019

Hey @angularsen, I have recently published a pr to create a new Fuel Efficiency unit (#703).

In one of the formulas, there is a division by zero. It happens, that sometimes in our code we have the following call FuelEfficiency.ToUnit(FuelEfficiencyUnit) which throws an exception.

After inspecting the generated code, I have found that nowhere it checks for division by zero, thus the formula 100/x becomes 100/0 and it throws an exception.

I am not sure how propose fixes for this problem, that's why I am writing it here. Right now, I have created an extension method, which first, checks for initial value, and then, if it's non-zero, it calls it's base method.

One more thing, you cannot apply check by zero to all values, because 0 Celsius is not 0 Kelvins and vice versa.

@aidbal aidbal added the bug label Sep 24, 2019
@angularsen
Copy link
Owner

Hi, that sounds about right. There are no zero checks, because that would be hard to do in a generic manner for all the different functions.

So I tried this:

// ArgumentException: PositiveInfinity or NegativeInfinity is not a valid number.Parameter name: value
FuelEfficiency.FromLitersPer100Kilometers(0).ToUnit(FuelEfficiencyUnit.KilometerPerLiter)

// OK
FuelEfficiency.FromLitersPer100Kilometers(1).ToUnit(FuelEfficiencyUnit.KilometerPerLiter)

Isn't this kind of the correct behavior, though?

Zero liters per 100 kilometers is infinite kilometers per liter. Did you perhaps expect the .Value property to be double.PositiveInfinity? If so, that is by design unfortunately #502.

For debatable reasons, a handful of quantities use decimal backing value type to support conversions between very small and very large units. That numeric type does not support NaN or Infinity, so we decided to not support those in any quantities for consistency.

We do have some ideas on how to fix this with generics #666 and allowing the consumer to choose between float, double and decimal, but no one is actively working on it yet. We could also consider allowing Infinity and NaN for the majority of quantities that use double, but I think I would rather invest time towards generics maybe.

Did that answer your question?

@aidbal
Copy link
Contributor Author

aidbal commented Sep 25, 2019

Actually you are quite right. 0 liters/100km means infinitely kilometers/liter. But I think that depends on our agreement. In practice, we have a parameter for such a type. And if it's unset, it has value of 0. So no matter what measurement system you use, it should be 0. That means 0 L/100km should be 0 km/L.

What are your ideas on this? Maybe it's just our business logic. And if so, as I said, we have adapted to it through extension methods.

@angularsen
Copy link
Owner

Could you take FuelEfficiency? as parameter and have an agreement that says it is null if it is unset? It seems ambiguous to let 0 mean both unset and the actual value 0.

@aidbal
Copy link
Contributor Author

aidbal commented Sep 30, 2019

Yeah sure, we will adapt it to our business needs. I opened this issue so others wouldn't encounter this problem.

@aidbal aidbal closed this as completed Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants