Fix criteria correctness: feedback order, proportionality, variations, dimension matches

app/context/physical_quantity/comparison.py

@@ -29,21 +29,30 @@ class CriterionSides:
     quantities: Mapping[str, ParsedQuantity]
 
 
+def is_dimension(node: ExprNode) -> bool:
+    """Whether `node` is a `dimension(...)` side of a criterion."""
+    return node.label.strip() == "dimension"
+
+
 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."""
+    """
+    The sides of `criterion`, where a side written out as a quantity is parsed with its units,
+    and a side written as `dimension(...)` is the quantity inside it.
+    """
     parsing_params = parameters.parsing_parameters["sympy"].replace(simplify=False)
     quantities = dict(parameters.reserved_expressions)
+    nodes = [node.children[0] if is_dimension(node) else node for node in criterion.children]
     sides = []
-    for (name, node) in zip(("criterion_lhs", "criterion_rhs"), criterion.children):
+    for (name, node) in zip(("criterion_lhs", "criterion_rhs"), nodes):
         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_string=nodes[0].content_string(),
+        rhs_string=nodes[1].content_string(),
         lhs=sides[0],
         rhs=sides[1],
         quantities=quantities,
@@ -70,17 +79,14 @@ def comparison_function_inner(lhs: Basic, rhs: Basic, substitutions: Substitutio
 
 
 def is_proportional(lhs: Basic, rhs: Basic, substitutions: Substitutions) -> tuple[bool | None, float | None]:
+    """Whether `lhs` is a constant multiple of `rhs`, and the ratio lhs/rhs when it is."""
     expr0, expr1 = _substitute(lhs, rhs, substitutions)
-    if expr0.cancel().simplify().simplify() != 0:
-        ratio = (expr0 / expr1).cancel().simplify()
-        result = ratio.is_constant()
-    elif expr0.cancel().simplify().simplify() != 0:
-        ratio = (expr1 / expr0).cancel().simplify()
-        result = ratio.is_constant()
-    else:
-        result = False
-    if result is True:
-        ratio = float(ratio)
-    else:
-        ratio = None
-    return result, ratio
+    # A zero side is not proportional to anything, and dividing by it would give zoo, which counts as constant
+    if expr0.cancel().simplify().simplify() == 0 or expr1.cancel().simplify().simplify() == 0:
+        return False, None
+    # Keep the ratio as lhs/rhs, because the prefix feedback relies on its direction
+    ratio = (expr0 / expr1).cancel().simplify()
+    result = ratio.is_constant()
+    if result:
+        return result, float(ratio)
+    return result, None

app/context/physical_quantity/graphs/match.py

@@ -9,8 +9,14 @@
 from lambdafeedback.expression_parsing import compute_relative_tolerance_from_significant_decimals
 from lambdafeedback.slr_parsing import ExprNode
 
-from app.context.graph_helpers import attach_terminal_criterion, outcome_tag
-from app.context.physical_quantity.comparison import CriterionSides, comparison_function, is_proportional, parse_criterion_sides
+from app.context.graph_helpers import attach_terminal_criterion, outcome_graph, outcome_tag
+from app.context.physical_quantity.comparison import (
+    CriterionSides,
+    comparison_function,
+    is_dimension,
+    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
 
@@ -98,13 +104,13 @@ def quantity_match(_response: Any) -> ReachedCriteria:
     return quantity_match
 
 
-def _dimension_match(label: str, sides: CriterionSides) -> Evaluate:
+def _dimension_match(evaluation_label: str, sides: CriterionSides) -> Evaluate:
     quantities = sides.quantities.items()
 
     def dimension_match(_response: Any) -> ReachedCriteria:
         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 outcome_tag(evaluation_label, dimension_match is True, {"lhs": sides.lhs_string, "rhs": sides.rhs_string})
     return dimension_match
 
 
@@ -265,24 +271,58 @@ def _add_match_nodes(
     )
 
 
-def criterion_match_node(criterion: ExprNode, parameters: EvaluationParameters, label: str | None = None) -> CriteriaGraph:
-    graph = CriteriaGraph(label)  # type: ignore[arg-type]  # Bug 8 in the bug report: label is None here
-    END = graph.add_end_node()
-    rtol, atol = _tolerances(parameters)
+def _dimension_graph(label: str, sides: CriterionSides) -> CriteriaGraph:
+    """A graph for `dimension(...) matches dimension(...)`, which only compares the dimensions."""
+    lhs, rhs = sides.lhs_string, sides.rhs_string
+    graph, END = outcome_graph(
+        label,
+        summary=f"Do the dimensions of {lhs} and {rhs} match?",
+        details=f"Do the dimensions of {lhs} and {rhs} match?",
+        evaluate=_dimension_match(label, sides)
+    )
+    attach_terminal_criterion(
+        graph,
+        label,
+        label + "_TRUE",
+        END,
+        summary=f"The quantities {lhs} and {rhs} have the same dimensions.",
+        details=f"The quantities {lhs} and {rhs} have the same dimensions.",
+        feedback_string_generator=physical_quantity_feedback_string_generators["MATCHES"]("DIMENSION_MATCH")
+    )
+    attach_terminal_criterion(
+        graph,
+        label,
+        label + "_FALSE",
+        END,
+        summary=f"The quantities {lhs} and {rhs} have different dimensions.",
+        details=f"The quantities {lhs} and {rhs} have different dimensions.",
+        feedback_string_generator=physical_quantity_feedback_string_generators["MATCHES"]("DIMENSION_MISMATCH")
+    )
+    return graph
 
+
+def criterion_match_node(criterion: ExprNode, parameters: EvaluationParameters, label: str | None = None) -> CriteriaGraph:
     if label is None:
         label = criterion.content_string()
 
     inputs = criterion.children
     sides = parse_criterion_sides(criterion, parameters)
 
+    # The criteria grammar only has `dimension(...) matches dimension(...)`, so both sides are dimensions
+    if is_dimension(inputs[0]):
+        return _dimension_graph(label, sides)
+
+    graph = CriteriaGraph(label)
+    END = graph.add_end_node()
+    rtol, atol = _tolerances(parameters)
+
     _add_match_nodes(
         graph,
         END,
         label,
         inputs,
         quantity_match=_quantity_match(parameters, label, sides, rtol, atol),
-        dimension_match=_dimension_match(label, sides),
+        dimension_match=_dimension_match(label + "_DIMENSION_MATCH", sides),
         unit_comparison=_unit_comparison(label, sides),
     )
     return graph

app/context/symbolic/__init__.py

@@ -48,8 +48,7 @@ def expression_parse(name: str, expr: str, parameters: SympyParsingConfig, evalu
 
 def feedback_procedure_generator(parameters: EvaluationParameters) -> dict[str, CriteriaGraph]:
     graphs = dict()
-    criteria = set(parameters.criteria.items())
-    for (label, criterion) in criteria:
+    for (label, criterion) in parameters.criteria.items():
         graph_template = graph_templates.get(criterion.label, criterion_eval_node)
         graphs.update({label: graph_template(criterion, parameters)})
     return graphs

app/context/symbolic/comparison.py

@@ -5,7 +5,7 @@
 from sympy import Basic, Equality, im, I
 from sympy import re as real_part
 
-from lambdafeedback.criteria import where_substitutions
+from lambdafeedback.criteria import CriteriaSyntaxError, where_substitutions
 from lambdafeedback.expression_parsing import SympyParsingConfig, parse_expression
 from lambdafeedback.slr_parsing import ExprNode
 
@@ -23,9 +23,17 @@ def unsimplified_with_unsplittable(parsing_params: SympyParsingConfig, symbols:
     )
 
 
+def where_substitution_strings(subs_node: ExprNode) -> list[tuple[str, str]]:
+    """`[(name, expression), ...]`, unparsed, for each substitution in the substitution part of a WHERE criterion."""
+    try:
+        return where_substitutions(subs_node)
+    except CriteriaSyntaxError as e:
+        raise Exception("Cannot parse criteria: " + str(e)) from e
+
+
 def parse_where_substitutions(subs_node: ExprNode, parsing_params: SympyParsingConfig) -> SubstitutionList:
     """`[(name, expression), ...]` for each substitution in the substitution part of a WHERE criterion."""
-    return [(name, parse_expression(expr, parsing_params)) for name, expr in where_substitutions(subs_node)]
+    return [(name, parse_expression(expr, parsing_params)) for name, expr in where_substitution_strings(subs_node)]
 
 
 def check_criterion(criterion: ExprNode, parameters: EvaluationParameters) -> bool | None:
@@ -53,11 +61,17 @@ def create_expressions_for_comparison(
     lhs_expr = parse_expression(lhs, parsing_params).subs(local_substitutions).subs(reserved_expressions).subs(local_substitutions)
     rhs_expr = parse_expression(rhs, parsing_params).subs(local_substitutions).subs(reserved_expressions).subs(local_substitutions)
     if parsing_params.params.complex_numbers:
+        lhs_expr, rhs_expr = normalise_complex(lhs_expr, rhs_expr)
+    return lhs_expr, rhs_expr
+
+
+def normalise_complex(lhs_expr: Basic, rhs_expr: Basic) -> tuple[Basic, Basic]:
+    """Both sides written as `re + I*im` when either side has an imaginary part, otherwise unchanged."""
+    if (im(lhs_expr) != 0) or (im(rhs_expr) != 0):
         simplified_lhs_expr = lhs_expr.cancel().simplify()
         simplified_rhs_expr = rhs_expr.cancel().simplify()
-        if (im(lhs_expr) != 0) or (im(lhs_expr) != 0):
-            lhs_expr = real_part(simplified_lhs_expr) + I * im(simplified_lhs_expr)
-            rhs_expr = real_part(simplified_rhs_expr) + I * im(simplified_rhs_expr)
+        lhs_expr = real_part(simplified_lhs_expr) + I * im(simplified_lhs_expr)
+        rhs_expr = real_part(simplified_rhs_expr) + I * im(simplified_rhs_expr)
     return lhs_expr, rhs_expr
 
 

app/context/symbolic/graphs/substitution.py

@@ -4,12 +4,12 @@
 
 from sympy import Basic
 
-from lambdafeedback.criteria import CriteriaGraph, OutputNode, ReachedCriteria, where_substitutions
+from lambdafeedback.criteria import CriteriaGraph, OutputNode, ReachedCriteria
 from lambdafeedback.expression_parsing import parse_expression
 from lambdafeedback.slr_parsing import ExprNode
 
 from app.context.graph_helpers import attach_terminal_criterion, outcome_graph, outcome_tag
-from app.context.symbolic.comparison import check_equality, parse_where_substitutions
+from app.context.symbolic.comparison import check_equality, parse_where_substitutions, where_substitution_strings
 from app.context.symbolic.variations import one_addition_to_subtraction, one_exponent_flip, one_swap_addition_and_multiplication
 from app.evaluation_parameters import EvaluationParameters
 from app.types import Substitutions
@@ -62,8 +62,9 @@ def _group_values(
     Groups `expression_to_vary` and its variations by their value after `local_subs` are applied.
     Returns the expressions for each value, and the variation groups that produce each value.
     """
-    unvaried_value = expression_to_vary.subs(local_subs)
-    value_key = str(unvaried_value.simplify())
+    # Simplify here too, because the variations are simplified before they are compared with it
+    unvaried_value = expression_to_vary.subs(local_subs).simplify()
+    value_key = str(unvaried_value)
     values_and_expressions = {value_key: set([expression_to_vary])}
     values_and_variations_group = {value_key: set(["UNKNOWN"])}
     for (group_label, info) in variation_groups.items():
@@ -143,7 +144,7 @@ def get_candidates(_response: Any) -> ReachedCriteria:
 def criterion_where_node(criterion: ExprNode, parameters: EvaluationParameters, label: str | None = None) -> CriteriaGraph:
     parsing_params = parameters.parsing_parameters
     expression = criterion.children[0]
-    subs = where_substitutions(criterion.children[1])
+    subs = where_substitution_strings(criterion.children[1])
     local_subs = parse_where_substitutions(criterion.children[1], parsing_params)
     if label is None:
         label = criterion.content_string()

app/context/symbolic/variations.py

@@ -50,16 +50,19 @@ def addition_to_multiplication(node: Basic, k: int) -> Basic:
         return node - node.args[k - 1] - node.args[k] + node.args[k - 1] * node.args[k]
 
     def multiplication_to_addition(node: Basic, k: int) -> Basic:
