Skip to content

Consistent sparse list format in sparse operators (backport #14067) #14110

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

Merged
merged 1 commit into from
Mar 27, 2025

Conversation

mergify[bot]
Copy link
Contributor

@mergify mergify bot commented Mar 27, 2025

Summary

Building a PauliEvolutionGate from a SparsePauliOp and SparseObservable representing the same Hamiltonian (both using only Paulis) currently does not generate the same circuit. Both are correct, but they are inconsistent: the CX chains in the Pauli evolution are just reversed. @alexanderivrii ran some benchmarks based on HamLib and found that, due to all our surprise, one ordering seems to be almost always result in less CX gates than the other. Somehow one order allows for better CX cancellations. The ordering comes back to what index order the to_sparse_list methods return, which we chose opposite in SparseObservable than SparsePauliOp, but we also could've chosen it to be consistent. Since there's now a performance impact, this PR changes to the consistent order.

Details and comments

Here's the results of @alexanderivrii's benchmarks:

hamlib test number,CX count (this PR),CX count (main)
0,3,3
1,16,34
2,6,6
3,4,4
4,21,21
5,47,83
6,49,140
7,58,94
8,128,330
9,32,68
10,87,87
11,225,284
12,14,14
13,44,50
14,80,128
15,852,1262
16,6388,24805
17,150,150
18,1486,2128
19,1593,2438
20,1486,2128
21,50,104
22,18,18
23,3214,6810
24,3069,5647
25,3107,6504
26,2470,6846
27,5170,13294
28,3454,6544
29,3842,7348
30,2048,5751
31,6237,11177
32,6675,12715
33,30,30
34,32,32
35,15378,27476
36,37756,84463
37,40080,74825
38,24,24
39,376,376
40,848,1984
41,54200,130606
42,200,200
43,52,52
44,890,2112
45,1773,3500
46,2530,6050
47,60,60
48,3150,5184
49,300,350
50,346,402
51,1602,1602
52,454,548
53,488,582
54,2970,4752
55,1530,3622
56,588,588
57,3939,3939
58,11592,19440
59,120,120
60,896,896
61,7552,24064
62,7552,24064
63,1262,1558
64,1206,1962
65,260,260
66,14098,23520
67,268,282
68,1008,1008
69,5898,5898
70,546,546
71,3077,5796
72,5378,10514
73,684,690
74,1800,1800
75,1800,1800
76,8794,22448
77,600,600
78,2784,4560
79,3834,9058
80,25524,42768
81,10380,24756
82,12421,24708
83,536,536
84,1140,1140
85,1551,1551
86,11291,16014
87,3493,4135
88,1606,1606
89,1368,1368
90,1665,1665
91,5816,7838
92,2268,2268
93,2781,2781
94,2368,2368
95,28564,32430
96,4560,4560
97,7380,7380
98,5007,5007
99,5580,5580

Another way to fix this would be to sort the indices and Paulis in the PauliEvolution but this (a) comes at additional cost and (b) will not allow users to insert custom index orders (since they would get re-sorted). So changing the order which we picked for a more natural reading seems fine here to be more consistent (and get better gate decompositions).

No reno since this was introduced in 2.0. I'm labelling this as bugfix/stable backport so we can fix this for 2.0.


This is an automatic backport of pull request #14067 done by Mergify.

* Consistent sparse list format in sparse operators

* review comments

- use combine in test
- fix comment

(cherry picked from commit 07e61c8)
@mergify mergify bot requested a review from a team as a code owner March 27, 2025 13:23
@qiskit-bot
Copy link
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

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

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

@github-actions github-actions bot added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Mar 27, 2025
@github-actions github-actions bot added this to the 2.0.0 milestone Mar 27, 2025
@jakelishman jakelishman enabled auto-merge March 27, 2025 13:29
@coveralls
Copy link

Pull Request Test Coverage Report for Build 14107675353

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall first build on mergify/bp/stable/2.0/pr-14067 at 88.066%

Totals Coverage Status
Change from base Build 14071049093: 88.1%
Covered Lines: 72734
Relevant Lines: 82590

💛 - Coveralls

@jakelishman jakelishman added this pull request to the merge queue Mar 27, 2025
Merged via the queue into stable/2.0 with commit f1bcb1e Mar 27, 2025
23 checks passed
@mtreinish mtreinish deleted the mergify/bp/stable/2.0/pr-14067 branch March 27, 2025 16:09
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants