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

Adds various convenience functions to qiskit #12470

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

Conversation

sbrandhsn
Copy link
Contributor

Summary

This PR adds some convenience functions to Qiskit's QuantumCircuit class. The functionality was previously available but required users to dive into the API before determining the wanted quantities. The new 'depth' and 'number of gates' methods were previously available by defining a custom filter function for depth and size methods of QuantumCircuit but I thought it might be worthwhile for new users to have a convenience function that defines such a filter function for them (and saves them e.g. from accidentally counting barriers towards their metrics).

@sbrandhsn sbrandhsn requested a review from a team as a code owner May 28, 2024 16:33
@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core

@coveralls
Copy link

coveralls commented May 28, 2024

Pull Request Test Coverage Report for Build 9384252860

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 10 of 10 (100.0%) changed or added relevant lines in 2 files are covered.
  • 90 unchanged lines in 8 files lost coverage.
  • Overall coverage increased (+0.04%) to 89.606%

Files with Coverage Reduction New Missed Lines %
qiskit/circuit/parameter.py 1 98.36%
crates/accelerate/src/isometry.rs 1 99.65%
qiskit/compiler/transpiler.py 4 92.52%
crates/qasm2/src/lex.rs 4 93.13%
qiskit/circuit/parameterexpression.py 5 96.51%
qiskit/transpiler/preset_passmanagers/init.py 13 90.3%
qiskit/transpiler/target.py 29 93.75%
crates/circuit/src/circuit_data.rs 33 93.13%
Totals Coverage Status
Change from base Build 9270778715: 0.04%
Covered Lines: 62384
Relevant Lines: 69620

💛 - Coveralls

---
features:
- |
Added convenience functions to the :class:`.QuantumCircuit` class that return the depth in terms of
Copy link
Member

Choose a reason for hiding this comment

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

The proposed new features are methods, not functions.

Comment on lines 3405 to 3406
def depth_nq(self, n: int, include_directives: bool = False) -> int:
"""Returns the depth in terms of quantum gates with at least n-qubits,
Copy link
Member

Choose a reason for hiding this comment

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

The key phrase seems to be at least. And for this reason, depth_nq with n=2 is not the same thing as depth_2q. This is potentially confusing.

Copy link
Member

Choose a reason for hiding this comment

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

why the function depth_nq is with >=n and num_nq_gates is with ==n ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe have just this one with default n = 2 and drop depth_2q

@sbrandhsn
Copy link
Contributor Author

Thanks for spotting these @garrison - I addressed your comments in the recent commit.

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.

I think these functions are generally useful, but do you see any specific use-cases? especially for the n-qubit functions.

For example, the function of depth_2q has already been used in several tests, including:

test/python/synthesis/test_cx_cz_synthesis.py:            
self.assertTrue(depth2q <= 5 * num_qubits)

test/python/synthesis/test_clifford_decompose_layers.py:            
self.assertTrue(depth2q <= 7 * num_qubits + 2)

test/python/synthesis/test_cz_synthesis.py:            
self.assertTrue(depth2q == 2 * num_qubits + 2)

test/python/synthesis/test_stabilizer_synthesis.py:            
self.assertTrue(depth2q == 2 * num_qubits + 2)

qiskit/circuit/quantumcircuit.py Outdated Show resolved Hide resolved
Comment on lines 3405 to 3406
def depth_nq(self, n: int, include_directives: bool = False) -> int:
"""Returns the depth in terms of quantum gates with at least n-qubits,
Copy link
Member

Choose a reason for hiding this comment

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

why the function depth_nq is with >=n and num_nq_gates is with ==n ?

