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

Implement Qdrift as ProductFormula #7281

Merged
merged 20 commits into from
Nov 29, 2021
Merged

Implement Qdrift as ProductFormula #7281

merged 20 commits into from
Nov 29, 2021

Conversation

anedumla
Copy link
Contributor

Summary

This pull request implements the QDrift method of Trotterization as a product formula and fixes Issue #7218.

Details and comments

The implementation is virtually the same as in https://github.com/Qiskit/qiskit-terra/blob/main/qiskit/opflow/evolutions/trotterizations/qdrift.py with the only change being that

self.sampled_ops = algorithm_globals.random.choice(
            np.array(scaled_ops, dtype=object),
            size=(int(N * self.reps),), p=weights / lambd,
        )

becomes

self.sampled_ops = algorithm_globals.random.choice(
            np.array(scaled_ops, dtype=object),
            size=(int(np.ceil(N * self.reps)),),
            p=weights / lambd,
        )

to make sure that the sample size is always at least 1 and because N is chosen to satisfy an error bound, so the former int(N * self.reps) would not have been strictly correct in some instances.

@anedumla anedumla requested a review from a team as a code owner November 17, 2021 16:47
@coveralls
Copy link

coveralls commented Nov 17, 2021

Pull Request Test Coverage Report for Build 1515324749

  • 43 of 46 (93.48%) changed or added relevant lines in 6 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.007%) to 82.857%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/synthesis/evolution/lie_trotter.py 3 4 75.0%
qiskit/synthesis/evolution/qdrift.py 29 31 93.55%
Totals Coverage Status
Change from base Build 1509385150: 0.007%
Covered Lines: 50147
Relevant Lines: 60522

💛 - Coveralls

qiskit/synthesis/evolution/qdrift.py Outdated Show resolved Hide resolved
qiskit/synthesis/evolution/qdrift.py Outdated Show resolved Hide resolved
qiskit/synthesis/evolution/suzuki_trotter.py Outdated Show resolved Hide resolved
@anedumla anedumla requested a review from dlasecki November 22, 2021 09:43
qiskit/synthesis/evolution/qdrift.py Outdated Show resolved Hide resolved
qiskit/synthesis/evolution/qdrift.py Outdated Show resolved Hide resolved
qiskit/synthesis/evolution/qdrift.py Outdated Show resolved Hide resolved
qiskit/synthesis/evolution/qdrift.py Outdated Show resolved Hide resolved
qiskit/synthesis/evolution/qdrift.py Outdated Show resolved Hide resolved
qiskit/synthesis/evolution/qdrift.py Outdated Show resolved Hide resolved
test/python/circuit/library/test_evolution_gate.py Outdated Show resolved Hide resolved
test/python/circuit/library/test_evolution_gate.py Outdated Show resolved Hide resolved
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

One super minor comment, but otherwise this looks fine to accept to me.

qiskit/synthesis/evolution/qdrift.py Outdated Show resolved Hide resolved
jakelishman
jakelishman previously approved these changes Nov 26, 2021
@jakelishman jakelishman added automerge Changelog: New Feature Include in the "Added" section of the changelog synthesis labels Nov 26, 2021
@jakelishman jakelishman added this to the 0.19 milestone Nov 26, 2021
@jakelishman jakelishman dismissed dlasecki’s stale review November 26, 2021 19:35

All changes have been marked resolved by Dariusz

@jakelishman
Copy link
Member

jakelishman commented Nov 26, 2021

I originally had this as automerge (trying to get things merged before the Monday/Tuesday rush), but then I realised that @dlasecki had never actually commented on whether the algorithm does exactly what it's supposed to, so I don't know if his original review was meant to be a complete check of everything he thought needed changing. I don't know enough about this particular expansion to approve that properly, so I'll wait for Dariusz to give his tick as well before merging.

@@ -113,6 +118,27 @@ def test_suzuki_trotter_manual(self):

self.assertEqual(evo_gate.definition.decompose(), expected)

@data(
(X + Y, 0.5, 1, [(Pauli("X"), 0.5), (Pauli("X"), 0.5)]),
(X, 0.238, 2, [(Pauli("X"), 1.050420168067227)]),
Copy link
Contributor

Choose a reason for hiding this comment

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

This test fails for me locally (the second one only) and I don't quite get why the coefficients of the sampled ops should be different than 1 since we now don't change the coeffs anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why it fails for you, maybe it's got something to do with the random seed? The coefficients should be the same but not one (N here is num_gates for us):
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah maybe making num_gates to an int makes the test fail

@Cryoris
Copy link
Contributor

Cryoris commented Nov 27, 2021

@anedumla I added a functional test to see if it works as expected and had to change the num_gates to an int (I think in the paper they use an int too).

Copy link
Contributor

@Cryoris Cryoris 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 for the implementation!

@mergify mergify bot merged commit 4a94b77 into Qiskit:main Nov 29, 2021
@anedumla anedumla deleted the qdrift branch November 29, 2021 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog synthesis
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants