Fix factor of 2 in Rz operator in trotter_export_to_qgates_test.py#1380
Fix factor of 2 in Rz operator in trotter_export_to_qgates_test.py#1380arettig wants to merge 1 commit into
Conversation
A factor of 2 discrepancy between the definition of the Rz(theta) operator is corrected when exporting to QASM format.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the coefficient calculations for the C-Phase and Rz gates generated in pauli_exp_to_qasm within trotter_exp_to_qgates.py. Specifically, it doubles the coefficient for the Rz gate when no ancilla is used, and flips the signs for C-Phase and Rz gates when an ancilla is used. Corresponding unit tests in trotter_exp_to_qgates_test.py have been updated to reflect these new coefficient values. No review comments were provided, so there is no feedback to address.
|
It's weird how Gemini Code Assist is all over some PRs, and other ones it just kinda wipes its hands and says "eh, not my concern." |
mhucka
left a comment
mhucka
left a comment
There was a problem hiding this comment.
Thanks for doing this. I have some comments that are not critical, so I approved the PR and will leave it to you to address them (or not) before merging.
| ret_list = ret_list + [ | ||
| "Rz {} {}".format(1 * term_coeff * evolution_time, ancilla) | ||
| "Rz {} {}".format(-1 * term_coeff * evolution_time, ancilla) | ||
| ] | ||
| else: | ||
| ret_list = ret_list + [ | ||
| "Rz {} {}".format(1 * term_coeff * evolution_time, ancilla) | ||
| "Rz {} {}".format(-1 * term_coeff * evolution_time, ancilla) | ||
| ] |
There was a problem hiding this comment.
Can this code duplication be avoided somehow?
There was a problem hiding this comment.
Also, I'm not sure the -1 part is needed. It should work to write the more idiomatic form
-term_coeff * evolution_timeunless there's a reason for human readability or something else?
There was a problem hiding this comment.
One more thing: instead of somelist + [something], it's more efficient to do somelist.append(something). The + operator creates a new copy of the list, which append avoids.
However, if the rest of the file has many more instances of the same style of construct (ret_list = rel_list + […]), then I would say it's better to do what you did here and keep the same pattern. A separate PR can fix the inefficient constructs (and do it for other files, because if it happens here it probably happens elsewhere in the code base).
2 participants
Fixes #721 (supersedes #1343). A factor of 2 error in Rz gates is corrected when outputting a Trotter decomposition to QASM (for both the controlled and uncontrolled versions). The corresponding tests are also corrected.