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

@@ -7,7 +7,7 @@
 from sympy.core.relational import Relational
 from sympy.logic.boolalg import BooleanAtom
 
-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
 
@@ -25,9 +25,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 CriteriaSyntaxError("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:
@@ -55,11 +63,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

@@ -286,6 +286,8 @@ def _build_parameters(context: Context, params: dict[str, Any], response: str, a
     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": {
@@ -462,8 +464,6 @@ def enter_stage(name: str) -> None:
         stage["name"] = name
         log_event("stage_start", stage["eval_id"], stage=name, elapsed_ms=elapsed_ms(stage["start_time"]))
 
-    _apply_tolerance_aliases(params)
-
     evaluation_result = EvaluationResult()
     evaluation_result.is_correct = False
 
@@ -496,7 +496,11 @@ def enter_stage(name: str) -> None:
 
     # Performs evaluation of response
     enter_stage("generate_feedback_procedures")
-    feedback_procedures = context.feedback_procedure_generator(evaluation_parameters)
+    try:
+        feedback_procedures = context.feedback_procedure_generator(evaluation_parameters)
+    except CriteriaSyntaxError as e:
+        # e.g. a WHERE substitution that is not an equality, which is only found when its graph is built
+        raise TaskConfigurationError(str(e)) from e
     for graph in feedback_procedures.values():
         _disable_evaluation_nodes(graph, evaluation_parameters.disabled_evaluation_nodes)
     enter_stage("generate_feedback")