From 7f4abf088efe60d238ed31344541bf7c7846a89b Mon Sep 17 00:00:00 2001 From: Marcus Messer Date: Fri, 16 Oct 2026 15:42:04 +0100 Subject: [PATCH 1/4] Refactor `_within_tolerance`: simplify logic, improve input validation, and handle edge cases for non-constant values; add targeted test coverage for criteria mismatches and tolerance checks. --- app/context/physical_quantity/graphs/match.py | 35 +++++++++++++------ .../physical_quantity_evaluation_test.py | 23 ++++++++++++ 2 files changed, 47 insertions(+), 11 deletions(-) diff --git a/app/context/physical_quantity/graphs/match.py b/app/context/physical_quantity/graphs/match.py index 33c4324..52a0250 100644 --- a/app/context/physical_quantity/graphs/match.py +++ b/app/context/physical_quantity/graphs/match.py @@ -49,19 +49,32 @@ 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 diff --git a/app/tests/physical_quantity_evaluation_test.py b/app/tests/physical_quantity_evaluation_test.py index d18b0e2..62a4dec 100644 --- a/app/tests/physical_quantity_evaluation_test.py +++ b/app/tests/physical_quantity_evaluation_test.py @@ -548,6 +548,29 @@ 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( "ans,res", [ From d3509db482cab6183eeb316b501ce3f4deaa0899 Mon Sep 17 00:00:00 2001 From: Marcus Messer Date: Fri, 16 Oct 2026 15:59:47 +0100 Subject: [PATCH 2/4] Add tests for handling infinity comparisons and refactor `do_comparison_infinite` to evaluate only requested comparisons --- app/context/symbolic/comparison.py | 11 +++------ app/tests/symbolic_evaluation_test.py | 33 +++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 8 deletions(-) diff --git a/app/context/symbolic/comparison.py b/app/context/symbolic/comparison.py index 996bf58..34f77f2 100644 --- a/app/context/symbolic/comparison.py +++ b/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 diff --git a/app/tests/symbolic_evaluation_test.py b/app/tests/symbolic_evaluation_test.py index ff1e44f..a87cca1 100644 --- a/app/tests/symbolic_evaluation_test.py +++ b/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" From b54da8cf6938b37f4e4b17c317904334297d9edd Mon Sep 17 00:00:00 2001 From: Marcus Messer Date: Fri, 16 Oct 2026 16:04:12 +0100 Subject: [PATCH 3/4] Add test for handling written-out quantities in comparison criteria and update `comparison_function` to return `None` for unresolved symbolic differences --- app/context/physical_quantity/comparison.py | 10 ++++++--- .../physical_quantity_evaluation_test.py | 21 +++++++++++++++++++ 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/app/context/physical_quantity/comparison.py b/app/context/physical_quantity/comparison.py index cb8664c..0d3afd7 100644 --- a/app/context/physical_quantity/comparison.py +++ b/app/context/physical_quantity/comparison.py @@ -28,12 +28,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 diff --git a/app/tests/physical_quantity_evaluation_test.py b/app/tests/physical_quantity_evaluation_test.py index 62a4dec..2d9c3ca 100644 --- a/app/tests/physical_quantity_evaluation_test.py +++ b/app/tests/physical_quantity_evaluation_test.py @@ -571,6 +571,27 @@ def test_matches_criteria_other_than_response_and_answer_when_values_differ(self if is_correct is not None: assert result["is_correct"] is is_correct + @pytest.mark.parametrize( + "criteria,is_correct", + [ + # Only check that these no longer raise: the units in the criterion aren't parsed yet + ("response > 1.5 m", None), + ("response > 1.5 metre", None), + ("response > answer", True), + ] + ) + def test_order_criteria_against_written_out_quantity(self, criteria, is_correct): + # Used to raise TypeError: cannot determine truth value of Relational (bug 14 in the bug report) + params = { + 'strict_syntax': False, + 'physical_quantity': True, + 'elementary_functions': True, + 'criteria': criteria, + } + result = evaluation_function("2.1 m", "1 m", params) + if is_correct is not None: + assert result["is_correct"] is is_correct + @pytest.mark.parametrize( "ans,res", [ From e567f76745a89b85c117d4eeb6f39e2306af6cb3 Mon Sep 17 00:00:00 2001 From: Marcus Messer Date: Fri, 16 Oct 2026 16:15:44 +0100 Subject: [PATCH 4/4] Refactor quantity comparison logic: introduce `CriterionSides` dataclass, simplify `parse_criterion_sides`, update match functions, and expand test coverage. --- app/context/physical_quantity/comparison.py | 40 +++++++++++--- app/context/physical_quantity/graphs/match.py | 52 +++++++++---------- app/context/physical_quantity/graphs/order.py | 12 +++-- app/context/physical_quantity/parsing.py | 5 ++ .../physical_quantity_evaluation_test.py | 44 ++++++++++++---- 5 files changed, 105 insertions(+), 48 deletions(-) diff --git a/app/context/physical_quantity/comparison.py b/app/context/physical_quantity/comparison.py index 0d3afd7..74c4139 100644 --- a/app/context/physical_quantity/comparison.py +++ b/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]: diff --git a/app/context/physical_quantity/graphs/match.py b/app/context/physical_quantity/graphs/match.py index 52a0250..e688389 100644 --- a/app/context/physical_quantity/graphs/match.py +++ b/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 @@ -78,51 +78,49 @@ def _within_tolerance(parameters: EvaluationParameters, lhs_string: str, rhs_str 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 = { @@ -276,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 diff --git a/app/context/physical_quantity/graphs/order.py b/app/context/physical_quantity/graphs/order.py index fbc9df8..1e11415 100644 --- a/app/context/physical_quantity/graphs/order.py +++ b/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, diff --git a/app/context/physical_quantity/parsing.py b/app/context/physical_quantity/parsing.py index 2be6c28..eeb0833 100644 --- a/app/context/physical_quantity/parsing.py +++ b/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 diff --git a/app/tests/physical_quantity_evaluation_test.py b/app/tests/physical_quantity_evaluation_test.py index 2d9c3ca..5612d8f 100644 --- a/app/tests/physical_quantity_evaluation_test.py +++ b/app/tests/physical_quantity_evaluation_test.py @@ -572,25 +572,49 @@ def test_matches_criteria_other_than_response_and_answer_when_values_differ(self assert result["is_correct"] is is_correct @pytest.mark.parametrize( - "criteria,is_correct", + "res,ans,criteria,is_correct", [ - # Only check that these no longer raise: the units in the criterion aren't parsed yet - ("response > 1.5 m", None), - ("response > 1.5 metre", None), - ("response > answer", True), + # 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, criteria, is_correct): - # Used to raise TypeError: cannot determine truth value of Relational (bug 14 in the bug report) + 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("2.1 m", "1 m", params) - if is_correct is not None: - assert result["is_correct"] is is_correct + 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",