Skip to content

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

Draft
m-messer wants to merge 4 commits into
refactor/design-patternsfrom
fix/criteria-crashes
Draft

m-messer wants to merge 4 commits into
refactor/design-patternsfrom
fix/criteria-crashes

Conversation

@m-messer

Copy link
Copy Markdown
Member

Problem

Some criteria gave learners an error instead of feedback. These bugs came up during the context refactor (bug report). This is the first PR of the bug-fix stack, and it is stacked on #283.

  • Bugs 1 + 12: matches criteria whose sides weren't exactly response/answer crashed or applied the tolerance to the wrong values. Sets and None standard values also crashed _within_tolerance.
  • Bug 10: comparisons involving complex infinity (e.g. 1/0) sometimes crashed. Every comparison operator was evaluated, not only the one requested, so whether it crashed depended on hash randomisation.
  • Bug 14: order criteria such as response > 1.5 m raised TypeError (14a). A quantity written out in the criterion was not parsed with units (14b).
  • Bug 16: response matches 2 m was always false. 14b fixes this.
  • Bug 17: order criteria crashed for quantities without units ('Rational' * 'NoneType'). 14b fixes this.

Changes

  • _within_tolerance (app/context/physical_quantity/graphs/match.py): returns False unless a tolerance is set, the criterion is response/answer, and both standard values are single sympy expressions.
  • do_comparison_infinite (app/context/symbolic/comparison.py): looks up only the requested operator in COMPARISON_OPERATORS and evaluates it inside the try.
  • comparison_function (app/context/physical_quantity/comparison.py): returns None (unknown) when sympy cannot decide the truth value, instead of raising TypeError.
  • CriterionSides (app/context/physical_quantity/comparison.py): parse_criterion_sides now returns a frozen dataclass. A side written out as a quantity (INPUT label) is parsed with the quantity parser and stands in the expression as a criterion_lhs/criterion_rhs symbol. Its ParsedQuantity goes into sides.quantities next to the response and answer.
    • match.py (_quantity_match, _dimension_match, _unit_comparison) and order.py (comparison_base_graph) substitute from sides.quantities.
    • order.py compares a quantity without a unit by its value alone.
    • parsed_quantity is split out of parse_quantity in app/context/physical_quantity/parsing.py.

Checklist

  • Tests added/updated
  • Docs updated (app/docs.md) if user-facing behaviour changed: n/a, these are bug fixes to behaviour that is already documented

Testing

  • New regression tests:
    • test_matches_criteria_other_than_response_and_answer_when_values_differ, test_order_criteria_against_written_out_quantity and test_matches_criteria_against_written_out_quantity in physical_quantity_evaluation_test.py.
    • test_complex_infinity and test_do_comparison_infinite_only_evaluates_requested_comparison in symbolic_evaluation_test.py. The direct test is there because the end-to-end crash only happened on some runs.
  • Each regression test failed before its fix.
  • pytest app: 1241 passed, 2 skipped, 1 failed. The failure is test_radians_to_frequency, which already failed before this PR.
  • mypy app: no issues.

Still open in the bug report, for the next PRs in the stack: 2, 3, 4, 6, 7, 8, 9, 11, 13, 15, and the feedback strings.

🤖 Generated with Claude Code

…n, and handle edge cases for non-constant values; add targeted test coverage for criteria mismatches and tolerance checks.
…on_infinite` to evaluate only requested comparisons
…nd update `comparison_function` to return `None` for unresolved symbolic differences
…ass, simplify `parse_criterion_sides`, update match functions, and expand test coverage.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant