-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Conversation
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:
|
Pull Request Test Coverage Report for Build 10672222565Warning: 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 |
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 entanglement = {
1: [(0,), (2,)],
2: [(0, 1), (1, 2)],
3: [(0, 1, 2)]
}
zz_feature_map = ZZFeatureMap(.., paulis=["z", "zz", "zzz"], entanglement=entanglement) |
…to PauliFeatureMap
…r to the comments in the code.
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 Also, I might be misunderstanding your concern, but I’m unclear on what you meant by:
The current implementation works even when the entanglement is
|
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
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 |
406431a
to
e6db70b
Compare
Hi @Cryoris, I have made some changes to the For And if the If you are fine with these changes then I'll go ahead and modify the typing hints and docstrings of |
Thanks for the update! Could you update the code such that there's no default And yes to tests and releasenotes 👍🏻 The section |
…trings to reflect new changes.
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. |
There was a problem hiding this 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.
releasenotes/notes/paulifeaturemap-takes-dictionary-as-entanglement-02037cb2d46e1c41.yaml
Outdated
Show resolved
Hide resolved
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. |
There was a problem hiding this 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", |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
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.
…nd included some logic to handle that.
@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. |
releasenotes/notes/paulifeaturemap-takes-dictionary-as-entanglement-02037cb2d46e1c41.yaml
Outdated
Show resolved
Hide resolved
…List[Tuple[int]] Co-authored-by: Julien Gacon <[email protected]>
Co-authored-by: Julien Gacon <[email protected]>
@Cryoris, I have made all the requested changes. |
There was a problem hiding this 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!
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 likeTwoLocal
does.Details and comments
Because both
ZFeatureMap
andZZFeatureMap
inherits from thePauliFeatureMap
the problem was with howPauliFeatureMap
handles the entanglement lists. To generate proper entanglement layersPauliFeatureMap
relies on theget_entangler_map()
(the method which takes 3 arguments) method fromNLocal
which handles invalid inputs and generates properly formatted entanglement layers. But unlikeTwoLocal
where we specify rotation blocks and the entanglement blocks as separate arguments, forPauliFeatureMap
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 addedget_entangler_map()
method toPauliFeatureMap
. The newly specifiedget_entangler_map()
method will override theget_entangler_map()
method from theNLocal
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 strfull
) then we fall back on the method fromNLocal
to handle those cases.With my fix all the below examples work as expected:
@nonhermitian please review this and let me know if I should make any further changes.