Remove dependance of BooleanExpression on tweedledum
#13769
Conversation
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 13499061671Warning: 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
💛 - Coveralls |
There was a problem hiding this comment.
Thanks @gadial for taking care of this - it looks great!
Note that there are some necessary API changes here, which makes sense and it's also allowed for Qiskit 2.0, but probably need better release notes, as well as raise deprecation warnings in Qiskit 1.4.
Did you make some experiments to check the performance compared to Tweedledum?
|
|
||
|
|
||
| @HAS_TWEEDLEDUM.require_in_instance | ||
| class BooleanExpression(ClassicalElement): | ||
| class BooleanExpression(Gate): |
There was a problem hiding this comment.
The easiest deprecation path would be to just deprecate BooleanExpression in place and have the new version in the circuit library.
It might also be worth considering a renaming to avoid too much confusion with the new object. What does this circuit implement exactly? Does it flip a target bit if the boolean expression is satisfied or does it something in-place? If it's the first, maybe BitflipOracle could be a good name.
There was a problem hiding this comment.
This seems like a sound strategy - we already have a PhaseOracle so we can add BitFlipOracle which will be similar but with a different synthesis, and both gates would rely on a non-gate version of BooleanExpression (maybe with a different name) which handles parsing, simulating (to create the truth table) and synthesizing (since we might want to invest in optimized algorithms later.
The main question is where to put those files. We can put BitFlipOracle next to PhaseOracle in qiskit.circuit.library but we still need to put the NewBooleanExpression module (which would consist at least of boolean_expression, boolean_expression_visitor and boolean_expression_synth files) somewhere that makes sense.
There was a problem hiding this comment.
Thanks @gadial for taking care of this! I have a few comments and questions.
qiskit/circuit/library/__init__.py
Outdated
| @@ -345,6 +345,7 @@ | |||
| QuantumVolume | |||
| PhaseEstimation | |||
| GroverOperator | |||
| BitFlipOracle | |||
There was a problem hiding this comment.
Seems like a good idea but I agree it belongs in another PR.
releasenotes/notes/boolean_expression_update-39c19edbbe71ba0d.yaml
Outdated
Show resolved
Hide resolved
releasenotes/notes/boolean_expression_update-39c19edbbe71ba0d.yaml
Outdated
Show resolved
Hide resolved
…icitly with large number of bits
There was a problem hiding this comment.
LGTM, thanks @gadial !
Thanks for the comparison of synthesis time compared to tweedledum.
I also wonder what is the CX-cost comparison of the transpiled circuits.
ShellyGarion
added
the
Changelog: New Feature
| class BitFlipOracle(QuantumCircuit): | ||
| r"""Bit-flip Oracle. |
There was a problem hiding this comment.
In the spirit of circuit library refactoring, do we want BitFlipOracle and PhaseOracle to also have Gate variants? @Cryoris, what are your thoughts on this? What about BooleanExpression?
There was a problem hiding this comment.
Personally I'm againt BooleanExpression being a Gate (as is currently the situation) since it's "interface" is not clear, as is reflected in the difference between phase oracle (n qubits, applies Z conditioned on f(x)) and bit flip oracle (n+1 qubits, applies X on a specific qubit conditioned on f(x)). Currently the somewhat arbitrary choice was to have BooleanExpression as a gate act as a bit-flip oracle.
There was a problem hiding this comment.
I completely agree that BooleanExpression should not be a Gate, thanks! I also think that we probably want to have the new BitFlipOracle available as a subclass of a quantum circuit, at the least to be consistent with PhaseOracle. However, for the sake of circuit library refactoring, in a follow-up we should also expose phase oracle and bit flip oracles as gates.
There was a problem hiding this comment.
I would like to make a point for Gates 🙂 in the circuit library modernization (#13046) we define that all objects are either gates or functions -- gates, if their action is defined by a mathematical operation and we are free to synthesize with different algorithms, and functions, if they are defined by their structure.
To me, this is therefor a clear case of a Gate. We can discuss what interface makes most sense, if there are multiple options. What you suggested above makes sense to me:
- a
BitFlipOracleflips a target qubit iff(x)is satisfied - a
PhaseOracleflips the phase of states satisfyingf(x)-- note that it is fine if the actual implementation uses an auxiliary qubit.
I would prefer not introducing a BitFlipOracle(QuantumCircuit) if we add a gate. The PhaseOracle should also be updated to be a gate once we agree on the interface, to match the rest of the library and enable different synthesis algorithms.
There was a problem hiding this comment.
On the point of interface to the future PhaseOracleGate and BitFlipOracleGate, do you think these gates should have the "evaluate" and "from_dimacs" methods?
There was a problem hiding this comment.
From the docstrings,
- The phase oracle implements
$|x\rangle \mapsto (-1)^{f(x)}|x\rangle$ , and thus does not involve an additional qubit, - The bitflip oracle implements
$|x\rangle|y\rangle \mapsto |x\rangle|f(x)\oplus y\rangle$ , and thus involves an additional "target" qubit$y$ .
@Cryoris, I think the questions are: would you like PhaseOracleGate and BitFlipOracleGate be a part of this PR and/or would you like to pending-deprecate PhaseOracle as a part of this PR?
Alternatively, maybe it's fine to introduce a BitFlipOracle quantum circuit (as this PR is doing), and add the two gates in a follow-up?
There was a problem hiding this comment.
My concrete question is whether I can simply change the line class BitFlipOracle(QuantumCircuit) into class BitFlipOracleGate(Gate) and be done with it, or whether that means I should implement additional methods currently not implemented.
As for evaluate_bitstring I don't mind removing it from bit flip oracle, and after deprecating PhaseOracle in favor of PhaseOracleGate we can also drop it from PhaseOracleGate. It's not a functionality that a gate should supply, and anyway the user can always do oracle.boolean_expression.simulate.
There was a problem hiding this comment.
If we are already introducing BitFlipOracleGate as part of this PR, should we also add PhaseOracleGate ?
(and PhaseOracle will be deprecated at Qiskit 3.0)
There was a problem hiding this comment.
Done in 991ded1
I think we can merge.
There was a problem hiding this comment.
This is a really-really cool PR. Thanks for making the phase oracle (and its friend, bit-flip oracle) available to those who can no longer use tweedledum.
My only suggestions are around documentation: expanding the release notes and adding python type hints to various functions.
releasenotes/notes/boolean_expression_update-39c19edbbe71ba0d.yaml
Outdated
Show resolved
Hide resolved
| class BitFlipOracle(QuantumCircuit): | ||
| r"""Bit-flip Oracle. |
There was a problem hiding this comment.
I completely agree that BooleanExpression should not be a Gate, thanks! I also think that we probably want to have the new BitFlipOracle available as a subclass of a quantum circuit, at the least to be consistent with PhaseOracle. However, for the sake of circuit library refactoring, in a follow-up we should also expose phase oracle and bit flip oracles as gates.
| var_order: list = None, | ||
| ) -> None: | ||
| """Creates a BitFlipOracle object | ||
|
|
||
| Args: | ||
| expression: A Python-like boolean expression. | ||
| var_order(list): A list with the order in which variables will be created. | ||
| (default: by appearance) |
There was a problem hiding this comment.
Some minor comments on docstrings (which might be off in the original class already, not that you added this 🙂)
- if
Noneis supported we can annotate it withOtherType | None(this might requirefrom __future__ import annotations) - you could also add the type of the list elements -- I'm not sure but this might be
list[str]? - if the type is in the signature it can be skipped in the docstring
- we typically skip the initial sentence in the initializer
| var_order: list = None, | |
| ) -> None: | |
| """Creates a BitFlipOracle object | |
| Args: | |
| expression: A Python-like boolean expression. | |
| var_order(list): A list with the order in which variables will be created. | |
| (default: by appearance) | |
| var_order: list[str] | None = None, | |
| ) -> None: | |
| """ | |
| Args: | |
| expression: A Python-like boolean expression. | |
| var_order: A list with the order in which variables will be created. | |
| (default: by appearance) |
releasenotes/notes/boolean_expression_update-39c19edbbe71ba0d.yaml
Outdated
Show resolved
Hide resolved
releasenotes/notes/boolean_expression_update-39c19edbbe71ba0d.yaml
Outdated
Show resolved
Hide resolved
releasenotes/notes/boolean_expression_update-39c19edbbe71ba0d.yaml
Outdated
Show resolved
Hide resolved
releasenotes/notes/boolean_expression_update-39c19edbbe71ba0d.yaml
Outdated
Show resolved
Hide resolved
| class BitFlipOracle(QuantumCircuit): | ||
| r"""Bit-flip Oracle. |
There was a problem hiding this comment.
On the point of interface to the future PhaseOracleGate and BitFlipOracleGate, do you think these gates should have the "evaluate" and "from_dimacs" methods?
releasenotes/notes/boolean_expression_update-39c19edbbe71ba0d.yaml
Outdated
Show resolved
Hide resolved
releasenotes/notes/boolean_expression_update-39c19edbbe71ba0d.yaml
Outdated
Show resolved
Hide resolved
|
|
||
| class TruthTable: | ||
| """A simple implementation of a truth table for a boolean function""" | ||
|
|
There was a problem hiding this comment.
would you like to add some explanation or example?
| (default: by appearance) | ||
| """ | ||
| self.boolean_expression = BooleanExpression(expression, var_order=var_order) | ||
| self.oracle = self.boolean_expression.synth(circuit_type="bit") |
There was a problem hiding this comment.
This runs the synthesis, right? If yes, this should be moved into _define to avoid eagerly synthesizing the circuit upon gate creation.
| def __init__( | ||
| self, | ||
| expression: str, | ||
| var_order: list[str] | None = None, |
There was a problem hiding this comment.
Could you add an optional label: str | None = None here, which allows the user to overwrite the default label? All gates typically have this 🙂 (also for the BitFlipOracleGate)
|
|
||
|
|
||
| @ddt | ||
| class TestPhaseOracleGate(QiskitTestCase): |
There was a problem hiding this comment.
Could you merge this test with the one for the circuit? They seem to be the same 🙂 You could e.g. do
@data( ... )
@unpack
def test_statevector(self, expression, truth_table):
for use_gate in [True, False]:
oracle = PhaseOracleGate(expression) if use_gate else PhaseOracle(expression)
# ... rest as before ...
with self.subTest(use_gate=use_gate):
self.assertListEqual( ... )There was a problem hiding this comment.
I actually prefer to avoid this since we are deprecating PhaseOracle and I want to keep is separate from PhaseOracleGate.
There was a problem hiding this comment.
We're only going to remove it for 3.0 though, so it would be nice to have them together since bugfixes and new checks would likely need to cover both. But we can also keep them separate and copy these, if they occur in the future, in case you prefer 🙂
There was a problem hiding this comment.
I'm convinced. Done in 454ecc5
There was a problem hiding this comment.
LGTM, but I won't put it on the merge queue yet in case @ShellyGarion has further comments 🙂
Summary
Rewrites the
BooleanExpressionclass such that expression parsing and circuit synthesis does not rely on thetweedledumlibrary, and moves it to thesynthesislibrary. Adds aBitFlipOracleclass to preserve the user-facing usage ofBooleanExpression.Partially addresses #13755.
Details and comments
BooleanExpressionis used to describe classical boolean functions and synthesize phase/bit-flip oracles for them. Currently it is used directly within qiskit only for thePhaseOracleclass.The current implementation of
BooleanExpressionrelies on thetweedledumlibrary for parsing and synthesizing. Since qiskit 2.0 will stop usingtweedledum, this PR makes the code ofBooleanExpressionindependent, in the cost of lower capabilities.These are the main changes:
astlibrary, using a customboolean_expression_visitor. Parsing DIMACS files is done directly using regexp.boolean_expression_synthmodule) using a straightforward approach seemingly also employed bytweedledum, given a representation of the function as ESOP (Exclusive sum of squares).BooleanExpressioninto ESOP representation was also added. While it features basic optimizations to reduce the size of the ESOP, it needs to generate the full truth table of the represented function, making it a viable approach only for functions on a relatively low number of variables. This can be improved upon in the future if required, using more sophisticated ESOP generation and minimization techniques (such as using BDDs or SAT-solvers).synthesis/boolean. A new class,BitFlipOraclewas added to the circuit library. The oldBooleanExpressioncode remains unchanged and will be deprecated in a separate PR along with the rest of theclassicalfunctionlibrary.Non comprehensive benchmarking indicates a factor-10 runtime blowup for the non-tweedledum version, but oracle generation for functions of around 10 variables is still viable.
Comparing the CX gate count in the result of transpiling the oracle for
GenericBackendV2(num_qubits=27)with pass managergenerate_preset_pass_manager(optimization_level=2, backend=backend)yields a slight advantage to the new implementation: