qiskit
qiskit copied to clipboard
Fixed ZZFeatureMap not accepting a list of entanglement
Summary
This pull request fixes #12432
When a List[List(int)] or List[List[List[int]]] or List[List[List[List[int]]]] is passed to the entanglement in ZZFeatureMap, it returns an error. This should work like TwoLocal does.
Details and comments
Because both ZFeatureMap and ZZFeatureMap inherits from the PauliFeatureMap the problem was with how PauliFeatureMap handles the entanglement lists. To generate proper entanglement layers PauliFeatureMap relies on the get_entangler_map() (the method which takes 3 arguments) method from NLocal which handles invalid inputs and generates properly formatted entanglement layers. But unlike TwoLocal where we specify rotation blocks and the entanglement blocks as separate arguments, for PauliFeatureMap the paulis are specified as a single list. Due to this, we need some additional logic to handle how entanglement layers are generated from the user specified entanglement list.
To fix this I have added a helper method _selective_entangler_map() and also added get_entangler_map() method to PauliFeatureMap. The newly specified get_entangler_map() method will override the get_entangler_map() method from the NLocal class for List[List(int)] or List[List[List[int]]] or List[List[List[List[int]]]] but if any other input is passed to entanglement (like a str full) then we fall back on the method from NLocal to handle those cases.
With my fix all the below examples work as expected:
- If entanglement is a List[List(int)]:
N = 5
layer1 = (0,1)
layer2 = (1,0)
qc = PauliFeatureMap(N, reps=2, paulis=['Z', 'ZZ'], entanglement=[layer1, layer3], insert_barriers=True)
qc.decompose().draw('mpl')
- If entanglement is a List[List[List[int]]] (This means that entanglement will change according to reps):
N = 5
layer1 = [(0,1,2)]
layer2 = [(3,2,1)]
qc = PauliFeatureMap(N, reps=3, paulis=['Z', 'ZZZ'], entanglement=[layer1, layer2], insert_barriers=True)
qc.decompose().draw('mpl', fold=-1)
- If entanglement is List[List[List[List[int]]]] (This means that entanglement will change according to reps and paulis):
N = 5
layers = [[[(0,1), (2,3)], [(1,2), (3,4)]], [[(1,0), (3,2)] , [(2,1), (4,3)]]]
qc = PauliFeatureMap(N, reps=3, paulis=['Z', 'ZZ', 'XX'], entanglement=layers, insert_barriers=True)
qc.decompose().draw('mpl')
@nonhermitian please review this and let me know if I should make any further changes.
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
Pull Request Test Coverage Report for Build 10672222565
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.
- For more information on this, see Tracking coverage changes with pull request builds.
- To avoid this issue with future PRs, see these Recommended CI Configurations.
- For a quick fix, rebase this PR at GitHub. Your next report should be accurate.
Details
- 15 of 17 (88.24%) changed or added relevant lines in 2 files are covered.
- 815 unchanged lines in 14 files lost coverage.
- Overall coverage decreased (-0.04%) to 89.139%
| Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
|---|---|---|---|
| qiskit/circuit/library/data_preparation/pauli_feature_map.py | 14 | 16 | 87.5% |
| <!-- | Total: | 15 | 17 |
| Files with Coverage Reduction | New Missed Lines | % |
|---|---|---|
| crates/accelerate/src/nlayout.rs | 2 | 91.51% |
| crates/circuit/src/interner.rs | 4 | 93.1% |
| crates/qasm2/src/lex.rs | 4 | 92.73% |
| qiskit/circuit/gate.py | 4 | 94.12% |
| qiskit/transpiler/passes/optimization/optimize_1q_decomposition.py | 5 | 94.17% |
| crates/circuit/src/packed_instruction.rs | 8 | 96.56% |
| qiskit/circuit/quantumcircuit.py | 35 | 93.53% |
| crates/accelerate/src/target_transpiler/mod.rs | 40 | 79.92% |
| qiskit/circuit/library/n_local/n_local.py | 44 | 83.85% |
| crates/accelerate/src/two_qubit_decompose.rs | 46 | 90.9% |
| <!-- | Total: | 815 |
| Totals | |
|---|---|
| Change from base Build 10529709209: | -0.04% |
| Covered Lines: | 71835 |
| Relevant Lines: | 80588 |
💛 - Coveralls
Thanks for the contribution @shravanpatel30! This is indeed a missing feature or wrong documentation. However, I'm not sure this is the right approach to solving it, as this selection might get very messy if you have Pauli connections with differing numbers of qubits.
Is there a use-case for which you need specific entanglements? If yes, another approach could be to support a dictionary with {num_qubits: entanglement} pairs, e.g.
entanglement = {
1: [(0,), (2,)],
2: [(0, 1), (1, 2)],
3: [(0, 1, 2)]
}
zz_feature_map = ZZFeatureMap(.., paulis=["z", "zz", "zzz"], entanglement=entanglement)
Hi @Cryoris, the data encoding using a feature map is an active area of research and so being able to control the entanglement in a feature map is really important. This gives more flexibility in creating custom feature maps.
Currently, classes like TwoLocal, NLocal, and others that handle encoding allow entanglement to be specified using various formats like List[List[int]], List[List[List[int]]], or List[List[List[List[int]]]]. If we switch to using a dictionary for ZZFeatureMap and PauliFeatureMap, could that introduce inconsistencies or issues, given that these classes inherit from NLocal? However, if you believe that using a dictionary wouldn’t cause any issues, I would be happy to implement this approach.
Also, I might be misunderstanding your concern, but I’m unclear on what you meant by:
as this selection might get very messy if you have Pauli connections with differing numbers of qubits.
The current implementation works even when the entanglement is layer1 = [(0,), (2,), (1,2), (0,1,2)]:
from qiskit.circuit.library import PauliFeatureMap, ZZFeatureMap
N = 3
layer1 = [(0,), (2,), (1,2), (0,1,2)]
qc = PauliFeatureMap(N, reps=2, paulis=['Z', 'ZZ', 'ZZZ'], entanglement=[layer1], insert_barriers=True)
qc.decompose().draw('mpl')
If we switch to using a dictionary for ZZFeatureMap and PauliFeatureMap, could that introduce inconsistencies or issues, given that these classes inherit from NLocal? However, if you believe that using a dictionary wouldn’t cause any issues, I would be happy to implement this approach.
It's good to think about this -- in this case I would say it is fine to have the subclass have a specific behavior, since it does have a special structure and the list[list[list... does not apply here.
Also, I might be misunderstanding your concern, but I’m unclear on what you meant by:
as this selection might get very messy if you have Pauli connections with differing numbers of qubits.
The current implementation works even when the entanglement is layer1 = [(0,), (2,), (1,2), (0,1,2)]
I mean that the entanglement list can (1) get very large if we have all layers in one list, and (2) it can get difficult to read as one could even accidentially mix the layers, such as [(0,1), (1,2), (2,3), (1,2,3), (3,4), (0,1,2), (0,),...].
Hi @Cryoris,
I have made some changes to the PauliFeatureMap so that it now supports a dictionary {num_qubits: entanglement}.
For entanglement = { 1: [(0,), (2,)], 2: [(0, 1), (1, 2)], 3: [(0, 1, 2)] }:
And if the entanglement = { 1: [(0,), (2,)], 3: [(0, 1, 2)] }, then full entanglement (which is default for PauliFeatureMap) is used for the 2-qubit pauli ZZ:
If you are fine with these changes then I'll go ahead and modify the typing hints and docstrings of PauliFeatureMap, ZFeatureMap and ZZFeatureMap. I'll also add few tests and releasenotes for these changes.
Thanks for the update! Could you update the code such that there's no default full entanglement when the number of qubits is not specified? I think it would be better if the code is as explicit as possible, instead of adding entanglement that is not specified. Users can always add the full entanglement if it's needed 🙂
And yes to tests and releasenotes 👍🏻 The section features_circuits should be the right section.
Hi @Cryoris, I have added tests and releasenotes. I have also updated the typing hints and docstrings to reflect new changes. Let me know if you want me to make any further changes.
Sorry about the failing test.
I replaced all the tab characters with spaces as suggested by the linting error, but it's still failing. When I check the compiled release notes on my PC, my recently added release note appears correctly, so I'm not sure where the problem lies.
Hi @ElePT, thanks for the suggestion. Both code and the release notes now pass all the checks. Let me know if I need to make any more changes.
@Cryoris, I have updated the typing hints and added some code to handle entanglement specified as a callable. I have also incorporated other changes in the code according to your comments.
@Cryoris, I have made all the requested changes.