-
Notifications
You must be signed in to change notification settings - Fork 177
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
Change __global__
register bit ordering for target mode to match library mode
#1027
Change __global__
register bit ordering for target mode to match library mode
#1027
Conversation
* Mapping updates to retain measurement order (which is critical for keeping ordering the same) * Circuit Simulator updates to populate global register in QIR result order instead of in alphabetical order
CUDA Quantum Docs Bot: A preview of the documentation can be found here. |
CUDA Quantum Docs Bot: A preview of the documentation can be found here. |
Thanks for looking into this Ben. I appreciate the added test and the docs update, they were very helpful. My one question - why order the global register by the order of the measurements? The original thought behind the global register was that it would represent the final classical state of user-specified measured qubits (or all if none provided). The ordering was meant to be the same ordering as the qubits (vector-like ordering). One concern I would have is that the two equivalent circuits you demonstrate in the docs update produce two different results and this may be a point of confusion later on (despite the well-laid out explanation in the docs). Let me know if I'm missing something or likely there was a bug in the initial implementation. |
I didn't change the behavior of the global register in library mode in this PR ... I just documented the behavior and made non-library mode match it. If you're saying that the currently library mode ordering is a bug, then we should probably open a new issue and decide what to do about the docs updates in this PR. They were intended capture the current state of the code, but if we anticipate changing that, then we need to decide whether or not we would a) publish/push the additional docs in this PR to describe the current state and just update the docs if/when we change the behavior in the future, or b) remove the docs update from this PR and capture them separately after any potential ordering changes are made. I'm open to either of those options (or any additional options, too), but I tend to favor (a) since it documents the current behavior. |
Awesome, this is what I was sort of starting to think. I'm a proponent of fixing the bug in CircuitSimulator and ensuring that the ordering corresponds to the ordering of the allocated qubits. It might also be good in a future patch to explicitly store the qubit to bit mapping in sample_result. |
OK, it sounds like I should move this PR back to "draft" and open a new PR to make library mode perform as you describe first. It may require some rework on this PR because this PR is ordering things based off of QIR result number in the backends, so we may need to change it to be based off of QIR qubit number if we make the change to CircuitSimulator. |
CUDA Quantum Docs Bot: A preview of the documentation can be found here. |
Co-authored-by: Thien Nguyen <58006629+1tnguyen@users.noreply.github.com>
CUDA Quantum Docs Bot: A preview of the documentation can be found here. |
CUDA Quantum Docs Bot: A preview of the documentation can be found here. |
CUDA Quantum Docs Bot: A preview of the documentation can be found here. |
CUDA Quantum Docs Bot: A preview of the documentation can be found here. |
CUDA Quantum Docs Bot: A preview of the documentation can be found here. |
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.
LGTM 👍
CUDA Quantum Docs Bot: A preview of the documentation can be found here. |
Regarding #978, this change makes the
__global__
register behave the same in target mode (both real backends and emulation) as library mode. To do this, the following changes were made:__global__
register based on alphabetically named results,which means they are now in measurement order of the original program. Due to Fix library mode qubit ordering bug. #1044 they are now in qubit allocation order.iqm_IQMTester.executeNoMeasurementsProgram
test because it was expecting an exception to be thrown if there were no measurements in an IQM program, but since measurements were implicitly added by the mapping pass, this no longer occurs. (This matches the behavior in library mode, so I believe this is a good change.)__global__
register in library mode vs non-library mode(s)__global__
register.Note that this PR will not fully resolve the above issue because named registers still exist in the returned
sample_result
, and that issue correctly notes that those named registers are not currently present in library mode.Potential items for further discussion
delay-measurements
pass can change whether or not the final operation is a measurement. This, in turn, can affect how the global register is formed. There is a commented-out test that demonstrates this behavior.Detailed implementation notes
cudaq::observe
is being called. Ifcudaq::observe
is being called, the measurements are injected in theObserveAnsatz
pass.ObserveAnsatz
pass may receive see a kernel with measurements introduced by the mapping pass. Forobserve
, it is critical that not all qubits are measured and reported back to the user, so we have to REMOVE the measurements that mapping added and add only the ones that are desired forobserve
.