Skip to content

fix: keep every bound in a chained comparison - #124

Merged
rmshaffer merged 5 commits into
amazon-braket:mainfrom
MohammedAlkindi:fix/chained-comparisons
Oct 1, 2026
Merged

rmshaffer merged 5 commits into
amazon-braket:mainfrom
MohammedAlkindi:fix/chained-comparisons

Conversation

@MohammedAlkindi

Copy link
Copy Markdown
Contributor

Issue #, if available: none filed.

Description of changes:

Python parses a < b < c as one Compare node holding every operator, but visit_Compare read only ops[0] and comparators[0]. Every bound after the first was discarded, so the emitted OpenQASM silently implemented different logic than the source. On main if 0 <= n < 10 with n = 1000 emits h __qubits__[0]; unconditionally; it now emits x.

The fix walks the chain and folds the pairs with ag__.and_, the helper the upstream logical_expressions pass already uses. Single and mixed chains are unchanged.

Testing done:

494 passed, 2 xfailed. Reverting only the source and keeping the two new tests leaves exactly those two failing, at 492 passed. ruff is clean apart from five pre-existing unused-noqa findings present either way.

Merge Checklist

General

  • Read the CONTRIBUTING doc
  • Used the commit message format from CONTRIBUTING
  • Updated documentation (not applicable)

Tests

  • Added tests that prove the fix is effective
  • Tests are not configured for a specific region or account

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Python parses a < b < c as one Compare node carrying every operator, but visit_Compare read only ops[0] and comparators[0], so each bound after the first was discarded and the emitted OpenQASM silently implemented different logic than the source. Iterate the whole chain and fold the pairs with ag__.and_, matching how the upstream logical_expressions pass decomposes chains for operators it overloads. Mixed chains still fall through to that pass unchanged.
@MohammedAlkindi
MohammedAlkindi requested a review from a team as a code owner September 14, 2026 15:17
@codecov

codecov Bot commented Sep 14, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (bcc6723) to head (a02c0d6).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #124   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           52        52           
  Lines         2290      2296    +6     
  Branches       266       268    +2     
=========================================
+ Hits          2290      2296    +6     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@rmshaffer rmshaffer 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.

Thank you for taking the initiative to fix this! I think it's in the right direction; just left a couple comments that I'd appreciate if you could address.

Comment thread src/autoqasm/converters/comparisons.py Outdated
Comment thread test/unit_tests/autoqasm/test_operators.py Outdated
Comment thread test/unit_tests/autoqasm/test_operators.py
Addresses review feedback: moves the rationale out of the docstring, folds the end-to-end regression case into test_comparison_ops_py, and adds coverage for a four-operator chain and for a chain containing an equality operator. Both new cases fail against the unmodified converter.
@MohammedAlkindi

Copy link
Copy Markdown
Contributor Author

All three are in 9121ce0, which I pushed without commenting, so this still shows as blocked.

Docstring rationale removed, and the standalone test is now the i/j lines inside test_comparison_ops_py.

Both chains you named are covered, using your examples. Reverting comparisons.py to main and keeping the new tests:

4 failed, 2 passed
  test_comparison_chained
  test_comparison_chained_multiple_operators      # 4 < a <= b <= c < 8
  test_comparison_chained_with_equality_operator  # 4 < a < b == c < 8
  test_comparison_ops_py

Restoring it: 6 passed. So both do depend on the change.

ruff check src clean. Local only: no Actions run has appeared on this PR, just readthedocs.

@MohammedAlkindi

Copy link
Copy Markdown
Contributor Author

Correction to my last line, I read the rollup too quickly. The four workflows did run on 9bfbaa8 and all passed. On 9121ce0 they are sitting at action_required:

Python package                    action_required
Check code format                 action_required
Code Freeze                       action_required
Check long description for PyPI   action_required

So they are waiting on your approval to run, not missing. Sorry for the noise.

The two chain tests asserted only that each comparison appeared somewhere in
the IR, which passes even if the operands are folded together wrongly. Assert
the full expected IR, matching how the rest of this file tests generated code,
so a mis-folded chain fails.

Also extend test_comparison_ops_py with a three-operator chain whose last bound
is the failing one, so dropping a late bound cannot pass unnoticed.
@MohammedAlkindi

Copy link
Copy Markdown
Contributor Author

All three addressed, thanks for the review.

Worth flagging from the validation you asked for: 4 < a < b == c < 8 is not merely working already. On main it compiles to a > 4 alone — visit_Compare reads ops[0] and comparators[0] and discards the rest of the chain, so the other three bounds are dropped with no error. That test is covering a real silent drop rather than confirming existing behaviour.

Both chain tests now assert the full expected IR instead of substrings, since a substring check passes even when the operands are folded together wrongly. test_comparison_ops_py also gained a three-operator chain whose failing bound is the last one.

@MohammedAlkindi

Copy link
Copy Markdown
Contributor Author

This and #125 are both approved and green, but main has moved one commit since (#126, the codecov-action bump), so both now show as behind. Should I update the branches, or is anything else needed before merging?

@rmshaffer
rmshaffer merged commit 7a6c9b3 into amazon-braket:main Oct 1, 2026
13 checks passed
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