-        return node - 2 * node.args[k]
+        return node / (node.args[k - 1] * node.args[k]) * (node.args[k - 1] + node.args[k])
 
     variations = replace_node_variations(expression, Add, addition_to_multiplication)
-    variations += replace_node_variations(expression, Mul, addition_to_multiplication)
+    variations += replace_node_variations(expression, Mul, multiplication_to_addition)
 
-    return variations
+    # For a node with two arguments, k-1 wraps around at k = 0, so each pair is changed twice
+    return list(dict.fromkeys(variations))
 
 
 def one_exponent_flip(expression: Basic) -> list[Basic]:
     def exponent_flip(node: Basic, k: int) -> Basic:
         return node**(-1)
     variations = replace_node_variations(expression, Pow, exponent_flip)
-    return variations
+
+    # The flip does not depend on k, so each power would be flipped once per argument
+    return list(dict.fromkeys(variations))

app/evaluation.py

@@ -243,6 +243,8 @@ def _build_parameters(context: Context, params: dict[str, Any], response: str, a
     (with `response` read as LaTeX if `is_latex` is set), the reserved keywords and the parsing parameters.
     """
     parameters = deepcopy(params)
+    # Applied to the copy, so the caller's `params` keep only what they set
+    _apply_tolerance_aliases(parameters)
 
     reserved_expressions_strings = {
         "learner": {
@@ -340,8 +342,6 @@ def evaluation_function(response: str, answer: str, params: dict[str, Any], incl
     strict_SI_syntax:
         - if set to True, use basic dimensional analysis functionality.
     """
-    _apply_tolerance_aliases(params)
-
     evaluation_result = EvaluationResult()
     evaluation_result.is_correct = False
 

app/tests/physical_quantity_evaluation_test.py

@@ -1,6 +1,9 @@
 import pytest
+from sympy import Integer, Symbol
 
 # Import necessary data and reference cases for tests
+from app import evaluation
+from app.context.physical_quantity.comparison import is_proportional
 from app.evaluation import evaluation_function
 from app.tests.physical_quantity_test_cases import slr_strict_si_syntax_test_cases, slr_natural_si_syntax_test_cases
 from lambdafeedback.units import (
@@ -554,8 +557,7 @@ def test_physical_quantity_with_rtol_and_imperial_mass_unit(self):
             ("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),
+            ("dimension(response) matches dimension(answer)", True),
             ("answer matches answer", True),
         ]
     )
@@ -568,8 +570,30 @@ def test_matches_criteria_other_than_response_and_answer_when_values_differ(self
             '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
+        assert result["is_correct"] is is_correct
+
+    @pytest.mark.parametrize(
+        "res,criteria,is_correct",
+        [
+            ("1.0 m", "dimension(response) matches dimension(answer)", True),
+            # Used to be false because the values were compared (bug 15 in the bug report)
+            ("2 m", "dimension(response) matches dimension(answer)", True),
+            ("2 km", "dimension(response) matches dimension(answer)", True),
+            ("2 s", "dimension(response) matches dimension(answer)", False),
+            ("2", "dimension(response) matches dimension(answer)", False),
+            ("2 m", "dimension(response) matches dimension(3 ft)", True),
+            ("2 m", "dimension(response) matches dimension(2 s)", False),
+        ]
+    )
+    def test_dimension_matches_compares_only_dimensions(self, res, criteria, is_correct):
+        params = {
+            'strict_syntax': False,
+            'physical_quantity': True,
+            'elementary_functions': True,
+            'criteria': criteria,
+        }
+        result = evaluation_function(res, "1.0 m", params)
+        assert result["is_correct"] is is_correct
 
     @pytest.mark.parametrize(
         "res,ans,criteria,is_correct",
@@ -616,6 +640,38 @@ def test_matches_criteria_against_written_out_quantity(self, res, criteria, is_c
         result = evaluation_function(res, "1 m", params)
         assert result["is_correct"] is is_correct
 
+    @pytest.mark.parametrize(
+        "lhs_value,rhs_value,expected",
+        [
+            (Integer(2000), Integer(2), (True, 1000.0)),
+            (Integer(2), Integer(2000), (True, 0.001)),
+            # Used to divide by zero (bug 2 in the bug report)
+            (Integer(2), Integer(0), (False, None)),
+            (Integer(0), Integer(2), (False, None)),
+            (Integer(0), Integer(0), (False, None)),
+            (Symbol("z"), Integer(2), (False, None)),
+        ]
+    )
+    def test_is_proportional(self, lhs_value, rhs_value, expected):
+        # Units and dimensions are never zero, so `evaluation_function` cannot reach these cases
+        x, y = Symbol("x"), Symbol("y")
+        assert is_proportional(x, y, [("x", lhs_value), ("y", rhs_value)]) == expected
+
+    @pytest.mark.parametrize("criteria", ["response matches answer", "response > answer"])
+    def test_criteria_graph_identifier(self, criteria, monkeypatch):
+        # The identifier is not in the serialised test data, so capture the graphs before feedback is generated
+        criteria_graphs = {}
+        generate_feedback = evaluation.generate_feedback
+
+        def capture_graphs(graphs, evaluation_parameters):
+            criteria_graphs.update(graphs)
+            generate_feedback(graphs, evaluation_parameters)
+
+        monkeypatch.setattr(evaluation, "generate_feedback", capture_graphs)
+        evaluation_function("2.1 m", "1 m", {"physical_quantity": True, "criteria": criteria})
+        # The matches graph used to have identifier None (bug 8 in the bug report)
+        assert criteria_graphs[criteria].identifier == criteria
+
     @pytest.mark.parametrize(
         "ans,res",
         [