Skip to content
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

Fix PauliEvolutionGate (using product formulas) for all-identity Pauli terms #13634

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Cryoris
Copy link
Contributor

@Cryoris Cryoris commented Jan 8, 2025

Summary

The PauliEvolutionGate, if used with a product formula synthesis (this is the default),
did not correctly handle all-identity terms in the operator:

Details and comments

Some minor corrections to docs are included.

@Cryoris Cryoris added stable backport potential The bug might be minimal and/or import enough to be port to stable Changelog: Bugfix Include in the "Fixed" section of the changelog Rust This PR or issue is related to Rust code in the repository labels Jan 8, 2025
@Cryoris Cryoris added this to the 1.3.2 milestone Jan 8, 2025
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

  • @Cryoris
  • @Qiskit/terra-core
  • @ajavadia

Co-authored-by: Alexander Ivrii <[email protected]>
@coveralls
Copy link

coveralls commented Jan 8, 2025

Pull Request Test Coverage Report for Build 12709550670

Details

  • 10 of 12 (83.33%) changed or added relevant lines in 3 files are covered.
  • 12 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.002%) to 88.911%

Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/circuit/src/operations.rs 3 5 60.0%
Files with Coverage Reduction New Missed Lines %
crates/accelerate/src/unitary_synthesis.rs 1 93.18%
crates/qasm2/src/lex.rs 5 92.98%
crates/qasm2/src/parse.rs 6 96.69%
Totals Coverage Status
Change from base Build 12707746058: 0.002%
Covered Lines: 79425
Relevant Lines: 89331

💛 - Coveralls

Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! There is just a conflict in the test file and a minor docs suggestion.

/// I_q0 X_q1 Y_q2 Z_q3 and will use a RZ rotation angle of 0.4.
/// ``(pauli_string, qubit_indices, rz_rotation_angle)``. An element of the form
/// ``("XIYZ", [0,1,2,3], 2)``, for example, is interpreted in terms of qubit indices as
/// I_q0 X_q1 Y_q2 Z_q3 and will use a RZ rotation angle of 2.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that you reordered the pauli string abovem shouldn't it be:

Suggested change
/// I_q0 X_q1 Y_q2 Z_q3 and will use a RZ rotation angle of 2.
/// X_q0 I_q1 Y_q2 Z_q3 and will use a RZ rotation angle of 2.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, yes -- good catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the circuit doesn't match that string -- both fixed in 55a5357!

Copy link
Member

@ShellyGarion ShellyGarion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a point to add another test based on #13644? (calling hamiltonian_variational_ansatz)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog Rust This PR or issue is related to Rust code in the repository stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
5 participants