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 creation of registers in synthesis methods #13086

Merged
merged 6 commits into from
Sep 10, 2024

Conversation

Cryoris
Copy link
Contributor

@Cryoris Cryoris commented Sep 4, 2024

Summary

Fixes #13041 (and the same problem in other places, but no one complained yet 🙂).

Details and comments

With the moving the circuit creation to Rust-space, we created circuits that did not have any quantum (or classical) registers, which led to issues as #13041. This PR adds an argument add_regs to QuantumCircuit._from_circuit_data (default False) which allows to add these registers and create backward-compatible circuits. In places where the final circuit was created from circuit data, this argument is set to True.

@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 labels Sep 4, 2024
@Cryoris Cryoris added this to the 1.2.1 milestone Sep 4, 2024
@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core
  • @levbishop

@Cryoris
Copy link
Contributor Author

Cryoris commented Sep 5, 2024

I'm not 100% happy with this solution of compose-ing the circuit onto a new one (even if we can skip copying the ops), but I didn't find another solution to attach a register/index to each of the qubits (which we need for QuantumCircuit.__eq__ to work as before).

Another solution could be to change that __eq__ only compares bits if they both are either new style (no register/index) or old style (register/index available). But the compose solution seems less intrusive, especially for a bugfix PR we want to include in a bugfix release.

@coveralls
Copy link

coveralls commented Sep 5, 2024

Pull Request Test Coverage Report for Build 10789366864

Details

  • 24 of 29 (82.76%) changed or added relevant lines in 7 files are covered.
  • 12 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.01%) to 89.164%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/circuit/quantumcircuit.py 16 21 76.19%
Files with Coverage Reduction New Missed Lines %
crates/accelerate/src/two_qubit_decompose.rs 1 90.82%
crates/qasm2/src/lex.rs 5 92.23%
crates/qasm2/src/parse.rs 6 97.61%
Totals Coverage Status
Change from base Build 10774677144: -0.01%
Covered Lines: 73033
Relevant Lines: 81909

💛 - Coveralls

@alexanderivrii
Copy link
Contributor

Thanks, @Cryoris! If I recall correctly, many of the circuit definition functions in circuit/library also create registers named q_* (and honestly I never really understood why we need this). Are you keeping this convention throughout your work on circuit library refactoring?

@Cryoris
Copy link
Contributor Author

Cryoris commented Sep 10, 2024

It now works without an additional compose and explicit call to add_register, but there's still a tiny overhead to adding registers coming from the register creation inside _from_circuit_data. However this only falls into weight when the circuit creation is super fast. For example here are timings repeated over 100 trials:

Permutation synthesis: slower (worse as size get's larger)
-- n = 10
(main) 1.91e-05 +- 5.16e-05s
(this PR) 2.26e-05 +- 5.01e-05s
-- n = 50
(main) 4.30e-05 +- 2.63e-05s
(this PR) 6.16e-05 +- 5.36e-05s
-- n = 100
(main) 8.63e-05 +- 8.49e-05s
(this PR) 1.11e-04 +- 9.73e-05s
-- n = 1000
(main) 2.78e-04 +- 1.05e-03s
(this PR) 1.63e-03 +- 3.36e-03s

Clifford: same speed
-- n = 10
(main) 1.24e-04 +- 8.60e-05s
(this PR) 1.24e-04 +- 6.16e-05s
-- n = 50
(main)  3.05e-03 +- 1.50e-04s
(this PR) 3.05e-03 +- 1.56e-04s
-- n = 100
(main) 1.80e-02 +- 7.58e-04s
(this PR) 1.80e-02 +- 4.37e-04s
-- n = 200
(main) 1.26e-01 +- 3.85e-03s
(this PR) 1.26e-01 +- 3.78e-03s

I think in practice this won't be an issue as HLS doesn't need the registers to synthesize building blocks within an existing circuit. We can add this fast path in a follow up, since that will require changing the API by adding an additional argument to the synthesis method, so we should avoid that in the backport.

@Cryoris
Copy link
Contributor Author

Cryoris commented Sep 10, 2024

@alexanderivrii for backward compatibility it would probably make sense to keep it, but let's see if that adds a significant overhead 🙂

Copy link
Contributor

@alexanderivrii alexanderivrii 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!

@alexanderivrii alexanderivrii added this pull request to the merge queue Sep 10, 2024
Merged via the queue into Qiskit:main with commit 1962704 Sep 10, 2024
15 checks passed
mergify bot pushed a commit that referenced this pull request Sep 10, 2024
* add qregs to output circuits

* never develop while not on up-to-date main

* one shan't commit faster than ones IDE can run black

* avoid compose

(cherry picked from commit 1962704)

# Conflicts:
#	qiskit/synthesis/linear_phase/cz_depth_lnn.py
#	qiskit/synthesis/permutation/permutation_reverse_lnn.py
github-merge-queue bot pushed a commit that referenced this pull request Sep 11, 2024
)

* Fix creation of registers in synthesis methods (#13086)

* add qregs to output circuits

* never develop while not on up-to-date main

* one shan't commit faster than ones IDE can run black

* avoid compose

(cherry picked from commit 1962704)

# Conflicts:
#	qiskit/synthesis/linear_phase/cz_depth_lnn.py
#	qiskit/synthesis/permutation/permutation_reverse_lnn.py

* Fix merge conflicts

some methods are not available in Rust on 1.2, so there's no add_regs flag to add

---------

Co-authored-by: Julien Gacon <jules.gacon@googlemail.com>
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 stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clifford.to_circuit() returns a circuit with emtpy qregs
5 participants