Fix part of #4044: Add support for computing polynomials from math ex…

…pressions (#4056) ## Explanation Fix part of #4044 Originally copied from #2173 when it was in proof-of-concept form Introduces support for computing polynomials from algebraic expressions (represented using the ``MathExpression`` proto). At a high-level, most algebraic expression representations of polynomials should be convertible to the proto ``Polynomial`` structure by the converter, but there's a main limitation: cases when polynomial factoring is needed (such as a root power) isn't currently supported, though single term polynomials can be rooted if they result in a valid root. Beyond that, any case where a non-integer or negative power occurs (such as variables in denominators or remainders during polynomial division) will result in a failure. Otherwise, full polynomial arithmetic is supported including: negation, addition, subtraction, multiplication, division (via long division), and exponentiation. Like past PRs, the converter attempts to retain maximum precision by attempting to keep integers and fraction representation as long as possible. There are also a lot of edge cases which is why the tests are so long. Some general notes: - PolynomialExtensionsTest was exempted to utilize parameterization for a few test cases - ``RealExtensions`` was updated to return a nullable result for square roots & powers to replace exceptional cases with null results so that they can be ignored similar to other failure cases - A new reverse sort function was added for iterables in ComparatorExtensions (to ensure polynomial sorting is correct) ## Essential Checklist - [x] The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".) - [x] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation. - [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide). - [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)). - [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop". - [x] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)). ## For UI-specific PRs only N/A -- This does not have user-facing changes, though it will be used as the base logic for future domain classifiers that will be added. Commit history: * Copy proto-based changes from #2173. * Introduce math.proto & refactor math extensions. Much of this is copied from #2173. * Migrate tests & remove unneeded prefix. * Add needed newline. * Some needed Fraction changes. * Introduce math expression + equation protos. Also adds testing libraries for both + fractions & reals (new structure). Most of this is copied from #2173. * Add protos + testing lib for commutative exprs. * Add protos & test libs for polynomials. * Lint fix. * Lint fixes. * Add math tokenizer + utility & tests. This is mostly copied from #2173. * Add math expression/equation parsing support. This includes full error detection, and specific test suites for each parsing case. Much revisement is needed in the tests, and some additional issues may yet need to be fixed in the parser and/or error-detection logic. This is copied from #2173 with revisement & reduction since it's part of a multi-PR split. * Add exp evaluation & LaTeX conversion support. This is mainly copied from #2173. * Remove unneeded comment lines. * Add expr->comparable operation list conv support. This enables the ability to compare two expressions such that operation associativity and commutativity is considered (i.e. items can be rearranged using those rules without breaking expression equality). This is mostly copied from #2173. * Add support for expression->polynomial conversion. This is mostly copied from #2173. * Fix broken test post-refactor. * Add reasonable import for abs(). * Fix equals errors for equations. This splits the current error into two: one for no equals being present (& adds it), and one for too many equals. This better supports the UI errors that need to be displayed to the user in these cases. * Fix rational^rational powers. This generifies the sqrt algorithm to support n-roots so that rationals raised by rationals can actually work & retain the rational (in cases where the root can actually be taken). * Ensure rational terms reduce to ints. This ensures cases like 8/1 become just '8' coefficients rather than staying as an irrational (for simplification). * Post-merge fix. * Add regex check, docs, and resolve TODOs. This also changes regex handling in the check to be more generic for better flexibility when matching files. * Lint fix. * Fix failing static checks. * Fix broken CI checks. Adds missing KDocs, test file exemptions, and fixes the Gradle build. * Lint fixes. * Add docs & exempted tests. * Remove blank line. * Add docs + tests. * Add parameterized test runner. This commit introduces a new parameterized test runner that allows proper combinations of parameterized & non-parameterized tests in the same suite, and in a way that should work on both Robolectric & Espresso (though the latter isn't currently verified). Further, this commit also introduces a TokenSubject that will be used more explicitly by the follow-up commit for verifying MathTokenizer. * Add & update tests. This introduces tests for PeekableIterator, and reimplements all of MathTokenizer's tests to be more structured, thorough, and a bit more maintainable (i.e. by leveraging parameterized tests). * Lint fixes. This includes a fix for 'fun interface' not working with ktlint (see #4122). * Remove internals that broke things. * Add regex exemptions. * Post-merge fix. * Add error tests. These tests are more or less comprehensive based on existing tests and some new ideas. All errors are now covered by MathExpressionParserTest. Error ordering is not tested. A new Truth subject was added for easier testing, as well (for MathParsingError). * Finish algebraic equation tests. * Reimplement numeric expression tests. This is almost a full replacement. The new tests are more structured and intentional to cover key high-level concepts. More tests may be added in the future, but this is a sensible initial test offering. This also updates MathExpressionSubject to support checking specifically for implicit multiplication (and it's now required for such cases since explicit is otherwise assumed). * Finish algebraic expression tests. These largely rely on numeric expression tests (since they focus on verifying specific variable scenarios). * Add missing tests for better coverage. * Add KDocs & test exemptions. * Lint fixes. * Remove temporary TODOs. * Add tests. * Split StringToFractionParser. This is a temporary change that will be finished upstream (since there's an earlier PR that's a better fit for this change). * Address reviewer comments + other stuff. This also fixes a typo and incorrectly ordered exemptions list I noticed during development of downstream PRs. * Move StringExtensions & fraction parsing. This splits fraction parsing between UI & utility components. * Address reviewer comments. * Alphabetize test exemptions. * Fix typo & add regex check. The new regex check makes it so that all parameterized testing can be more easily tracked by the Android TL. * Add missing KDocs. * Post-merge cleanups. Also, fix text file exemption ordering. * Add new test for negation with math symbol. * Post-merge fixes. * Add KDocs. Also, add new regex exemption for new parameterized tests in this branch. * Refactor & simplify real ext impl. Also, fix/clarify some KDocs. * Lint fixes. * Simplify operation list converter a lot. This inlines three recursive operations to be done during the actual computation to simplify the overall converter complexity (and to make determining the test matrix easier). * Prepare for new tests. * Remove the ComparableOperationList wrapper. * Change parameterized method delimiter. * Use utility directly in test. * Post-merge fixes. This adjusts for the removal of ComparableOperationList (i.e. no wrapper proto). * Add first round of tests. This includes fixes to the converter itself as it wasn't distributing both product inversions and negation correctly in several cases. Tests should now be covering these cases. * Finish initial test suite. Still needs to be cleaned up, but after converter refactoring attempts. * Simplify operation sorting comparators. * Remove old tests. * Add remaining missing tests. * KDocs & test exemption. * Renames & lint fixes. * Post-merge fixes. * Add tests. * KDocs + exemptions. Also, clean up polynomial sorting. * Lint fixes. * Use more intentional epsilons for float comparing. * Treat en-dash as a subtraction symbol. * Add explicit platform selection for paramerized. This adds explicit platform selection support rather than it being automatic based on deps. While less flexible for shared tests, this offers better control for tests that don't want to to use Robolectric for local tests. This also adds a JUnit-only test runner, and updates MathTokenizerTest to use it (which led to an almost 40x decrease in runtime). * Exemption fixes. Also, fix name for the AndroidJUnit4 runner. * Remove failing test. * Fix unary expression precedence. Also, use ParameterizedJunitTestRunner for MathExpressionParserTest. * Fixes & add more test cases. * Post-merge fixes & test changes. Also, update RealExtensionsTest to use the faster JUnit runner. * Use utility directly in LaTeX tests. * Post-merge fixes. Also, update ExpressionToComparableOperationConverterTest to use the fast JUnit-only runner. * Post-merge fixes. Also, update PolynomialExtensionsTest to use fast JUnit-only runner. * Address reviewer comment. Clarifies the documentation in the test runner around parameter injection. * Fix broken build. * Fix broken build post-merge. * Post-merge fix. * More post-merge fixes. * Fix TODO comment.
oppia · Mar 26, 2022 · 8112421 · 8112421
1 parent dbe4e91
commit 8112421
Show file tree

Hide file tree

Showing 15 changed files with 6,256 additions and 150 deletions.
diff --git a/scripts/assets/file_content_validation_checks.textproto b/scripts/assets/file_content_validation_checks.textproto
@@ -290,6 +290,7 @@ file_content_checks {
   exempted_file_name: "utility/src/test/java/org/oppia/android/util/math/ExpressionToComparableOperationConverterTest.kt"
   exempted_file_name: "utility/src/test/java/org/oppia/android/util/math/MathExpressionParserTest.kt"
   exempted_file_name: "utility/src/test/java/org/oppia/android/util/math/MathTokenizerTest.kt"
+  exempted_file_name: "utility/src/test/java/org/oppia/android/util/math/PolynomialExtensionsTest.kt"
   exempted_file_name: "utility/src/test/java/org/oppia/android/util/math/RealExtensionsTest.kt"
   exempted_file_patterns: "testing/src/main/java/org/oppia/android/testing/junit/.+?\\.kt"
 }
diff --git a/utility/src/main/java/org/oppia/android/util/math/BUILD.bazel b/utility/src/main/java/org/oppia/android/util/math/BUILD.bazel
@@ -124,6 +124,7 @@ kt_android_library(
     deps = [
         ":expression_to_comparable_operation_converter",
         ":expression_to_latex_converter",
+        ":expression_to_polynomial_converter",
         ":numeric_expression_evaluator",
         "//model/src/main/proto:math_java_proto_lite",
     ],
@@ -135,6 +136,7 @@ kt_android_library(
         "PolynomialExtensions.kt",
     ],
     deps = [
+        ":comparator_extensions",
         ":real_extensions",
         "//model/src/main/proto:math_java_proto_lite",
     ],
@@ -189,6 +191,18 @@ kt_android_library(
     ],
 )
 
+kt_android_library(
+    name = "expression_to_polynomial_converter",
+    srcs = [
+        "ExpressionToPolynomialConverter.kt",
+    ],
+    deps = [
+        ":polynomial_extensions",
+        ":real_extensions",
+        "//model/src/main/proto:math_java_proto_lite",
+    ],
+)
+
 kt_android_library(
     name = "numeric_expression_evaluator",
     srcs = [

diff --git a/utility/src/main/java/org/oppia/android/util/math/ComparatorExtensions.kt b/utility/src/main/java/org/oppia/android/util/math/ComparatorExtensions.kt
@@ -14,11 +14,34 @@ import com.google.protobuf.MessageLite
  * all of their items are equal per this [Comparator], including duplicates.
  */
 fun <U> Comparator<U>.compareIterables(first: Iterable<U>, second: Iterable<U>): Int {
+  return compareIterablesInternal(first, second, reverseItemSort = false)
+}
+
+/**
+ * Compares two [Iterable]s based on an item [Comparator] and returns the result, in much the same
+ * way as [compareIterables] except this reverses the result (that is, [first] will be considered
+ * less than [second] if it's larger).
+ *
+ * This should be used in place of a standard 'reversed()' since it will properly reverse (both the
+ * internal sorting and the comparison needs to be reversed in order for the reversal to be
+ * correct).
+ */
+fun <U> Comparator<U>.compareIterablesReversed(first: Iterable<U>, second: Iterable<U>): Int {
+  // Note that first & second are reversed here.
+  return compareIterablesInternal(second, first, reverseItemSort = true)
+}
+
+private fun <U> Comparator<U>.compareIterablesInternal(
+  first: Iterable<U>,
+  second: Iterable<U>,
+  reverseItemSort: Boolean
+): Int {
   // Reference: https://stackoverflow.com/a/30107086.
-  val firstIter = first.sortedWith(this).iterator()
-  val secondIter = second.sortedWith(this).iterator()
+  val itemComparator = if (reverseItemSort) reversed() else this
+  val firstIter = first.sortedWith(itemComparator).iterator()
+  val secondIter = second.sortedWith(itemComparator).iterator()
   while (firstIter.hasNext() && secondIter.hasNext()) {
-    val comparison = this.compare(firstIter.next(), secondIter.next())
+    val comparison = this.compare(firstIter.next(), secondIter.next()).coerceIn(-1..1)
     if (comparison != 0) return comparison // Found a different item.
   }
 

diff --git a/utility/src/main/java/org/oppia/android/util/math/ExpressionToPolynomialConverter.kt b/utility/src/main/java/org/oppia/android/util/math/ExpressionToPolynomialConverter.kt
@@ -0,0 +1,167 @@
+package org.oppia.android.util.math
+
+import org.oppia.android.app.model.MathBinaryOperation
+import org.oppia.android.app.model.MathBinaryOperation.Operator.ADD
+import org.oppia.android.app.model.MathBinaryOperation.Operator.DIVIDE
+import org.oppia.android.app.model.MathBinaryOperation.Operator.EXPONENTIATE
+import org.oppia.android.app.model.MathBinaryOperation.Operator.MULTIPLY
+import org.oppia.android.app.model.MathBinaryOperation.Operator.SUBTRACT
+import org.oppia.android.app.model.MathExpression
+import org.oppia.android.app.model.MathExpression.ExpressionTypeCase.BINARY_OPERATION
+import org.oppia.android.app.model.MathExpression.ExpressionTypeCase.CONSTANT
+import org.oppia.android.app.model.MathExpression.ExpressionTypeCase.EXPRESSIONTYPE_NOT_SET
+import org.oppia.android.app.model.MathExpression.ExpressionTypeCase.FUNCTION_CALL
+import org.oppia.android.app.model.MathExpression.ExpressionTypeCase.GROUP
+import org.oppia.android.app.model.MathExpression.ExpressionTypeCase.UNARY_OPERATION
+import org.oppia.android.app.model.MathExpression.ExpressionTypeCase.VARIABLE
+import org.oppia.android.app.model.MathFunctionCall.FunctionType
+import org.oppia.android.app.model.MathFunctionCall.FunctionType.FUNCTION_UNSPECIFIED
+import org.oppia.android.app.model.MathFunctionCall.FunctionType.SQUARE_ROOT
+import org.oppia.android.app.model.MathUnaryOperation
+import org.oppia.android.app.model.MathUnaryOperation.Operator.NEGATE
+import org.oppia.android.app.model.MathUnaryOperation.Operator.POSITIVE
+import org.oppia.android.app.model.Polynomial
+import org.oppia.android.app.model.Real
+import org.oppia.android.app.model.MathBinaryOperation.Operator as BinaryOperator
+import org.oppia.android.app.model.MathUnaryOperation.Operator as UnaryOperator
+
+/**
+ * Converter from [MathExpression] to [Polynomial].
+ *
+ * See the separate protos for specifics on structure, and [reduceToPolynomial] for the actual
+ * conversion function.
+ */
+class ExpressionToPolynomialConverter private constructor() {
+  companion object {
+    /**
+     * Returns a new [Polynomial] that represents this [MathExpression], or null if it's not a valid
+     * polynomial.
+     *
+     * Polynomials are defined as a list of terms where each term has a coefficient and zero or more
+     * variables. There are a number of specific constraints that this function guarantees for all
+     * returned polynomials:
+     * - Terms will never have duplicate variable expressions (e.g. there will never be a returned
+     *   polynomial with multiple 'x' terms, but there can be an 'x' and 'x^2' term). This is
+     *   because effort is taken to combine like terms.
+     * - Terms are always sorted by lexicography of the variable names and variable powers which
+     *   allows for comparison that operates independently of commutativity, associativity, and
+     *   distributivity.
+     * - There will only ever be at most one constant term in the polynomial.
+     * - There will always be at least 1 term (even if it's the constant zero).
+     * - The polynomial will be mathematically equivalent to the original expression.
+     * - Coefficients will be kept to the highest possible precision (i.e. integers and fractions
+     *   will be preferred over irrationals unless a rounding error occurs).
+     * - Most polynomial operations will be computed, including unary negation, addition,
+     *   subtraction, multiplication (both implicit and explicit), division, and powers.
+     *
+     * Note that this will return null if a polynomial cannot be computed, such as in the cases:
+     * - The expression represents a division where the result has a remainder polynomial.
+     * - The expression results in a variable with a negative power or a division by an expression.
+     * - The expression results in a non-integer power (which includes a current limitation for
+     *   expressions like 'sqrt(x)^2'; these cannot pass because internally the method cannot
+     *   represent 'x^1/2').
+     * - The expression results in a power variable (which can never represent a polynomial).
+     * - The expression is invalid (e.g. a default proto instance).
+     *
+     * This function is only expected to be used in conjunction with algebraic expressions. It's
+     * suggested to use evaluation when comparing for equivalence among numeric expressions as it
+     * should yield the same result and be more performant.
+     *
+     * The tests for this method provide very thorough and broad examples of different cases that
+     * this function supports. In particular, the equality tests are useful to see what sorts of
+     * expressions can be considered the same per [Polynomial] representation.
+     */
+    fun MathExpression.reduceToPolynomial(): Polynomial? {
+      return replaceSquareRoots()
+        .reduceToPolynomialAux()
+        ?.removeUnnecessaryVariables()
+        ?.simplifyRationals()
+        ?.sort()
+    }
+
+    private fun MathExpression.replaceSquareRoots(): MathExpression {
+      return when (expressionTypeCase) {
+        BINARY_OPERATION -> toBuilder().apply {
+          binaryOperation = binaryOperation.toBuilder().apply {
+            leftOperand = binaryOperation.leftOperand.replaceSquareRoots()
+            rightOperand = binaryOperation.rightOperand.replaceSquareRoots()
+          }.build()
+        }.build()
+        UNARY_OPERATION -> toBuilder().apply {
+          unaryOperation = unaryOperation.toBuilder().apply {
+            operand = unaryOperation.operand.replaceSquareRoots()
+          }.build()
+        }.build()
+        FUNCTION_CALL -> when (functionCall.functionType) {
+          SQUARE_ROOT -> toBuilder().apply {
+            // Replace the square root function call with the equivalent exponentiation. That is,
+            // sqrt(x)=x^(1/2).
+            binaryOperation = MathBinaryOperation.newBuilder().apply {
+              operator = EXPONENTIATE
+              leftOperand = functionCall.argument.replaceSquareRoots()
+              rightOperand = MathExpression.newBuilder().apply {
+                constant = ONE_HALF
+              }.build()
+            }.build()
+          }.build()
+          FUNCTION_UNSPECIFIED, FunctionType.UNRECOGNIZED, null -> this
+        }
+        // This also eliminates groups from the expression.
+        GROUP -> group.replaceSquareRoots()
+        CONSTANT, VARIABLE, EXPRESSIONTYPE_NOT_SET, null -> this
+      }
+    }
+
+    private fun MathExpression.reduceToPolynomialAux(): Polynomial? {
+      return when (expressionTypeCase) {
+        CONSTANT -> createConstantPolynomial(constant)
+        VARIABLE -> createSingleVariablePolynomial(variable)
+        BINARY_OPERATION -> binaryOperation.reduceToPolynomial()
+        UNARY_OPERATION -> unaryOperation.reduceToPolynomial()
+        // Both functions & groups should be removed ahead of polynomial reduction.
+        FUNCTION_CALL, GROUP, EXPRESSIONTYPE_NOT_SET, null -> null
+      }
+    }
+
+    private fun MathBinaryOperation.reduceToPolynomial(): Polynomial? {
+      val leftPolynomial = leftOperand.reduceToPolynomialAux() ?: return null
+      val rightPolynomial = rightOperand.reduceToPolynomialAux() ?: return null
+      return when (operator) {
+        ADD -> leftPolynomial + rightPolynomial
+        SUBTRACT -> leftPolynomial - rightPolynomial
+        MULTIPLY -> leftPolynomial * rightPolynomial
+        DIVIDE -> leftPolynomial / rightPolynomial
+        EXPONENTIATE -> leftPolynomial pow rightPolynomial
+        BinaryOperator.OPERATOR_UNSPECIFIED, BinaryOperator.UNRECOGNIZED, null -> null
+      }
+    }
+
+    private fun MathUnaryOperation.reduceToPolynomial(): Polynomial? {
+      return when (operator) {
+        NEGATE -> -(operand.reduceToPolynomialAux() ?: return null)
+        POSITIVE -> operand.reduceToPolynomialAux() // Positive unary changes nothing.
+        UnaryOperator.OPERATOR_UNSPECIFIED, UnaryOperator.UNRECOGNIZED, null -> null
+      }
+    }
+
+    private fun createSingleVariablePolynomial(variableName: String): Polynomial {
+      return createSingleTermPolynomial(
+        Polynomial.Term.newBuilder().apply {
+          coefficient = ONE
+          addVariable(
+            Polynomial.Term.Variable.newBuilder().apply {
+              name = variableName
+              power = 1
+            }.build()
+          )
+        }.build()
+      )
+    }
+
+    private fun createConstantPolynomial(constant: Real): Polynomial =
+      createSingleTermPolynomial(Polynomial.Term.newBuilder().setCoefficient(constant).build())
+
+    private fun createSingleTermPolynomial(term: Polynomial.Term): Polynomial =
+      Polynomial.newBuilder().apply { addTerm(term) }.build()
+  }
+}
diff --git a/utility/src/main/java/org/oppia/android/util/math/FloatExtensions.kt b/utility/src/main/java/org/oppia/android/util/math/FloatExtensions.kt
@@ -3,7 +3,9 @@ package org.oppia.android.util.math
 import kotlin.math.abs
 
 /**
- * The error margin used for approximately [Float] equality checking.
+ * The error margin used for approximately [Float] equality checking, that is, the largest distance
+ * from any particular number before a new value will be considered unequal (i.e. all values between
+ * a float and (float-interval, float+interval) will be considered equal to the float).
  *
  * Note that the machine epsilon value from https://en.wikipedia.org/wiki/Machine_epsilon is defined
  * defined as the smallest value that, when added to, or subtract from, 1, will result in a value

diff --git a/utility/src/main/java/org/oppia/android/util/math/FractionExtensions.kt b/utility/src/main/java/org/oppia/android/util/math/FractionExtensions.kt
@@ -10,8 +10,10 @@ fun Fraction.hasFractionalPart(): Boolean {
 }
 
 /**
- * Returns whether this fraction only represents a whole number. Note that for the fraction '0' this
- * will return true.
+ * Returns whether this fraction only represents a whole number.
+ *
+ * Note that for the fraction '0' this will return true. Furthermore, this will return false for
+ * whole number-like improper fractions such as '3/1'.
  */
 fun Fraction.isOnlyWholeNumber(): Boolean {
   return !hasFractionalPart()

diff --git a/utility/src/main/java/org/oppia/android/util/math/MathExpressionExtensions.kt b/utility/src/main/java/org/oppia/android/util/math/MathExpressionExtensions.kt
@@ -3,9 +3,11 @@ package org.oppia.android.util.math
 import org.oppia.android.app.model.ComparableOperation
 import org.oppia.android.app.model.MathEquation
 import org.oppia.android.app.model.MathExpression
+import org.oppia.android.app.model.Polynomial
 import org.oppia.android.app.model.Real
 import org.oppia.android.util.math.ExpressionToComparableOperationConverter.Companion.convertToComparableOperation
 import org.oppia.android.util.math.ExpressionToLatexConverter.Companion.convertToLatex
+import org.oppia.android.util.math.ExpressionToPolynomialConverter.Companion.reduceToPolynomial
 import org.oppia.android.util.math.NumericExpressionEvaluator.Companion.evaluate
 
 /**
@@ -37,3 +39,10 @@ fun MathExpression.evaluateAsNumericExpression(): Real? = evaluate()
  * See [convertToComparableOperation] for details.
  */
 fun MathExpression.toComparableOperation(): ComparableOperation = convertToComparableOperation()
+
+/**
+ * Returns the [Polynomial] representation of this [MathExpression].
+ *
+ * See [reduceToPolynomial] for details.
+ */
+fun MathExpression.toPolynomial(): Polynomial? = reduceToPolynomial()