Restore previous behavior to PolynomialTensor.rotate_basis() by default#1418
Conversation
PolynomialTensor's rotate_basis method previously used the transpose of the rotation matrix. This is not correct, but changing it will break users code. A transpose flag is introduced to specify whether or not to transpose the rotation matrix, with True being the default for now. A deprecation warning is given if the transpose flag is not specified.
![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 introduces a transpose parameter to the rotate_basis method in PolynomialTensor and InteractionRDM classes. By default, transpose is set to None, which triggers a DeprecationWarning and defaults to True to maintain backwards compatibility. When transpose=True, the rotation matrix is transposed before being applied. The existing tests and usages have been updated to explicitly set transpose=False where appropriate, and new tests have been added to verify the transpose behavior and the deprecation warning. There are no review comments, so I have no additional feedback to provide.
mhucka
left a comment
mhucka
left a comment
There was a problem hiding this comment.
Thanks for doing this so quickly. Here are some minor change requests. Overall looks good!
Changes the DeprecationWarning for using rotate_basis with no transpose argument to the more proper FutureWarning. Also fixes some minor typos and adds typing to functions.
general_rotate_basis is the function underlying the rotate_basis methods. This function is explicitly used by ReCirq, so restoring the default behavior of the rotate_basis methods is not enough to prevent breaking codes. general_rotate_basis is now itself modified to transpose the rotation matrix by default.
Add the transpose parameter to rotate_basis() when used in the jupyter notebook to avoid any warnings.
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
mhucka
left a comment
mhucka
left a comment
There was a problem hiding this comment.
Looks great! Thanks for doing this.
2 participants
PolynomialTensor's rotate_basis method previously used the transpose of the rotation matrix. This is not correct, but changing it in #805 broke users' code. A
transposeflag is introduced to specify whether or not to transpose the rotation matrix, withTruebeing the default for now (to be changed to the preferredFalsein a later release). A deprecation warning is given if the transpose flag is not specified.