Add feat(transpiler): add UnrollBoxes pass #15400
Conversation
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 20022336002Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
|
Hi Debasmita, thanks for doing this! The code is very clean and mostly LGTM. I had a question though. |
| self.known_annotations = known_annotations or (lambda ann: True) | ||
| self.max_depth = max_depth | ||
|
|
||
| def _validate_annotations(self, box_op: BoxOp, depth: int = 0) -> bool: |
There was a problem hiding this comment.
What's the use of the depth argument exactly? I assume that it's to not unroll BoxOps that exceed a certain depth. Will this work fine for the recursive case with multiple nested Boxes? Also I don't see it being passed in the .run(self,dag) function. Is this supposed to be called by the user as well?
There was a problem hiding this comment.
Thank @Shobhit21287 . The depth argument was intended for a future extension where UnrollBoxes would stop unrolling once a configurable nesting depth (max_depth) is reached. Right now _validate_annotations is always called without a depth, so the parameter is effectively unused and recursive calls always see depth == 0
There was a problem hiding this comment.
anyways I removed it thanks @Shobhit21287
|
|
||
| def __init__( | ||
| self, | ||
| recursive: bool = True, |
There was a problem hiding this comment.
Hi, thank you for your contribution. I had a couple of clarifications regarding this PR. There is a recursive flag being used over here, and the value is being set. However, I don't see it being reused anywhere else in code. Has it been set for future use? If yes, would you say it makes more sense to keep this addition in a future PR rather than this one?
There was a problem hiding this comment.
Yep that recursive wasn’t used anywhere. It was left over from an earlier prototype, I have removed the recursive and max_depth parameters and now only expose known_annotations; if we decide we need a configurable recursion limit later, I can add that in a follow‑up PR with tests
|
|
||
| for ann_dict in getattr(box_op, "annotations", []): | ||
| if not self.known_annotations(ann_dict): | ||
| return False |
There was a problem hiding this comment.
can we add a test case here to test behavior on passing unknown annotations? To ensure that this callable returns False, but the :class:BoxOp remains in the circuit?
There was a problem hiding this comment.
I’ve added test_unknown_annotation_keeps_box, which sets an unknown annotation and a known_annotations predicate that only accepts ‘safe’; the test asserts that the BoxOp is preserved in this case.
| class UnrollBoxes(TransformationPass): | ||
| """Unroll BoxOp operations by replacing them with their internal circuit body.""" | ||
|
|
||
| def __init__( |
There was a problem hiding this comment.
I think there is merit in writing a short doctstring here, defining the input arguments, their types and short descriptions for the :meth:__init__. Since this is a user-facing functionality.
There was a problem hiding this comment.
Thanks, that makes sense. I havee added a short constructor docstring for UnrollBoxes.init describing the known_annotations argument and how it controls which boxes are safe to unroll.
aaryav-3
left a comment
aaryav-3
left a comment
There was a problem hiding this comment.
Thank you so much for your contribution, this PR is minimally and effectively written to introduce the new transpilar pass, I just had a couple of doubts and clarifications before it is good to go
…kit license headers for UnrollBoxes
5 participants
Implement a new UnrollBoxes transpiler pass to replace BoxOp “scaffolding” with the gates from the boxed circuit body.
Support nested boxes via @trivial_recurse, so boxes inside boxes are unrolled correctly.
Add an annotation-aware API (known_annotations: Callable[[dict], bool]) so callers can control which annotations are safe to ignore during unrolling.
Fixes Transpiler pass to replace boxes by their contents? #14468