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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 6 commits into from
Jan 14, 2025
Merged

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

merged 6 commits into from
Jan 14, 2025
+80 −21

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

@Cryoris @alexanderivrii
fix rustiq
91dd9c2 
Co-authored-by: Alexander Ivrii <[email protected]>
@coveralls
Copy link

coveralls commented Jan 8, 2025 •
edited
Loading

Pull Request Test Coverage Report for Build 12742676629

Details

  • 10 of 12 (83.33%) changed or added relevant lines in 3 files are covered.
  • 9 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.004%) to 88.923%
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/qasm2/src/lex.rs 3 92.98%
crates/qasm2/src/parse.rs 6 97.62%
Totals Coverage Status
Change from base Build 12710188694: 0.004%
Covered Lines: 79433
Relevant Lines: 89328
💛 - Coveralls

@Cryoris Cryoris mentioned this pull request Jan 10, 2025
Closed
@Cryoris Cryoris linked an issue Jan 10, 2025 that may be closed by this pull request
Hamiltoninan variational ansatz not working when considering Pauli-strings with only identity operators #13644
Closed
ElePT
ElePT reviewed Jan 10, 2025
View reviewed changes
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.

crates/accelerate/src/circuit_library/pauli_evolution.rs Outdated Show resolved Hide resolved
ShellyGarion
ShellyGarion reviewed Jan 12, 2025
View reviewed changes
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)?

@Cryoris
Copy link
Contributor Author

Cryoris commented Jan 13, 2025

@ShellyGarion it certainly doesn't hurt -- added in 9254a32 🙂

ShellyGarion
ShellyGarion approved these changes Jan 14, 2025
View reviewed changes
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.

LGTM! thanks

ElePT
ElePT approved these changes Jan 14, 2025
View reviewed changes
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 too :)

@alexanderivrii alexanderivrii added this pull request to the merge queue Jan 14, 2025
Merged via the queue into Qiskit:main with commit b98e0d0 Jan 14, 2025
17 checks passed
mergify bot pushed a commit that referenced this pull request Jan 14, 2025
@Cryoris @mergify
Fix PauliEvolutionGate (using product formulas) for all-identity Pa…
e1135e3 
…uli terms (#13634)

* fix PauliEvo for all identities

* fix rustiq

Co-authored-by: Alexander Ivrii <[email protected]>

* fix docs

* regression test 13644

---------

Co-authored-by: Alexander Ivrii <[email protected]>
(cherry picked from commit b98e0d0)
@mergify mergify bot mentioned this pull request Jan 14, 2025
Merged
github-merge-queue bot pushed a commit that referenced this pull request Jan 14, 2025
@mergify @Cryoris
Fix PauliEvolutionGate (using product formulas) for all-identity Pa…
fcb0cdd 
…uli terms (#13634) (#13658)

* fix PauliEvo for all identities

* fix rustiq

Co-authored-by: Alexander Ivrii <[email protected]>

* fix docs

* regression test 13644

---------

Co-authored-by: Alexander Ivrii <[email protected]>
(cherry picked from commit b98e0d0)

Co-authored-by: Julien Gacon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ElePT ElePT ElePT approved these changes

@ShellyGarion ShellyGarion ShellyGarion approved these changes

@alexanderivrii alexanderivrii Awaiting requested review from alexanderivrii alexanderivrii is a code owner

Assignees
No one assigned
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
Milestone
1.3.2
6 participants
@Cryoris @qiskit-bot @coveralls @ShellyGarion @ElePT @alexanderivrii