Cancel an inverse trigonometric function only on its own branch (#884)

BREAKING-CHANGES.md

@@ -64,6 +64,7 @@ read first.
 | **silent** | a `RewriteRecording` across an `await`, or work started under it | lost, or somebody else's | follows the call |
 | loud | a polynomial system with more equations than unknowns | `WrongNumberOfArgumentsException` | solved |
 | loud | a known gap, e.g. a cubic inequality | `AngouriBugException`, asking to be reported | `NotSufficientlySupportedException` |
+| **silent** | `arcsin(sin(x))` and three siblings | `x`, wrong wherever `x` leaves the principal interval | left as written unless `x` is a real in that interval |
 
 ---
 
@@ -193,6 +194,52 @@ The pairs elsewhere in the API are unaffected, because none of them has to guess
 integration `Range`, the arguments of `Substitute`, the cases of `MathS.Piecewise` and
 `ToProvided` are all ordered pairs whose two halves have distinct, stated roles.
 
+### An inverse trigonometric function no longer cancels its own function off the principal branch
+
+`arcsin` is a left inverse of `sin` only on `[-pi/2, pi/2]`, and the three siblings likewise only on
+their own intervals. The rewrite was unconditional, so `Simplify` returned a value that is wrong at
+every real point outside the interval:
+
+| | was | is | the value at that point |
+|---|---|---|---|
+| `arcsin(sin(x))` | `x` | left as written | `pi - x` for `x` in `[pi/2, 3pi/2]`, and so on |
+| `arccos(cos(x))` | `x` | left as written | `2*pi - x` for `x` in `[pi, 2pi]` |
+| `arctan(tan(x))` | `x` | left as written | `x - pi` for `x` in `(pi/2, 3pi/2)` |
+| `arccotan(cotan(x))` | `x` | left as written | `x - pi` for `x` in `(pi, 2pi)` |
+
+Measured on a build of `35f4ae5a` before this change and after it. The four rules are older than
+1.4.0 and untouched since, so 1.4.0 answers the same way, but the numbers below are from the two
+2.0.0 builds rather than from a 1.4.0 one:
+
+```
+"arcsin(sin(x))".Simplify().Substitute("x", 3).EvalNumerical()
+  was  3
+  is   0.14159...        which is pi - 3, and is what arcsin(sin(3)) equals
+
+"arctan(tan(x))".Simplify().Substitute("x", 2).EvalNumerical()
+  was  2
+  is   -1.14159...       = 2 - pi
+
+"arccos(cos(x))".Simplify().Substitute("x", 4).EvalNumerical()
+  was  4
+  is   2.28318...        = 2*pi - 4
+```
+
+**Where the argument is a real number inside the interval, the cancellation still happens and stays
+exact**: `arcsin(sin(1/2))` is `1/2`, not a decimal. Where it is symbolic, the expression is left as
+written, which is what SymPy and Mathematica both answer.
+
+A condition was deliberately *not* attached. `arcsin(sin(x))` is defined for every real `x`, so
+`x provided x >= -pi/2 and x <= pi/2` would say the expression is undefined outside the interval
+when in fact it merely has another value — trading a wrong value for a wrong domain.
+
+**The other direction is unchanged**, and needs no assumption: `sin(arcsin(z))`, `cos(arccos(z))`,
+`tan(arctan(z))` and `cotan(arccotan(z))` are all `z`, because they compose the *right* inverse.
+
+`CircleTest.Test8` asserted `arccotan(cotan(3x)) == 3x` and was pinning the wrong answer; it now
+asserts that the expression is left alone. Issue
+[#884](https://github.com/asc-community/AngouriMath/issues/884).
+
 ### A known gap no longer presents as a bug
 
 `FutureReleaseException` is removed, and the twelve places that threw through it now throw

Sources/AngouriMath/Functions/Simplification/Patterns/Patterns.Trigonometry.cs

@@ -6,12 +6,55 @@
 //
 
 using System;
+using PeterO.Numbers;
 using static AngouriMath.Entity;
 
 namespace AngouriMath.Functions
 {
     internal static partial class Patterns
     {
+        /// The real value of <paramref name="argument"/> where there is one to read. A rule
+        /// that holds only on a principal branch may fire only where the branch can be
+        /// decided, so a symbolic argument -- or one off the real line -- has to leave the
+        /// node alone.
+        private static bool TryReadReal(Entity argument, out EDecimal value)
+        {
+            value = EDecimal.Zero;
+            if (argument.Evaled is not Number.Real real || !real.EDecimal.IsFinite) return false;
+            value = real.EDecimal;
+            return true;
+        }
+
+        /// <summary>Whether the argument lies in <c>[-pi/2, pi/2]</c>, or in the open interval
+        /// when <paramref name="closed"/> is false — the intervals <c>arcsin</c> and
+        /// <c>arctan</c> answer in.</summary>
+        /// <remarks>
+        /// Tested by doubling the argument and comparing against pi rather than by halving pi,
+        /// because addition of two <see cref="EDecimal"/>s is exact and division is not. The
+        /// comparison is still only as good as the working precision, and an argument that
+        /// close to an endpoint is treated as outside the interval — which declines to
+        /// rewrite, the safe direction.
+        /// </remarks>
+        private static bool WithinHalfPi(Entity argument, bool closed)
+        {
+            if (!TryReadReal(argument, out var value)) return false;
+            var absolute = value.Abs();
+            var comparison = absolute.Add(absolute).CompareTo(MathS.DecimalConst.pi);
+            return closed ? comparison <= 0 : comparison < 0;
+        }
+
+        /// <summary>Whether the argument lies in <c>[0, pi]</c>, or in the open interval when
+        /// <paramref name="closed"/> is false — the intervals <c>arccos</c> and
+        /// <c>arccotan</c> answer in.</summary>
+        private static bool WithinZeroAndPi(Entity argument, bool closed)
+        {
+            if (!TryReadReal(argument, out var value)) return false;
+            if (value.IsNegative || (!closed && value.IsZero)) return false;
+            var comparison = value.CompareTo(MathS.DecimalConst.pi);
+            return closed ? comparison <= 0 : comparison < 0;
+        }
+
+
         internal static Entity TrigonometricRules(Entity x) => x switch
         {
             // sin({}) * cos({}) = 1/2 * sin(2{})
@@ -58,13 +101,27 @@ internal static partial class Patterns
             Mulf(Tanf(var any1), Cotanf(var any1a)) when any1 == any1a => 1,
             Mulf(Cotanf(var any1), Tanf(var any1a)) when any1 == any1a => 1,
 
-            // arcfunc(func(x)) = x
-            Arcsinf(Sinf(var any1)) => any1,
-            Arccosf(Cosf(var any1)) => any1,
-            Arctanf(Tanf(var any1)) => any1,
-            Arccotanf(Cotanf(var any1)) => any1,
-
-            // func(arcfunc(x)) = x
+            // arcfunc(func(x)) = x, but only where x already lies in the interval arcfunc
+            // answers in. Outside it the composition folds back into that interval, so
+            // arcsin(sin(3)) is pi - 3 and arccos(cos(4)) is 2*pi - 4. Applied
+            // unconditionally this returned a wrong value at ordinary real points --
+            // https://github.com/asc-community/AngouriMath/issues/884
+            //
+            // A symbolic argument is left as written, which is what SymPy and Mathematica
+            // both do, and is a legitimate answer where returning x is not. Attaching the
+            // interval as a condition instead would be a second wrong answer: the
+            // composition is defined for every real x, so saying it is undefined outside the
+            // interval trades a wrong value for a wrong domain.
+            //
+            // The two tangent-family intervals are open, because tan and cotan have no value
+            // at the endpoints and the composition has none there either.
+            Arcsinf(Sinf(var any1)) when WithinHalfPi(any1, closed: true) => any1,
+            Arccosf(Cosf(var any1)) when WithinZeroAndPi(any1, closed: true) => any1,
+            Arctanf(Tanf(var any1)) when WithinHalfPi(any1, closed: false) => any1,
+            Arccotanf(Cotanf(var any1)) when WithinZeroAndPi(any1, closed: false) => any1,
+
+            // func(arcfunc(x)) = x, and this direction needs no assumption: it composes the
+            // *right* inverse, so sin(arcsin(z)) is z wherever arcsin(z) is defined at all.
             Sinf(Arcsinf(var any1)) => any1,
             Cosf(Arccosf(var any1)) => any1,
             Tanf(Arctanf(var any1)) => any1,

Sources/Tests/UnitTests/Common/CircleTest.cs

@@ -85,8 +85,14 @@ public void DecimalToRational(string @decimal, string rational)
         [Fact]
         public void Test7() => Assert.Equal(3 * x, MathS.Sin(MathS.Arcsin(x * 3)).Simplify());
 
+        // This asserted 3 * x, which is not what arccotan(cotan(3x)) is: arccotan answers in
+        // (0, pi), so at x = 2 the expression is about 2.86 and 3 * x is 6. The test was
+        // pinning a wrong answer, and the rewrite behind it now fires only where the argument
+        // is a real number inside that interval --
+        // https://github.com/asc-community/AngouriMath/issues/884
         [Fact]
-        public void Test8() => Assert.Equal(3 * x, MathS.Arccotan(MathS.Cotan(x * 3)).Simplify());
+        public void Test8() => Assert.Equal(MathS.Arccotan(MathS.Cotan(3 * x)),
+            MathS.Arccotan(MathS.Cotan(x * 3)).Simplify());
 
         [Theory]
         [InlineData("x / y + x * x * y")]

Sources/Tests/UnitTests/Common/SimplificationRegressionTest.cs

@@ -521,5 +521,69 @@ public void AContradictoryPairOfComparisonsFailsOverTheReals(string input)
             using var _ = MathS.Settings.Codomain.Set(Domain.Real);
             Assert.Equal(Entity.Boolean.False, input.ToEntity().Simplify());
         }
+
+        // https://github.com/asc-community/AngouriMath/issues/884
+        // arcsin is a left inverse of sin only on [-pi/2, pi/2], and the three siblings
+        // likewise only on their principal intervals. The rewrite was unconditional, so
+        // arcsin(sin(3)) simplified to 3 where the value is pi - 3 -- a wrong answer at an
+        // ordinary real point, not a missing one.
+        //
+        // Asserted numerically because the point of the bug is the value: comparing the
+        // simplified form against the original at a point outside the principal interval
+        // is the only assertion that fails for the right reason.
+        [Theory]
+        [InlineData("arcsin(sin(x))", "3")]
+        [InlineData("arcsin(sin(x))", "pi")]
+        [InlineData("arcsin(sin(x))", "-2")]
+        [InlineData("arccos(cos(x))", "4")]
+        [InlineData("arccos(cos(x))", "-1")]
+        [InlineData("arctan(tan(x))", "2")]
+        [InlineData("arctan(tan(x))", "-2")]
+        [InlineData("arccotan(cotan(x))", "-1")]
+        public void SimplifyingAnInverseOfItsOwnFunctionKeepsTheValueOffThePrincipalBranch(
+            string expression, string at)
+        {
+            var original = expression.ToEntity();
+            var simplified = original.Simplify();
+            var before = original.Substitute("x", at.ToEntity()).EvalNumerical();
+            var after = simplified.Substitute("x", at.ToEntity()).EvalNumerical();
+            Assert.True(Magnitude(before - after) < 1e-20,
+                $"{expression} simplified to {simplified.Stringize()}, which at x = {at} is "
+                + $"{after.Stringize()} rather than {before.Stringize()}");
+        }
+
+        // The cancellation is still wanted where the argument is inside the principal
+        // interval, and there it has to stay exact rather than becoming a decimal. The
+        // expectation is simplified too because "1/2" parses as a Divf rather than a
+        // Rational (https://github.com/asc-community/AngouriMath/issues/873), and Entity
+        // equality is structural; a decimal answer still fails, which is what this pins.
+        [Theory]
+        [InlineData("arcsin(sin(1/2))", "1/2")]
+        [InlineData("arccos(cos(1/2))", "1/2")]
+        [InlineData("arctan(tan(1/2))", "1/2")]
+        [InlineData("arcsin(sin(0))", "0")]
+        [InlineData("arctan(tan(-1/3))", "-1/3")]
+        public void SimplifyingAnInverseOfItsOwnFunctionStaysExactOnThePrincipalBranch(
+            string expression, string expected)
+        {
+            Assert.Equal(expected.ToEntity().Simplify(), expression.ToEntity().Simplify());
+        }
+
+        // The other direction composes the *right* inverse and needs no assumption at all:
+        // sin(arcsin(z)) is z wherever arcsin(z) is defined. Pinned so that guarding the
+        // unsound half does not take the sound half with it.
+        [Theory]
+        [InlineData("sin(arcsin(x))")]
+        [InlineData("cos(arccos(x))")]
+        [InlineData("tan(arctan(x))")]
+        [InlineData("cotan(arccotan(x))")]
+        public void ComposingAFunctionOverItsOwnInverseIsStillTheIdentity(string input)
+        {
+            Assert.Equal("x".ToEntity(), input.ToEntity().Simplify());
+        }
+
+        static double Magnitude(Entity difference) =>
+            ((System.Numerics.Complex)difference.EvalNumerical()).Magnitude;
+
     }
 }