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

Fixed ZZFeatureMap not accepting a list of entanglement #12767

Merged
merged 12 commits into from
Sep 3, 2024

Conversation

shravanpatel30
Copy link
Contributor

@shravanpatel30 shravanpatel30 commented Jul 12, 2024

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')
List List int
  • 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)
List List List int
  • 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')
List List List List int

@nonhermitian please review this and let me know if I should make any further changes.

@shravanpatel30 shravanpatel30 requested a review from a team as a code owner July 12, 2024 00:37
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Jul 12, 2024
@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

@coveralls
Copy link

coveralls commented Jul 12, 2024

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.

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%
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%
Totals Coverage Status
Change from base Build 10529709209: -0.04%
Covered Lines: 71835
Relevant Lines: 80588

💛 - Coveralls

@Cryoris
Copy link
Contributor

Cryoris commented Aug 16, 2024

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)

@shravanpatel30
Copy link
Contributor Author

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')
image

@Cryoris
Copy link
Contributor

Cryoris commented Aug 19, 2024

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,),...].

@shravanpatel30
Copy link
Contributor Author

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)] }:
image

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:
image

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.

@Cryoris
Copy link
Contributor

Cryoris commented Aug 26, 2024

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.

@shravanpatel30
Copy link
Contributor Author

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.

@shravanpatel30
Copy link
Contributor Author

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.

Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

Hi @shravanpatel30, I have left a suggestion that might fix the issue in the reno, I think the cause of the lint complaint is related to the indent in the code example. I have also added a few formatting suggestions for the referenced class names. You can directly click on "commit suggestion" to try it out.

@shravanpatel30
Copy link
Contributor Author

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.

Copy link
Contributor

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

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

Looks almost good, thanks for the adjustments!

@@ -116,7 +115,7 @@ def __init__(
self,
feature_dimension: Optional[int] = None,
reps: int = 2,
entanglement: Union[str, List[List[int]], Callable[[int], List[int]]] = "full",
entanglement: Union[str, Dict[int, List[Tuple[int]]], Dict[int, List[List[int]]]] = "full",
Copy link
Contributor

Choose a reason for hiding this comment

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

This should still accept a callable, which can return an entanglement specific for a repetition. So the signature should be: Union[str, Dict[...], Callable[[int], Union[str | Dict[...]].

@@ -156,6 +158,7 @@ def __init__(
self._data_map_func = data_map_func or self_product
self._paulis = paulis or ["Z", "ZZ"]
self._alpha = alpha
self.entanglement = entanglement
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary? This attribute should already be accessible via self._entanglement.

f"For num_qubits = {qb}, entanglement must be a "
f"tuple of length {qb}. You specified {en}."
)
self.entanglement[qb][ind] = tuple(map(int, en))
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we need this; users should already provide integers (as per the type hint) and I think the code works with both tuples and lists.

@shravanpatel30
Copy link
Contributor Author

@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.

@shravanpatel30
Copy link
Contributor Author

@Cryoris, I have made all the requested changes.

Copy link
Contributor

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

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

Thanks for the fix & new feature!

@Cryoris Cryoris added this pull request to the merge queue Sep 3, 2024
Merged via the queue into Qiskit:main with commit f30f851 Sep 3, 2024
15 checks passed
@ShellyGarion ShellyGarion added Changelog: New Feature Include in the "Added" section of the changelog Changelog: Bugfix Include in the "Fixed" section of the changelog labels Nov 5, 2024
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 Changelog: New Feature Include in the "Added" section of the changelog Community PR PRs from contributors that are not 'members' of the Qiskit repo
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

ZZFeatureMap does not accept layer lists for entanglement kwarg
6 participants