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 Commutation Analysis #12995
Oxidize Commutation Analysis #12995
Changes from 116 commits
f421a26
7425d18
a7016b0
861b548
d310746
878e3d6
f01ddfa
4bd7391
c60bb8d
24659c5
1c3521e
bab0ee0
7e52136
665c6b3
ce512b2
cc2ba14
e6c0637
b4c2ee3
b24ae43
e34c3ec
1cbbf84
85041c7
e878608
f7de4e4
2a4c864
b59499a
f5bb696
93b2927
c911205
1e4e6f3
463b86a
01c06e5
f6b27ff
0e62ad0
aaf38b9
28f6de1
11c52ab
d68743f
e07f3d5
30a4a1a
6dec768
1de3290
5c6f006
b90b660
9a6f953
7970e3d
f317077
701c980
464e87b
7f1b451
6aab4a6
e5e57c6
584ee9b
bdeb5f6
1d249ab
40c15ec
39586ba
6c5e1e9
5c9d285
db89660
e9e4a27
6a5f389
de1ee92
eeded61
a0501d3
1225061
21dec35
e37c5b0
c0adbb8
d76fd80
22e0044
ee3f4d3
454208c
847b425
3724873
cd9c119
5266807
53c902c
6e33d38
2b45b04
18c406d
b42f7e4
c564b80
6292e93
8f0f7d2
6636860
2504d1c
52cf8bf
ac75c91
816f5eb
6e8f779
d312791
f4bfa9e
29f1c07
c554666
0041098
a05df8c
f231c83
2bbf36b
f8afd5d
ea09d48
62d2cdb
71ac2fd
12d0a05
9235004
af74cb9
d625539
e19f7c6
52ba2e0
6835c8f
c7dde73
34a8a14
d7cf2cc
8b49c19
7207998
c08cfae
7073570
68f4f4b
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.
Not that it matters, but I'd have done this with:
outside the function definition because it is never changed. But realistically the compiler will likely end up with the same generated code in both cases so it doesn't matter.
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.
Still consistency is nice -- I'll move it outside 👍🏻
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 a probably a good candidate for parallelization. We can naively make this a parallel iterator for the outer loop like:
We'll probably have to play some games around synchronization, although we can probably change it into an iterator collector. But we should do this in a follow up because it might require some deeper changes (as the compiler points them out).
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.
Yeah good point -- I'll have a look in parallel 🙂 I was trying to propagate the Result coming from
commute_inner
but if we collect then we can still do that 👍🏻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.
Realistically the
BitData
you're accessing here is the cache. It's basically just doing a vec lookup and returning the python object.That being said I'm wondering can wire here be anything besides a
Qubit
? Not that it really matters because this code is correct and will handle the other cases as expected.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.
No you're right, it cannot, since we explicitly only iterate over
0..dag.num_qubits().map(|i| Wire::Qubit(i))
. We can change the code to expect a Qubit, that might be a bit cleaner to the readersThere 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 was wondering what happens if a dag has non-continuous qubits, i.e. gaps in the set of qubits such as qubits 0...3 and 6..10 with no qubits at index 4, 5 - can this happen and was this supported in the Python implementation?
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.
The indices can't ever be non-continuous in Rust or Python. They're literally the index in the
list
/Vec
fordag.qubits
. So if you remove a qubit everything has a shift penalty. The rust code just makes this a bit more explicit than it ever was in Python because there was a layer of indirection around it in Python with the bit objects.