-
Notifications
You must be signed in to change notification settings - Fork 192
[executor] A function to remove parameter expressions #2478
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
base: executor_preview
Are you sure you want to change the base?
Conversation
|
Note that, with the current implementation, some of the parameters and instructions are shared between the original and the generated circuits. |
joshuasn
left a comment
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.
Have you looked at samplomatic's ParameterExpressionTable? The logic for handling the parameter expressions is very similar to what you have here, it might be a good idea to use that so we don't have to support support two different implementations of the same thing.
Further, it also allows you to construct the parameter table without knowing the parameters, which could be useful on its own.
SamFerracin
left a comment
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, Yael!
I left a few comments. Additionally, I +1 Josh's comment that it would be nice to structure parameter handling exactly as in Samplomatic, which would make it easier to maintain
|
|
||
|
|
||
| def _remove_parameter_expressions_in_blocks( | ||
| circ: QuantumCircuit, |
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.
| circ: QuantumCircuit, | |
| circuit: QuantumCircuit, |
|
|
||
|
|
||
| def remove_parameter_expressions( | ||
| circ: QuantumCircuit, param_values: np.ndarray |
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.
| circ: QuantumCircuit, param_values: np.ndarray | |
| circuit: QuantumCircuit, param_values: np.ndarray |
| """Create an input to the quantum program that's | ||
| free from parameter expressions.""" |
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.
| """Create an input to the quantum program that's | |
| free from parameter expressions.""" | |
| """A helper to replace a circuit's parameter expressions with parameters.""" |
This is a function on circuits, not on programs or program items. Also, more than removing stuff, we replace stuff, so consider using replace_* instead of remove_*
Additionally, I would expand a bit the docstring, to add a short explanation of what we do, how we recurse into conditionals, and how we transform the parameter values.
| parameter_table: dict[str, Parameter] = {} | ||
| new_param_value_cols: list[np.ndarray] = [] | ||
|
|
||
| new_circ = _remove_parameter_expressions_in_blocks( |
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.
The "_in_blocks" part of _remove_parameter_expressions_in_blocks here is a bit confusing, because the circuit may not have any blocks. For readability, I would consider removing "_in_blocks" and calling it _remove_parameter_expressions
| param_values = np.array( | ||
| [ | ||
| [[1, 2], [3, 4], [5, 6], [7, 8]], | ||
| [[9, 10], [11, 12], [13, 14], [15, 16]], | ||
| [[17, 18], [19, 20], [21, 22], [23, 24]], | ||
| ] | ||
| ) |
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.
| param_values = np.array( | |
| [ | |
| [[1, 2], [3, 4], [5, 6], [7, 8]], | |
| [[9, 10], [11, 12], [13, 14], [15, 16]], | |
| [[17, 18], [19, 20], [21, 22], [23, 24]], | |
| ] | |
| ) | |
| param_values = np.arange(1, 25).reshape((3, 4, 2)) |
Generally, the params will be floats. Therefore, I would actually favour something like this:
| param_values = np.array( | |
| [ | |
| [[1, 2], [3, 4], [5, 6], [7, 8]], | |
| [[9, 10], [11, 12], [13, 14], [15, 16]], | |
| [[17, 18], [19, 20], [21, 22], [23, 24]], | |
| ] | |
| ) | |
| param_values = np.random.random((3, 4, 2)) |
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 find it more readable if the array is written explicitly. How about keeping the original array, and only adding dtype=float?
|
I'm taking back what I said in the standup today, switching to samplomatic's parameter table only simplifies. |
| The function tranverses the circuit and collects all the parameters and parameter expressions. | ||
| A new parameter is created for every parameter expression that is not a parameter. |
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 think that you shouldn't worry about replacing only the parameter expressions.
| The function tranverses the circuit and collects all the parameters and parameter expressions. | |
| A new parameter is created for every parameter expression that is not a parameter. | |
| The function tranverses the circuit and replaces all the parameters and parameter expressions with new parameters. |
This would allow you to simplify massively this function (no need for parameter_expressions_to_new_parameters_map) and to make a more standard use of the parameter table where you simply:
- Append every parameter/parameter expression you find to it
- Call evaluate at the end
If there are some concerns that are missing, we should improve the parameter expression table directly, rather than writing more code in places where we use it (cc'in @joshuasn here, as he's the one who wrote the table in the first place)
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 still need parameter_expressions_to_new_parameters_map, because the same parameter / parameter expression can appear several times in a circuit. So, if a+b appears 100 times, I want to create one parameter; later when I see a+b in my circuit I want to know with which parameter to replace it, and I do it by querying the map.
Co-authored-by: Samuele Ferracin <[email protected]>
Co-authored-by: Samuele Ferracin <[email protected]>
The executor does not accept parameter expressions. This PR provides a function that users can run before they instantiate a quantum program. The function will transform their circuit and parameter values such that parameter expressions are removed.
For the PR to be ready:
Parameter.bind_allinstead of projecting and usingbind.