Key polynomial long division by the power and not by how it was written (#751)

Sources/AngouriMath/Functions/TreeAnalyzer/LongDivision.cs

@@ -56,6 +56,34 @@ internal static PolynomialInformation GatherAllPossiblePolynomials(Entity expr,
             return res;
         }
 
+        /// <summary>
+        /// A power in the one form that equal powers share. <see cref="EDecimal"/> keeps the
+        /// scale it was written or computed at, and is equal only to an <see cref="EDecimal"/>
+        /// of that same scale -- so <c>2</c> and <c>2.0</c> are the same number and different
+        /// dictionary keys. Removing the trailing zeros is what makes them one key.
+        /// </summary>
+        /// <remarks>
+        /// The unlimited context, so that reducing a power never rounds it. Rounding here
+        /// would merge two powers that are genuinely different, which is the opposite
+        /// mistake and a worse one.
+        /// </remarks>
+        private static EDecimal Canonical(EDecimal power) => power.Reduce(EContext.Unlimited);
+
+        private static Dictionary<EDecimal, Entity> Canonicalise(Dictionary<EDecimal, Entity> powers)
+        {
+            var canonical = new Dictionary<EDecimal, Entity>();
+            foreach (var pair in powers)
+            {
+                var power = Canonical(pair.Key);
+                // Two powers of the source may reduce to the same one, and then they are one
+                // monomial and their coefficients add. Overwriting instead would silently
+                // drop a term.
+                canonical[power] = canonical.TryGetValue(power, out var already)
+                    ? already + pair.Value : pair.Value;
+            }
+            return canonical;
+        }
+
         /// <summary>
         /// Divides one polynomial over another one:
         /// <a href="https://en.wikipedia.org/wiki/Polynomial_long_division"/>
@@ -83,10 +111,19 @@ internal static (Entity Divided, Entity Remainder)? PolynomialLongDivision(Entit
             var polyvar = monoinfoP.Keys.FirstOrDefault(monoinfoQ.ContainsKey);
             // cannot divide, return unchanged
             if (polyvar is null) return null;
-            var maxpowP = monoinfoP[polyvar].Keys.Max() ?? throw new AngouriBugException("No null expected");
-            var maxpowQ = monoinfoQ[polyvar].Keys.Max() ?? throw new AngouriBugException("No null expected");
-            var maxvalP = monoinfoP[polyvar][maxpowP];
-            var maxvalQ = monoinfoQ[polyvar][maxpowQ];
+
+            // The powers are dictionary keys, and EDecimal is equal only to an EDecimal of the
+            // same scale: 2 and 2.0 are the same number and not the same key. Every power
+            // arrived at by arithmetic below carries the scale of that arithmetic, so
+            // 1.5 + 0.5 comes out as 2.0 and misses the 2 already in the dictionary. Reduced
+            // once, here, so that every key in play is in the one canonical form and the
+            // lookups below mean what they say. https://github.com/asc-community/AngouriMath/issues/751
+            var powersOfP = Canonicalise(monoinfoP[polyvar]);
+            var powersOfQ = Canonicalise(monoinfoQ[polyvar]);
+            var maxpowP = powersOfP.Keys.Max() ?? throw new AngouriBugException("No null expected");
+            var maxpowQ = powersOfQ.Keys.Max() ?? throw new AngouriBugException("No null expected");
+            var maxvalP = powersOfP[maxpowP];
+            var maxvalQ = powersOfQ[maxpowQ];
 
             // TODO: add case where all powers are non-positive
             // for now just return polynomials unchanged
@@ -97,30 +134,29 @@ internal static (Entity Divided, Entity Remainder)? PolynomialLongDivision(Entit
             while (maxpowP.GreaterThanOrEquals(maxpowQ))
             {
                 // KeyPair is ax^n with Key=n, Value=a
-                var deltapow = maxpowP - maxpowQ;
+                var deltapow = Canonical(maxpowP - maxpowQ);
                 var deltamul = maxvalP / maxvalQ;
                 result[deltapow] = deltamul;
 
-                foreach (var n in monoinfoQ[polyvar])
+                foreach (var n in powersOfQ)
                 {
-                    // TODO: precision loss can happen here. MUST be fixed somehow
-                    var newpow = deltapow + n.Key;
-                    if (monoinfoP[polyvar].TryGetValue(newpow, out var existing))
-                        monoinfoP[polyvar][newpow] = existing - deltamul * n.Value;
+                    var newpow = Canonical(deltapow + n.Key);
+                    if (powersOfP.TryGetValue(newpow, out var existing))
+                        powersOfP[newpow] = existing - deltamul * n.Value;
                     else
-                        monoinfoP[polyvar][newpow] = -deltamul * n.Value;
+                        powersOfP[newpow] = -deltamul * n.Value;
                 }
-                _ = monoinfoP[polyvar].Remove(maxpowP);
-                if (monoinfoP[polyvar].Count == 0)
+                _ = powersOfP.Remove(maxpowP);
+                if (powersOfP.Count == 0)
                     break;
 
-                maxpowP = monoinfoP[polyvar].Keys.Max() ?? throw new AngouriBugException("No null expected");
-                maxvalP = monoinfoP[polyvar][maxpowP];
+                maxpowP = powersOfP.Keys.Max() ?? throw new AngouriBugException("No null expected");
+                maxvalP = powersOfP[maxpowP];
             }
 
             // check if all left in P is zero. If something left, division is impossible => return P / Q
             Entity rest = 0;
-            foreach (var coef in monoinfoP[polyvar])
+            foreach (var coef in powersOfP)
                 if (coef.Value.Simplify() is not Integer(0) and var simplified)
                     if (coef.Key.IsZero) // Don't insert unnecessary x^0 because it's undefined for x=0
                         rest += simplified;

Sources/Tests/UnitTests/Common/LongDivisionPowerKeysTest.cs

@@ -0,0 +1,112 @@
+//
+// Copyright (c) 2019-2022 Angouri.
+// AngouriMath is licensed under MIT.
+// Details: https://github.com/asc-community/AngouriMath/blob/master/LICENSE.md.
+// Website: https://am.angouri.org.
+//
+
+using System.Linq;
+using AngouriMath;
+using AngouriMath.Extensions;
+using Xunit;
+
+namespace AngouriMath.Tests.Common
+{
+    /// <summary>
+    /// Polynomial long division keeps its monomials in a dictionary keyed by the power, and
+    /// the power is an <c>EDecimal</c>, which is equal only to an <c>EDecimal</c> of the same
+    /// scale -- so <c>2</c> and <c>2.0</c> are the same number and different keys. Every power
+    /// the algorithm computes carries the scale of that arithmetic, so dividing <c>a^2</c> by
+    /// <c>a^(1/2)</c> looked for <c>1.5 + 0.5 = 2.0</c> and missed the <c>2</c> already there.
+    /// The term it should have cancelled was added as a second entry instead, the loop ran a
+    /// second time over it, and the quotient came back **negated**:
+    /// <c>2 * a / sqrt(a)</c> simplified to <c>-2 * sqrt(a)</c>.
+    /// https://github.com/asc-community/AngouriMath/issues/751
+    /// </summary>
+    public sealed class LongDivisionPowerKeysTest
+    {
+        /// <summary>
+        /// Checked by value and not by printed form: the magnitude was right all along and
+        /// only the sign was wrong, so a test that compared shapes could have passed.
+        /// </summary>
+        private static void AssertSimplifiesToSameValueAt(string expr, string variable, string point)
+        {
+            var original = expr.ToEntity();
+            var simplified = original.Simplify();
+            Entity At(Entity e)
+            {
+                var substituted = e.Substitute(variable, point.ToEntity());
+                while (substituted is Entity.Providedf(var inner, _)) substituted = inner;
+                return substituted;
+            }
+            Assert.Equal(
+                At(original).EvalNumerical().RealPart.EDecimal.ToDouble(),
+                At(simplified).EvalNumerical().RealPart.EDecimal.ToDouble(),
+                9);
+        }
+
+        [Theory]
+        [InlineData("a ^ 2 / sqrt(a)")]
+        [InlineData("2 * a / sqrt(a)")]
+        [InlineData("a * 2 / a ^ (1/2)")]
+        [InlineData("a * 3 / a ^ (1/2)")]
+        [InlineData("a * (-2) / a ^ (1/2)")]
+        [InlineData("a / sqrt(a)")]
+        [InlineData("a ^ 3 * 2 / a ^ (1/2)")]
+        [InlineData("a ^ 2 / a ^ (1/3)")]
+        [InlineData("a * 2 / a ^ (1/4)")]
+        [InlineData("a ^ (5/2) / a ^ (1/2)")]
+        public void AQuotientLeavingAFractionalPowerKeepsItsValue(string expr) =>
+            AssertSimplifiesToSameValueAt(expr, "a", "17/10");
+
+        /// <summary>
+        /// The answers, stated outright, since "same value" would also be satisfied by leaving
+        /// the quotient alone -- and before this the fractional cases were not left alone, they
+        /// were answered wrongly.
+        /// </summary>
+        [Theory]
+        [InlineData("a ^ 2 / sqrt(a)", "a ^ (3/2)")]
+        [InlineData("2 * a / sqrt(a)", "2 * sqrt(a)")]
+        [InlineData("a / sqrt(a)", "sqrt(a)")]
+        [InlineData("a ^ 2 / a ^ (1/3)", "a ^ (5/3)")]
+        public void TheQuotientIsAnsweredAndNotMerelyLeftAlone(string expr, string expected)
+        {
+            var simplified = expr.ToEntity().Simplify();
+            while (simplified is Entity.Providedf(var inner, _)) simplified = inner;
+            Assert.Equal(Entity.Number.Integer.Create(0),
+                (simplified - expected.ToEntity()).Simplify());
+        }
+
+        // What must not change: an integer remainder was always right, because 2 - 1 and 1
+        // land on the same scale and so on the same key.
+        [Theory]
+        [InlineData("x ^ 2 / x", "x")]
+        [InlineData("(x ^ 2 - 1) / (x - 1)", "x + 1")]
+        [InlineData("(x ^ 3 - 1) / (x - 1)", "x ^ 2 + x + 1")]
+        [InlineData("(x ^ 4 - 1) / (x ^ 2 - 1)", "x ^ 2 + 1")]
+        [InlineData("(x ^ 3 - 6 * x ^ 2 + 11 * x - 6) / (x - 1)", "x ^ 2 - 5 * x + 6")]
+        public void AnIntegerRemainderIsUnaffected(string expr, string expected)
+        {
+            var simplified = expr.ToEntity().Simplify();
+            while (simplified is Entity.Providedf(var inner, _)) simplified = inner;
+            Assert.Equal(Entity.Number.Integer.Create(0),
+                (simplified - expected.ToEntity()).Simplify());
+        }
+
+        /// <summary>
+        /// The property the whole class fails, and the one that found it: a simplification has
+        /// the value of the expression it came from. Swept over a spread of quotients rather
+        /// than the reported one, since the reported one was the only member of its family
+        /// anybody had written down.
+        /// </summary>
+        [Fact]
+        public void NoQuotientOfPowersChangesValueUnderSimplification()
+        {
+            string[] numerators = { "a", "a ^ 2", "a ^ 3", "2 * a", "3 * a ^ 2", "a * 2" };
+            string[] denominators = { "sqrt(a)", "a ^ (1/2)", "a ^ (1/3)", "a ^ (2/3)", "a", "a ^ 2" };
+            foreach (var numerator in numerators)
+                foreach (var denominator in denominators)
+                    AssertSimplifiesToSameValueAt($"({numerator}) / ({denominator})", "a", "17/10");
+        }
+    }
+}