Optimized the CompareTo and Equals methods (FractionValueEqualityComparer) #71
Conversation
…arer). Updated the benchmark results and the Readme.md (for the benchmarks)
|
Here are the absolute differences (in nanoseconds) for the
And here how we stack-up against the
|
|
And here are the results of the
And here are the absolute results- here I'm going to give the
Now, typically this isn't suppose to be like that- and for most scenarios it would probably be faster to just |
|
Thank you for taking the time to optimize the two methods. I don't have much free time, so I've only planned small time windows for code reviews. Unfortunately, even after the second attempt, I was unable to understand the algorithms implemented in the pull request. Admittedly, that could also be due to my advancing age or my lack of love for mathematics in general ;-) Did you get this from a book - something where I can find out the mathematical correctness? I don't want to rely on unit tests alone here. I still find the current code easy to understand (also taking into account the special treatment for NaN/Infinity). I'm not sure the performance gains (which are in the nanosecond range) are worth it. |
|
I need to go out in 5 minutes so I'll try to make it quick (will elaborate later if necessary):
|
| var numeratorQuotient = BigInteger.DivRem(numerator1, numerator2, out var remainderNumerators); | ||
| var denominatorQuotient = BigInteger.DivRem(denominator1, denominator2, out var remainderDenominators); | ||
| // if the fractions are equal: numeratorQuotient should be equal to denominatorQuotient | ||
| if (numeratorQuotient != denominatorQuotient) { |
There was a problem hiding this comment.
This is the most interesting part:
What we're doing is taking a roundabout way of comparing the fractions by checking if their ratio is exactly 1 :
A / B == 1 -> A == B
Expressing this using the 4 terms gives us {n1, d1} / {n2, d2} = {(n1 * d2) / (d1 * n2)} = {n1 / n2} * {d2 / d1}
When we set {n1 / n2} * {d2 / d1} = 1 (and having previously ensured that the terms are non-negative)
It follows that {d1 / d2} = {n1 / n2} should be true (in order for A to be equal to B)
From here on we do the standard DivRem comparison (split into separate operations here, for simplicity):
{d1 / d2} = {n1 / n2} =>
BigInteger.Divide(d1, d2) == BigInteger.Divide(n1, n2)// the quotients should be equal(d1 % d2) / d2 == (n1 % n2) / n2// the remainders (r1, r2) expressed as a ratio (Fraction) of their respective dividend should be equal
If all conditions along the way are satisfied, we still end up comparing the product of 2 terms, but for most other cases we exit early.
Furthermore, the reason for selecting this particular set of operations is not accidental (it would have been simpler to just compare {n1/d1} and {n2/d2}): the reason is that if the two fractions are actually equal, then the result of dividing the two fractions would be something like {10/10} or {200/200} (but never {1/1} since d1>d2). In any way, this value is almost certainly not huge and the final 2 products (remainderNumerators * denominator2 and remainderDenominators * numerator2) would have values that are smaller that the original cross-term multiplication (due to remainderDenominators < denominator2 < denominator1).
There was a problem hiding this comment.
{(n1 * d2) / (d1 * n2)} = {n1 / n2} * {d2 / d1}
Ah, after reproducing the calculation on paper (or unforming the formula accordingly), I see that this equation is correct. Thanks.
There was a problem hiding this comment.
FYI - the code reviews take longer because I can't always understand your thought process based on the code (especially when mathematical equations need to be transformed).
| } | ||
|
|
||
| // comparing the positive term ratios: | ||
| // {9/7} / {4/3} = {(2 + 1/4) / (2 + 1/3)} = {(9/4)/(7/3)} = {27/28} => (2).CompareTo(2) == 0 and (1/4).CompareTo(1/3) == -1 |
There was a problem hiding this comment.
Can you please explain how this comment should be understood?
That's what I would see here:
{9/7} / {4/3} = (1 + 2/7) / (1 + 1/3) => (1).CompareTo(1) == 0 AND (2/7).CompareTo(1/3) == -1
There was a problem hiding this comment.
I'm doing the same roundabout comparison again- dividing the two fractions (like with the Equals) and then checking if their ratio is larger or smaller than {1/1}.
Another way of thinking about these divisions is to imagine that the terms represent decimals: {9.0 / 7.0} / {4.0 / 3.0} = {(9.0/4.0) / (7.0/3.0) = {2.25 / 2.33} . The part preceding the decimal separator (2) is the quotient, while the rest is the remainder divided by the dividend (e.g. 1/4 adjusted to denominator of 100 is 25/100).
|
I'm done with the code review (algorithm understood). For testing, I added the previous CompareTo method (V1) and a variation for an A/B test. See pull-request: https://github.com/danm-de/Fractions/pull/73/files#diff-b9514275b55edffc0f210fa857e59e0f1d706c067c44164fdd805e86fe77c498 The benchmark results on my computer (Intel Core i7-8650U) are... difficult to interpret. Actually, they are not clear to me - I cannot tell which algorithm is the "winner". I also feel that the benchmark must not only focus on the edge cases (or "extreme values"). Which numbers are likely? And what magnitude are these numbers? Fractions.Benchmarks.NonNormalizedComparisonBenchmarks-report.csv Fractions.Benchmarks.NonNormalizedComparisonBenchmarks-report-github.md |
Yes, the *ComparisonBenchmarks have become a little overwhelming to run / read though.. I'll post a chart with your results in a bit.. While preparing the charts, I thought I'd run the benchmark with only these methods: Here are the results:
And here are the charts with your results:
For the differences I've merged the two runs- here we're seeing the summed-up difference in nanoseconds between
And here is
|
Looking at the results I see just one or two values for which
I'm not sure there is a straightforward answer here- if you ask me what is the most common values that would be compared, from the numbers we have - I'd say something like Now let's consider the remaining cases (I'm of course discounting the
From the opposite point of view, Finally, if we're only working with the |
Ok, clearly my agent has skipped some math classes- but I think the reasoning still holds.. 😄 |
|
Even if the battle is “only” for a few nanoseconds: I would now merge the pull request into the master branch. I'll be honest - the algorithm is not easy to understand right away and requires explanation. I hope my added code comments will be helpful in some way to future-danm and future-lipchev 🥲 |
CompareToandEqualsmethods (FractionValueEqualityComparer)Readme.md(for the benchmarks)