Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Oxidize the ConsolidateBlocks pass #13368
Oxidize the ConsolidateBlocks pass #13368
Changes from all commits
ed2b41b
155a574
d3e900b
29bb569
e50fc1c
8da1b2f
61b831f
55523bb
51fa6a5
8c43f01
b93df76
0423d1c
f8841f2
700814e
5c4c50f
a422990
62df015
e001bab
73fae86
864fc51
11742b4
f1e645f
b00e22c
b5b0172
223bf2e
b6071ee
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To improve the memory efficiency here, we could simplify the handling of the
blocks
andruns
by unifying them withor_else
andunwarp_or_else
. This approach would reduces the unnecessary temporaryVec
allocations and leveragers iterators for more efficient memory usage.For instance, something like:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll see if I can come up with something for this. The behavior of cloning runs like this doesn't feel correct and I think probably wouldn't work correctly because it'll try to call
blocks_to_matrix
with a single qubit matrix. But I'll see if I can come up with something to avoid temporary vecs. I think I did this to make the borrow checker happy, but it was long enough ago I'm not 100% sure.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I see a path here, the conflict is on the type mismatch between
NodeIndex
and theusize
input. From a typing perspective rust won't be happy becausecollect_2q_runs()
returns aVec
ofNodeIndex
and we can't take that as an input. The best idea I had to avoid an extra allocation was to takeNodeIndex
on the input Vec but we can't define theFromPyObject
trait for a type not defined in qiskit.Your suggestion here doesn't actually avoid any allocations except if blocks is
Some(_)
, but at the cost of a second allocation forblocks
isNone
path. Theblocks
isNone
path is the more common path though because that's what we run in the preset pass managers, so I'd rather stick with the barecollect_2q_runs
call and have the conversion cost from theblocks
list if it's specified.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, I understand now, and that makes sense. You're correct about the typing conflict and the allocation cost trade-offs, especially since the blocks is
None
path is more common in the preset pass managers. Let’s stick with what you initially suggested with the barecollect_2q_runs
call, and handle the conversion cost from the blocks list if it's specified. Thanks for clarifying!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is perhaps more evidence that we ought to just use
usize
in any public interface of the circuit crate rather than the internal representation.(no action requested)