Fix criteria crashes: matches tolerance, complex infinity, written-out quantities in criteria

app/context/physical_quantity/comparison.py

@@ -1,25 +1,53 @@
 from __future__ import annotations
 
-from collections.abc import Callable
+from collections.abc import Callable, Mapping
+from dataclasses import dataclass
 
 from sympy import Basic, Symbol
 
 from lambdafeedback.expression_parsing import parse_expression
 from lambdafeedback.slr_parsing import ExprNode
+from lambdafeedback.units import parse_quantity as parse_physical_quantity
 
 from app.context.operators import COMPARISON_OPERATORS
+from app.context.physical_quantity.parsing import ParsedQuantity, parsed_quantity
 from app.evaluation_parameters import EvaluationParameters
 from app.types import Substitutions
 
 NONE_PLACEHOLDER = Symbol('NONE_PLACEHOLDER')
 
 
-def parse_criterion_sides(criterion: ExprNode, parameters: EvaluationParameters) -> tuple[str, str, Basic, Basic]:
-    """The left- and right-hand sides of `criterion` as strings, and parsed without simplification."""
+@dataclass(frozen=True)
+class CriterionSides:
+    """The left- and right-hand sides of a criterion, and the quantities that the sides are written in terms of."""
+    lhs_string: str
+    rhs_string: str
+    lhs: Basic
+    rhs: Basic
+    # The response, the answer and any quantity written out in the criterion (e.g. `2 m`), by the
+    # symbol that stands for them in `lhs` and `rhs`
+    quantities: Mapping[str, ParsedQuantity]
+
+
+def parse_criterion_sides(criterion: ExprNode, parameters: EvaluationParameters) -> CriterionSides:
+    """The sides of `criterion`, where a side written out as a quantity is parsed with its units."""
     parsing_params = parameters.parsing_parameters["sympy"].replace(simplify=False)
-    lhs_string = criterion.children[0].content_string()
-    rhs_string = criterion.children[1].content_string()
-    return lhs_string, rhs_string, parse_expression(lhs_string, parsing_params), parse_expression(rhs_string, parsing_params)
+    quantities = dict(parameters.reserved_expressions)
+    sides = []
+    for (name, node) in zip(("criterion_lhs", "criterion_rhs"), criterion.children):
+        if node.label == "INPUT":
+            quantity = parse_physical_quantity(node.content_string(), parameters.parsing_parameters["quantity"], name)
+            quantities[name] = parsed_quantity(quantity)
+            sides.append(Symbol(name))
+        else:
+            sides.append(parse_expression(node.content_string(), parsing_params))
+    return CriterionSides(
+        lhs_string=criterion.children[0].content_string(),
+        rhs_string=criterion.children[1].content_string(),
+        lhs=sides[0],
+        rhs=sides[1],
+        quantities=quantities,
+    )
 
 
 def _substitute(lhs: Basic, rhs: Basic, substitutions: Substitutions) -> tuple[Basic, Basic]:
@@ -28,12 +56,16 @@ def _substitute(lhs: Basic, rhs: Basic, substitutions: Substitutions) -> tuple[B
     return lhs.subs(local_substitutions), rhs.subs(local_substitutions)
 
 
-def comparison_function(comparison_operator: str) -> Callable[[Basic, Basic, Substitutions], bool]:
+def comparison_function(comparison_operator: str) -> Callable[[Basic, Basic, Substitutions], bool | None]:
     comparison = COMPARISON_OPERATORS[comparison_operator]
 
-    def comparison_function_inner(lhs: Basic, rhs: Basic, substitutions: Substitutions) -> bool:
+    def comparison_function_inner(lhs: Basic, rhs: Basic, substitutions: Substitutions) -> bool | None:
         expr0, expr1 = _substitute(lhs, rhs, substitutions)
-        return bool(comparison((expr0-expr1).cancel().simplify().simplify(), 0))
+        try:
+            return bool(comparison((expr0-expr1).cancel().simplify().simplify(), 0))
+        except TypeError:
+            # The difference still contains symbols, so sympy cannot determine the truth value
+            return None
     return comparison_function_inner
 
 

app/context/physical_quantity/graphs/match.py

@@ -10,7 +10,7 @@
 from lambdafeedback.slr_parsing import ExprNode
 
 from app.context.graph_helpers import attach_terminal_criterion, outcome_tag
-from app.context.physical_quantity.comparison import comparison_function, is_proportional, parse_criterion_sides
+from app.context.physical_quantity.comparison import CriterionSides, comparison_function, is_proportional, parse_criterion_sides
 from app.evaluation_parameters import EvaluationParameters
 from app.feedback.physical_quantity import feedback_string_generators as physical_quantity_feedback_string_generators
 
@@ -49,67 +49,78 @@ def _missing_or_unexpected(parameters: EvaluationParameters, label: str, lhs_str
 
 def _within_tolerance(parameters: EvaluationParameters, lhs_string: str, rhs_string: str, rtol: float, atol: float) -> bool:
     """Whether the standard values of the response and answer are within the numerical tolerances."""
-    value_match = False
     # TODO: better analysis of where `answer` is found in the criteria so that
     #       numerical tolerances can be applied appropriately
-    if rtol > 0 or atol > 0:
-        if (lhs_string == 'answer' and rhs_string == 'response') or (lhs_string == 'response' and rhs_string == 'answer'):
-            ans = parameters.reserved_expressions["answer"].standard_value.simplify()
-            res = parameters.reserved_expressions["response"].standard_value.simplify()
-        if (ans is not None and ans.is_constant()) and (res is not None and res.is_constant()):
-            if rtol > 0 and (ans != 0):
-                value_match = bool(abs(float((ans-res)/ans)) < rtol)
-            elif atol > 0 or (ans == 0):
-                answer_unit_factor = float(parameters.reserved_expressions["answer"].quantity.unit_factor)
-                value_match = bool(abs(float(ans-res)) < atol*answer_unit_factor)
+    if rtol == 0 and atol == 0:
+        return False
+
+    if not ((lhs_string == 'answer' and rhs_string == 'response') or (lhs_string == 'response' and rhs_string == 'answer')):
+        return False
+
+    ans_value = parameters.reserved_expressions["answer"].standard_value
+    res_value = parameters.reserved_expressions["response"].standard_value
+
+    # Quantities with several standard values are a set, and quantities without a value are None
+    if not (isinstance(ans_value, Basic) and isinstance(res_value, Basic)):
+        return False
+
+    ans = ans_value.simplify()
+    res = res_value.simplify()
+
+    value_match = False
+
+    if ans.is_constant() and res.is_constant():
+        if rtol > 0 and (ans != 0):
+            value_match = bool(abs(float((ans-res)/ans)) < rtol)
+        elif atol > 0 or (ans == 0):
+            answer_unit_factor = float(parameters.reserved_expressions["answer"].quantity.unit_factor)
+            value_match = bool(abs(float(ans-res)) < atol*answer_unit_factor)
     return value_match
 
 
-def _quantity_match(
-    parameters: EvaluationParameters, label: str, lhs: Basic, rhs: Basic, lhs_string: str, rhs_string: str, rtol: float, atol: float
-) -> Evaluate:
-    reserved_expressions = parameters.reserved_expressions.items()
+def _quantity_match(parameters: EvaluationParameters, label: str, sides: CriterionSides, rtol: float, atol: float) -> Evaluate:
+    quantities = sides.quantities.items()
 
     def quantity_match(_response: Any) -> ReachedCriteria:
-        missing_or_unexpected = _missing_or_unexpected(parameters, label, lhs_string, rhs_string)
+        missing_or_unexpected = _missing_or_unexpected(parameters, label, sides.lhs_string, sides.rhs_string)
         if missing_or_unexpected is not None:
             return missing_or_unexpected
 
-        substitutions = [(key, expr.standard_value) for (key, expr) in reserved_expressions]
-        value_match = is_equal(lhs, rhs, substitutions)
+        substitutions = [(key, expr.standard_value) for (key, expr) in quantities]
+        value_match = is_equal(sides.lhs, sides.rhs, substitutions)
         if value_match is False:
-            value_match = _within_tolerance(parameters, lhs_string, rhs_string, rtol, atol)
+            value_match = _within_tolerance(parameters, sides.lhs_string, sides.rhs_string, rtol, atol)
 
-        substitutions = [(key, expr.standard_unit) for (key, expr) in reserved_expressions]
-        unit_match = is_equal(lhs, rhs, substitutions)
+        substitutions = [(key, expr.standard_unit) for (key, expr) in quantities]
+        unit_match = is_equal(sides.lhs, sides.rhs, substitutions)
 
-        return outcome_tag(label, value_match is True and unit_match is True, {"lhs": lhs_string, "rhs": rhs_string})
+        return outcome_tag(label, value_match is True and unit_match is True, {"lhs": sides.lhs_string, "rhs": sides.rhs_string})
     return quantity_match
 
 
-def _dimension_match(parameters: EvaluationParameters, label: str, lhs: Basic, rhs: Basic, lhs_string: str, rhs_string: str) -> Evaluate:
-    reserved_expressions = parameters.reserved_expressions.items()
+def _dimension_match(label: str, sides: CriterionSides) -> Evaluate:
+    quantities = sides.quantities.items()
 
     def dimension_match(_response: Any) -> ReachedCriteria:
-        substitutions = [(key, expr.dimension) for (key, expr) in reserved_expressions]
-        dimension_match, _ = is_proportional(lhs, rhs, substitutions)
-        return outcome_tag(label+"_DIMENSION_MATCH", dimension_match is True, {"lhs": lhs_string, "rhs": rhs_string})
+        substitutions = [(key, expr.dimension) for (key, expr) in quantities]
+        dimension_match, _ = is_proportional(sides.lhs, sides.rhs, substitutions)
+        return outcome_tag(label+"_DIMENSION_MATCH", dimension_match is True, {"lhs": sides.lhs_string, "rhs": sides.rhs_string})
     return dimension_match
 
 
-def _unit_comparison(parameters: EvaluationParameters, label: str, lhs: Basic, rhs: Basic) -> Evaluate:
-    reserved_expressions = parameters.reserved_expressions.items()
+def _unit_comparison(label: str, sides: CriterionSides) -> Evaluate:
+    quantities = sides.quantities.items()
 
     def unit_comparison(_response: Any) -> ReachedCriteria:
-        substitutions = [(key, expr.original_unit) for (key, expr) in reserved_expressions]
+        substitutions = [(key, expr.original_unit) for (key, expr) in quantities]
 
-        if is_equal(lhs, rhs, substitutions) is True:
+        if is_equal(sides.lhs, sides.rhs, substitutions) is True:
             output_tags = {
                 label+"_UNIT_COMPARISON"+"_IDENTICAL": None
             }
         else:
-            local_substitutions = [(key, expr.quantity.expanded_unit) for (key, expr) in reserved_expressions]
-            result, ratio = is_proportional(lhs, rhs, local_substitutions)
+            local_substitutions = [(key, expr.quantity.expanded_unit) for (key, expr) in quantities]
+            result, ratio = is_proportional(sides.lhs, sides.rhs, local_substitutions)
             # `is_proportional` returns a ratio whenever the result is True
             if result is True and ratio is not None and ratio >= 1000:
                 output_tags = {
@@ -263,15 +274,15 @@ def criterion_match_node(criterion: ExprNode, parameters: EvaluationParameters,
         label = criterion.content_string()
 
     inputs = criterion.children
-    lhs_string, rhs_string, lhs, rhs = parse_criterion_sides(criterion, parameters)
+    sides = parse_criterion_sides(criterion, parameters)
 
     _add_match_nodes(
         graph,
         END,
         label,
         inputs,
-        quantity_match=_quantity_match(parameters, label, lhs, rhs, lhs_string, rhs_string, rtol, atol),
-        dimension_match=_dimension_match(parameters, label, lhs, rhs, lhs_string, rhs_string),
-        unit_comparison=_unit_comparison(parameters, label, lhs, rhs),
+        quantity_match=_quantity_match(parameters, label, sides, rtol, atol),
+        dimension_match=_dimension_match(label, sides),
+        unit_comparison=_unit_comparison(label, sides),
     )
     return graph

app/context/physical_quantity/graphs/order.py

@@ -14,19 +14,21 @@
 def comparison_base_graph(
     criterion: ExprNode, parameters: EvaluationParameters, comparison_operator: str, label: str | None = None
 ) -> CriteriaGraph:
-    reserved_expressions = parameters.reserved_expressions.items()
-
     if label is None:
         label = criterion.content_string()
 
     inputs = criterion.children
-    _, _, lhs, rhs = parse_criterion_sides(criterion, parameters)
+    sides = parse_criterion_sides(criterion, parameters)
 
-    stardard_forms = [(key, expr.standard_value*expr.standard_unit) for (key, expr) in reserved_expressions]
+    # A quantity without a unit (standard_unit None) is just its value
+    stardard_forms = [
+        (key, expr.standard_value if expr.standard_unit is None else expr.standard_value*expr.standard_unit)
+        for (key, expr) in sides.quantities.items()
+    ]
     compare = comparison_function(comparison_operator)
 
     def compare_evaluate(_response: Any) -> ReachedCriteria:
-        return outcome_tag(label, compare(lhs, rhs, stardard_forms) is True)
+        return outcome_tag(label, compare(sides.lhs, sides.rhs, stardard_forms) is True)
 
     graph, END = outcome_graph(
         label,

app/context/physical_quantity/parsing.py

@@ -36,6 +36,11 @@ def parse_quantity(name: str, expr: str, parameters: Parameters, evaluation_resu
     quantity = parse_physical_quantity(expr, parameters["quantity"], name)
     for index, reverted in enumerate(quantity.reverted_units):
         evaluation_result.add_feedback(f"{name}_REVERTED_UNIT_{index}", reverted_unit_feedback(reverted))
+    return parsed_quantity(quantity)
+
+
+def parsed_quantity(quantity: PhysicalQuantity) -> ParsedQuantity:
+    """`quantity` with the forms of its value and unit used by the criteria."""
     standard_value: Expression | None
     if quantity.standard_values is None:
         standard_value = None

app/context/symbolic/comparison.py

@@ -77,15 +77,10 @@ def create_expressions_for_comparison(
 def do_comparison_infinite(comparison_symbol: str, lhs_expr: Basic, rhs_expr: Basic) -> bool | None:
     # When either side is infinite, subtracting (e.g. oo - oo) yields nan, making
     # the subtraction-based do_comparison unreliable. Compare the sides directly instead.
-    direct_comparisons = {
-        "=": lhs_expr == rhs_expr,
-        ">": lhs_expr > rhs_expr,
-        ">=": lhs_expr >= rhs_expr,
-        "<": lhs_expr < rhs_expr,
-        "<=": lhs_expr <= rhs_expr,
-    }
+    # Only the requested comparison is evaluated: ordering complex infinity (zoo) raises TypeError.
+    comparison = COMPARISON_OPERATORS[comparison_symbol.strip()]
     try:
-        return bool(direct_comparisons[comparison_symbol.strip()])
+        return bool(comparison(lhs_expr, rhs_expr))
     except Exception:
         return None
 

app/tests/physical_quantity_evaluation_test.py

@@ -548,6 +548,74 @@ def test_physical_quantity_with_rtol_and_imperial_mass_unit(self):
         result = evaluation_function(res, ans, params, include_test_data=True)
         assert result["is_correct"] is True
 
+    @pytest.mark.parametrize(
+        "criteria,is_correct",
+        [
+            ("response matches 2 m", False),
+            ("response matches 2", False),
+            ("2 m matches response", False),
+            # Only checks that it no longer raises: the value match ignores `dimension(...)`
+            ("dimension(response) matches dimension(answer)", None),
+            ("answer matches answer", True),
+        ]
+    )
+    def test_matches_criteria_other_than_response_and_answer_when_values_differ(self, criteria, is_correct):
+        # Used to raise UnboundLocalError in the tolerance check (bug 1 in the bug report)
+        params = {
+            'strict_syntax': False,
+            'physical_quantity': True,
+            'elementary_functions': True,
+            'criteria': criteria,
+        }
+        result = evaluation_function("2.1 m", "1.0 m", params)
+        if is_correct is not None:
+            assert result["is_correct"] is is_correct
+
+    @pytest.mark.parametrize(
+        "res,ans,criteria,is_correct",
+        [
+            # Used to raise TypeError: cannot determine truth value of Relational (bug 14 in the bug report)
+            ("2.1 m", "1 m", "response > 1.5 m", True),
+            ("2.1 m", "1 m", "response > 1.5 metre", True),
+            ("2.1 m", "1 m", "response > 3 m", False),
+            ("2.1 m", "1 m", "response > answer", True),
+            # Used to raise TypeError when the quantities have no units
+            ("2.1", "1", "response > answer", True),
+            ("2.1", "1", "response < 2", False),
+        ]
+    )
+    def test_order_criteria_against_written_out_quantity(self, res, ans, criteria, is_correct):
+        params = {
+            'strict_syntax': False,
+            'physical_quantity': True,
+            'elementary_functions': True,
+            'criteria': criteria,
+        }
+        result = evaluation_function(res, ans, params)
+        assert result["is_correct"] is is_correct
+
+    @pytest.mark.parametrize(
+        "res,criteria,is_correct",
+        [
+            # Used to be false for every response (bug 16 in the bug report)
+            ("2 m", "response matches 2 m", True),
+            ("2 m", "response matches 2 metre", True),
+            ("2 m", "2 m matches response", True),
+            ("2000 mm", "response matches 2 m", True),
+            ("2.1 m", "response matches 2 m", False),
+            ("2 s", "response matches 2 m", False),
+        ]
+    )
+    def test_matches_criteria_against_written_out_quantity(self, res, criteria, is_correct):
+        params = {
+            'strict_syntax': False,
+            'physical_quantity': True,
+            'elementary_functions': True,
+            'criteria': criteria,
+        }
+        result = evaluation_function(res, "1 m", params)
+        assert result["is_correct"] is is_correct
+
     @pytest.mark.parametrize(
         "ans,res",
         [

app/tests/symbolic_evaluation_test.py

@@ -1,5 +1,8 @@
 import pytest
 
+from sympy import Integer, Symbol, oo, zoo
+
+from app.context.symbolic.comparison import do_comparison_infinite
 from app.evaluation import evaluation_function
 from lambdafeedback.expression_parsing import substitute
 from lambdafeedback.expression_parsing.symbol_tables import ELEMENTARY_FUNCTIONS as elementary_functions_names
@@ -2104,6 +2107,36 @@ def test_infinity_alias(self):
         result = evaluation_function(response, answer, params)
         assert result["is_correct"] is False
 
+    @pytest.mark.parametrize(
+        "response,answer,is_correct",
+        [
+            ("1/0", "x", False),
+            ("zoo", "1", False),
+            ("oo", "oo", True),
+        ]
+    )
+    def test_complex_infinity(self, response, answer, is_correct):
+        # Used to raise TypeError: Invalid comparison of non-real zoo (bug 10 in the bug report)
+        params = {
+            'strict_syntax': False,
+            'elementary_functions': True,
+        }
+        result = evaluation_function(response, answer, params)
+        assert result["is_correct"] is is_correct
+
+    @pytest.mark.parametrize(
+        "comparison_symbol,lhs,rhs,expected",
+        [
+            ("=", zoo, Symbol("x"), False),
+            ("=", zoo, Integer(1), False),
+            ("=", oo, oo, True),
+            (">", zoo, Integer(1), None),
+        ]
+    )
+    def test_do_comparison_infinite_only_evaluates_requested_comparison(self, comparison_symbol, lhs, rhs, expected):
+        # The end-to-end test above only reaches this function in some runs, so check it directly
+        assert do_comparison_infinite(comparison_symbol, lhs, rhs) is expected
+
     def test_equality_expression_mismatch_with_custom_criteria(self):
         response = "x-0.883x^2=0.251X^3"
         answer = "0.4842x-0.1163x^3"