Oxidize PauliFeatureMap
#13045
Oxidize PauliFeatureMap
#13045
Conversation
Cryoris
added
Changelog: New Feature
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 10813596822Warning: 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 |
| /// insert_barriers: Whether to insert barriers in between the Hadamard and evolution layers. | ||
| /// data_map_func: An accumulation function that takes as input a vector of parameters the | ||
| /// current gate acts on and returns a scalar. | ||
| /// |
There was a problem hiding this comment.
parameter_prefix appears in the python code but not here. parameters appear here and not in the python code.
There was a problem hiding this comment.
it's a bit confusing that both the python and rust functions have the same name
There was a problem hiding this comment.
Yeah, the parameters are constructed Python-side because (1) we avoid calling Python once we're in Rust space and (2) I'm not even sure we can create ParameterVectors Rust side 🤔
And I gave them the same name because they do the same thing 🙂 I know we often have function_inner on the Rust side but IMO that makes it look like it's some internal-only function -- even though they are perfectly fine to be called from the Rust interface 😄 but we can discuss this, if you want to change it 👍🏻
There was a problem hiding this comment.
Thanks for the great work, the PR is in a very good shape. The only minor question that I had is why several python tests for the (older) PauliFeatureMap were not transferred to the (newer) pauli_feature_map.
The other comment is that the pauli_feature_map method in this PR essentially implements the very general circuit construction from NLocal (namely the _build method) in the special case of how it's called from PauliFeatureMap, and I am wondering if continuing this thread may lead to some code duplication in the future. However, I am completely fine with how this looks right now, since, were we to decide to do so, we could always implement the extended build_nlocal method as a function and let pauli_feature_map call this function with appropriate arguments. Personally I also find the new code easier to read because I do not need to carefully trace which arguments in NLocal's _build are set to which values and would be against this potential future generalization, but some other people may think otherwise. You do not need to do anything, but I am leaving this as a comment to see if anyone would strongly object.
Last but not least, I am awed by your Rust skills and have learned quite a few things by reviewing this PR. 🦀
- improve readability - only create barrier if necessary
performance seems to be unaffected :)
* port PauliFeatureMap to Rust * more docs, less error expecting * cleanup, docstrings * clippy & speedup * base for dictionary entanglement * rm unnecessary pub * add reno * refactor ``rmultiply_param`` * fix & test dict entanglement * clippy * Refactor Rust code a bit - improve readability - only create barrier if necessary * RIP iterators (for proper error handling) performance seems to be unaffected :)
Summary
This PR oxidizes the
PauliFeatureMap(and derivatives) and implements a first step in refactoring the circuit library, where structurally fixed circuits are moved to functions. As part of this,PauliFeatureMap& co. are pending deprecation.Details and comments
Some benchmark data for
reps=10and different numbers of qubitsn:caching the expression across the repetitions(edit: tried, was slower), or (bigger scale) have the parameter logic in Rust.If Fixed ZZFeatureMap not accepting a list of entanglement #12767 is merged, this will be handled in a follow up.Edit: Already included in this, as it was a minor update only.