Skip to content

Added support for brackets#263

Merged
m-messer merged 5 commits into
mainfrom
feature/support_bracket_types
Jul 23, 2026
Merged

Added support for brackets#263
m-messer merged 5 commits into
mainfrom
feature/support_bracket_types

Conversation

@m-messer

Copy link
Copy Markdown
Member

Problem

Only () was accepted as a valid bracket, causing parse failures for common textbook notation like [x+y]*z.

Limitations

{} is not supported in the feature. {} is already in use for "multiple acceptable answers" ({x+1, x-1} = either is correct), and {} is routinely used to denote sets.

Changes

  • expression_utilities.py: new convert_bracket_notation() maps [/](/). Called in preprocess_expression() (normalizes preview output) and parse_expression() (covers convention parsing + direct grading-path calls).
  • preview_test.py: added [] cases to test_brackets, expecting output normalized to ().

@peterbjohnson peterbjohnson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pasted Claude's list of potential issues below. I suggest we only make the substitution if not strict_syntax? And perhaps also a custom parameter to not substitute at all (sub_square_brackets?). You could then do the same with curly brackets, and they can be enabled/disabled - perhaps with the default as disabled?

Claude:

Interval notation: [a,b], (a,b), [a,b) all mean different things (closed/open bounds). Flattening everything to () throws away that distinction — sympy's Interval cares about it.

Matrix/list literals: [1,2,3] or [[1,2],[3,4]] are meant as a sympy list or Matrix. Turning them into (1,2,3) or nested tuples changes the type entirely, and sympify won't build a Matrix from it.

Indexing vs grouping: x[0] is array/element access, not grouping. Convert it and it becomes x(0) — a function call on x. If any of your inputs use bracket indexing, this silently changes meaning.

Naive string replace: swapping every [→( and ]→) doesn't respect nesting or existing parens. Use a stack to match bracket pairs properly, especially with mixed/nested ()/[] — otherwise you can get mismatched or misassociated groupings.

Empty brackets [] — decide what that should even mean before conversion.

@m-messer

m-messer commented Jul 22, 2026

Copy link
Copy Markdown
Member Author

Thanks for the comment. I'll comment on each element below:

. I suggest we only make the substitution if not strict_syntax? And perhaps also a custom parameter to not substitute at all (sub_square_brackets?). You could then do the same with curly brackets, and they can be enabled/disabled - perhaps with the default as disabled?

That's a great idea for excluding it from strict_syntax. Instead of two parameters for supporting bracket substitutions, we could have one and substitute both [] and {} with only one additional parameter?

Interval notation: is currently not supported by compareExpressions, [2,5) will not parse. Is this something we want to support or is it out of scope for this evaluation function?

Matrix/list literals: We have a matrix input that uses a separate evaluation function to handle matrices. Additionally, compareExpressions does not have native matrix support. Is this something we want to add or should we update arraySymbolicEqual to use compareExpressions?

Indexing vs grouping: This also isn't currently supported by compareExpressions

Naive string replace: I don't understand what claude is saying here. If we have an input [(x+y)*(z-y)] subsituting the [] with () would give ((x+y) * (z-y)) which is equivalent.

Empty brackets []: Currently, not supported

@m-messer

Copy link
Copy Markdown
Member Author

Context

The original review suggested gating the []() substitution behind strict_syntax, possibly with an additional dedicated parameter, and doing the same for {}.

Solution

Both [] and {} are now accepted as alternative grouping notation for (), gated behind strict_syntax — when strict_syntax is True, neither is substituted and the literal brackets are passed through to SymPy as before.

Use of {} around an answer expression already means multiple acceptable answers (e.g. "{1, 2, 3}"). So substitution is skipped automatically without needing further parameters.

Changes

  • convert_bracket_notation (expression_utilities.py) now replaces both [] and {} with (), still called only when strict_syntax is disabled.
  • Added is_multiple_answers_wrapper, which uses the existing find_matching_parenthesis helper to correctly detect whether a string's leading { genuinely matches its trailing } (as opposed to the two just happening to sit at each end of the string).
  • create_expression_set now uses is_multiple_answers_wrapper for its multi-answer detection instead of the old naive check.
  • Added tests covering: []/{} grouping conversion, non-conversion under strict_syntax, the fixed multi-answer detection edge case, and a regression test confirming genuine multi-answer {} syntax still works under both strict_syntax values.

@peterbjohnson

Copy link
Copy Markdown
Member

Thanks, good initiative. I agree with all of this (can't find an 'approve' button.

For matrices, there will be times when we need to handle the whole matrix, not just sequentially test corresponding cells in an outer loop. That's a future requirement. But we do anticipate preceding arrays in future.

@m-messer
m-messer merged commit 30f0b02 into main Jul 23, 2026
1 check passed
@m-messer
m-messer deleted the feature/support_bracket_types branch July 23, 2026 08:02
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.

2 participants