@@ -3386,6 +3386,91 @@ def depth(

return max(op_stack)

def depth_2q(self, include_directives: bool = False) -> int:
"""Returns the depth in terms of quantum gates with at least 2-qubits,
Copy link
Member

Choose a reason for hiding this comment

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

why the function depth_2q is with >=2 and num_2q_gates is with ==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.

num_nq_gates exactly checks the number of qubits because I thought users might be interested in that quantity in general. Also, num_nonlocal_gates determines the number of gates with at least two qubits. I extended the arguments of num_nonlocal_gates to include an integer n that specifies the minimum number of qubits in a gate to still be counted towards the quantity returned by num_nonlocal_gates. The default behavior of num_nonlocal_gates stays the same, so I hope our stability policy is not violated...

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree the name is a bit confusing an inconsistent with the others. Consider changing to something like depth_multi_qubit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was also thinking about whether depth_nq should work on gates with exactly n qubits or for >= n qubits. I decided for the latter because I thought the reason an user might want depth_nq to exclude certain gates is that they are comparatively cheap. I think it is safe to assume that if an user regards a gate with n qubits to be relatively expensive, they would also consider a gate with n+i qubits to be expensive (if there are no other properties to decide on). I also was not able to see a situation where an user might only be interested in the two-qubit gate depth while ignoring all other gates in the presence of e.g. three-qubit gates.

@sbrandhsn
Copy link
Contributor Author

Thanks for your comments, I addressed them in the most recent commits! :-)

I think these functions are generally useful, but do you see any specific use-cases? especially for the n-qubit functions.

For example, the function of depth_2q has already been used in several tests, including:

test/python/synthesis/test_cx_cz_synthesis.py:            
self.assertTrue(depth2q <= 5 * num_qubits)

test/python/synthesis/test_clifford_decompose_layers.py:            
self.assertTrue(depth2q <= 7 * num_qubits + 2)

test/python/synthesis/test_cz_synthesis.py:            
self.assertTrue(depth2q == 2 * num_qubits + 2)

test/python/synthesis/test_stabilizer_synthesis.py:            
self.assertTrue(depth2q == 2 * num_qubits + 2)

I think these functions will be useful to any user that is interested in building their own circuits or looking into the characteristics of these circuits. Regarding the n-qubit versions of the introduced functions: they would be useful to anyone working more with n-qubit gates either because the hardware they are working with is potentially able to realize competitive n-qubit gates or because they are looking at synthesis/transpilation problems that involve n-qubit gates.

On the other hand, the two-qubit versions are going to be searched for first by users that only worked with two-qubit gates yet.

else:
return self.size(lambda x: x.qubits == 2)

def num_nq_gates(self, n: int, include_directives: bool = False) -> int:
Copy link
Contributor

Choose a reason for hiding this comment

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

Same suggestion: have n:int = 2 and drop the num_2q_gates

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your suggestion. I removed the n=2 versions of the methods and updated the release notes accordingly!

@jakelishman
Copy link
Member

I think there was a meeting where these were discussed that I wasn't in, so apologies if I'm repeating something:

Did we consider the alternative of doing all these by "examples" documentation within the relevant functions or discussion of the circuit data model? The depth ones in particular feel like they could have been examples, and the others are relatively simple filters on an explicitly public data field - I'm concerned that adding more methods makes it harder to find everything on QuantumCircuit, and whenever there's a new method, people have to learn all its intricacies about what it does and doesn't do. It feels like teaching people to do it themselves might be more scalable on both flexibility and discoverability.

@sbrandhsn
Copy link
Contributor Author

That's a good option and I think we should include the added procedures in the documentation at some point. However, it turns out many users need these functions and defining them repeatedly seems bothersome. There likely are also users that don't want to learn Qiskit but still want to use it in order to get accurate information on a circuit they were cooking up somehow.

To make it worse, the definition of depth_nq provided in the documentation at

assert qc.depth(lambda instr: len(instr.qubits) > 1) == 1
is inaccurate (it would also include n-qubit barriers in the depth calculation which I think is at least unexpected by users). So, having one source of truth for crucial circuit characteristics seems worthwhile to me.

@Cryoris
Copy link
Contributor

Cryoris commented Jun 7, 2024

Generally I agree with Jake that we've been trying to avoid adding methods that don't have an explicit use-case to not clutter the circuit (which already has loads of methods) with more. Since most functionality here is already in place, a compromise could be to extend docstrings or functions of the existing methods.

  • instead of depth_nq, we could add a concrete example in the docstring of the depth method (or even add an argument to do that)
  • same for num_nonlocal_gates and num_nq_gates
  • the non-idle wires are already easy to get from dag.idle_wires, though granted that users would first have to convert to a DAGCircuit

@garrison
Copy link
Member

garrison commented Jun 7, 2024

  • the non-idle wires are already easy to get from dag.idle_wires, though granted that users would first have to convert to a DAGCircuit

This is the only one that I think (IMO) may be a good candidate for its own new method. And I think it'd be more useful as a nonidle_qubits() method that returns the set, rather than taking len(set(...)).

@sbrandhsn
Copy link
Contributor Author

I'm fine with having the functionality provided by these convenience procedures as a parameter to the default depth and size methods - although I think that this may not be a terrible good fit and would actually be better represented by new methods. I will just summarize my arguments and then leave it at that: :-)

  • As soon as a user works with circuits in any way (e.g. construction, transpilation, synthesis, debugging low fidelity), the user will need the information provided by these procedures.
  • A user would absolutely expect this information to be available at hand without having to understand the details of the data structures involved or defining their own lambda function.
  • Adding these procedures to the docstring can be missed easily, especially by novice users.
  • The methods proposed in this PR have been implemented incorrectly in the past (
    assert qc.depth(lambda instr: len(instr.qubits) > 1) == 1
    , An idle qubit should still be considered idle even if it has a barrier qiskit-addon-cutting#621 ). Especially novice users will not know to exclude directives in their lambda function when determining depth_nq, nonidle_qubits, or num_nq_gates and will thus get an inaccurate/incorrect return.
  • Having these functions not accept a lambda function may yield a more efficient implementation in Rust space (which may get relevant with quantum error correction).
  • We already have convenience methods in QuantumCircuit such as num_nonlocal_gates - this PR only supplements what has been started by that method and provides a potentially novice user with a good picture of their quantum circuit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants