Repository navigation
Conversation
Argus reviewAuto-review is off for this repo. Tick the box below to run a review on this PR.
Estimated cost
Tip: you can also comment |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the
 |
5abfdca to
17495c7
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| return (None, []) | ||
|
|
||
| statements.extend(rhs_statements) | ||
| if lhs_value is None or rhs_value is None: |
There was a problem hiding this comment.
Are you sure that these values are None only in the case of stretch operands? What if they are None in some other operation and we raise this arithmetic stretch operand error?
There was a problem hiding this comment.
No, not only stretch: 1 + 2 * f(3) with an extern f and 1 + g() with a subroutine that returns nothing also reach that line with a None operand (both still give the same TypeError as on main). The check no longer looks at lhs_value/rhs_value, it only fires when an operand is an identifier declared as stretch in scope whose value is still None, so those cases fall through untouched. The None part is still needed because const stretch s = 100ns; has a value and a + 2 * s evaluates fine with it on main, which test_const_stretch_arithmetic now covers.
| return var_value | ||
|
|
||
| @classmethod | ||
| def _find_stretch_operand(cls, expression): |
There was a problem hiding this comment.
I think it would be easier to just check the scope manager for the variable rather than recursively going into the expression again. We already register stretch declarations in the visitor-
Line 1891 in b4ca301
You can just do a ref check AND type check with _check_var_type
There was a problem hiding this comment.
Changed in 5581df0: _find_stretch_operand is gone, and the binary branch now looks up each Identifier operand with get_from_visible_scope and checks it with Qasm3Validator.validate_variable_type(var, StretchType), the same check _check_var_type runs (calling _check_var_type directly would raise on the non-stretch operand instead). Nested cases like a + 2 * s are still caught when the inner 2 * s is evaluated, so I added that one to the test. I also rebased on main to clear the CHANGELOG conflict.
There was a problem hiding this comment.
@SAY-5 thanks for working on this ! Apologies for the delay in reviewing.
The idea is correct, but I think it'll be better to re-use the existing methods rather than adding a special handler for stretch name parsing
Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
17495c7 to
5581df0
Compare
| assert cls.visitor_obj | ||
| var = cls.visitor_obj._scope_manager.get_from_visible_scope(operand.name) | ||
| # const stretch declarations carry a value, so only reject unresolved ones | ||
| if Qasm3Validator.validate_variable_type(var, StretchType) and var.value is None: |
There was a problem hiding this comment.
I should've caught this already but I'm only realizing it now - can we rather just update the _check_var_initialized method?
It already has a branch for the stretch type, so we can add this change there. I think this will be even cleaner than adding this single stretch type handling in the BinaryExpression branch
TheGupta2012
left a comment
TheGupta2012
left a comment
There was a problem hiding this comment.
Hi @SAY-5 thanks for the change! Your update makes me see a further simplification, can you please address it? It should be good to merge post that
Summary of changes
Closes #389.
A
durationinitialized from arithmetic involving astretchoperand(
duration b = a + s;,2 * s,a + 2 * s) raised a bareTypeError: unsupported operand type(s) for +: 'float' and 'NoneType'. Astretchcarries no value until schedule time, so
_check_var_initializeddeliberately letsit stay
None; thatNonethen reachedqasm3_expression_op_mapinevaluate_expressionand blew up in Python arithmetic. A barestretch s;anddelay[s] q[0];were unaffected, so the gap was confined to arithmetic.Full symbolic stretch arithmetic is a larger piece of work, so this takes the
minimum acceptable path from the issue: when a binary expression has a
Noneoperand that traces back to a
stretchvariable, raise aValidationErrornamingthe variable and its span instead of letting the
TypeErrorescape. Non-stretchNoneoperands fall through unchanged, and pure duration arithmetic is untouched.Verified locally
tests/qasm3/declarations/test_classical.pypasses, including two new cases undertest_stretch_arithmetic_not_supported.test_delay.pyandtest_box.pypass aswell.
black,isort,pylintandmypyare clean on the touched files.