bugfix in Rational.cs, when both Nominator and Denominator equals to 0 #2453
Conversation
|
|
| @@ -74,6 +74,11 @@ public Rational(double value, bool bestPrecision) | |||
|
|
|||
| this.Numerator = (uint)rational.Numerator; | |||
| this.Denominator = (uint)rational.Denominator; | |||
|
|
|||
| if(this.Numerator == 0 && this.Denominator == 0) | |||
There was a problem hiding this comment.
If there's an issue here it will have to be fixed in LongRational
There was a problem hiding this comment.
if you want to update GPS location of ExifInformation, it accepts Retional[]. That's why I suggest this change. Anyway if you are creating Rational.FromDouble(0) Denominator shouldn't be 0.
There was a problem hiding this comment.
Ah, you're right, the problem is deeper on line 206 of LongRational.cs it should be
return new LongRational(0, 1);
There was a problem hiding this comment.
That's the correct fix. Can you also add some unit tests for this to make sure this stays fixed?
There was a problem hiding this comment.
yes, sure will do that and will push all as a different pull request. You can close this one.
There was a problem hiding this comment.
No need to open a new PR. That’s just confusing. Push to this one please.
There was a problem hiding this comment.
And you can also use a force push to update this PR with a new set of commits.
fix issue with 0 Denominator.
| @@ -153,6 +153,10 @@ public static LongRational FromDouble(double value, bool bestPrecision) | |||
| double df = numerator / (double)denominator; | |||
| double epsilon = bestPrecision ? double.Epsilon : .000001; | |||
|
|
|||
| if(val < epsilon) { | |||
There was a problem hiding this comment.
Shouldn't the fix be the following inserted at 149?
if (value == 0)
{
return new LongRational(0, 1);
}
Prerequisites
Description
In cases, when you're trying to create Rational.FromDouble(0) it generates an invalid Rational item.