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

Insertion ordered parameters #3910

Closed
wants to merge 11 commits into from

Conversation

Cryoris
Copy link
Contributor

@Cryoris Cryoris commented Mar 2, 2020

Summary

The information of the insertion order of parameters is present in the QuantumCircuit but actively discarded at several points. This PR changes this behaviour and updates the circuit to return parameters as list in the order of insertion. This is in agreement of dictionaries since Python 3.6, whose content is insertion ordered.

This feature is required for backward compatibility in Aqua's refactor of the variational forms, resp. the Ansatz object.

Details and comments

Since Python 3.6 dictionaries are insertion ordered, i.e. retrieving the keys of a dictionary will return an iterable mirroring the order in which the keys are inserted. In the QuantumCircuit, parameters are stored in a dictionary so the information of the insertion order is present.

Currently, we discard this information at two points:

  1. if QuantumCircuit.parameters is called we return a set of the parameters, which can change the order
  2. if we create a Gate or Instruction out of a circuit (by calling to_gate/instruction()), the parameters are actively sorted by name

The new behaviour ensures that parameters are insertion-sorted by

  1. returning a list, not set, of the parameter dictionary keys
  2. not re-sorting the parameters of the circuit

This feature mirrors the behaviour some users might expect and is necessary for changes in Aqua introduced by the Ansatz object.

Copy link
Member

@kdk kdk left a comment

Choose a reason for hiding this comment

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

Code changes look good. The converters could also be updated to track parameter ordering. One comment in line and one question.

For context, the initial Parameter implementation avoided setting insertion order to be parameter order to avoid ambiguities when building and composing parameterized circuits (especially if the user did not know the circuit was parameterized). The thinking was that in cases where parameter ordering had a defined meaning, users could track it in a list or use a ParameterVector. (The current to_gate sort-by-name behavior is unusual. Really, we should ask users to provide the ordering they expect.)

Are these approaches (parameter in a list, or a ParameterVector) difficult or not feasible for the ansatz use case?

@Cryoris
Copy link
Contributor Author

Cryoris commented Mar 3, 2020

For context, the initial Parameter implementation avoided setting insertion order to be parameter order to avoid ambiguities when building and composing parameterized circuits (especially if the user did not know the circuit was parameterized).

How does name-sorting avoid ambiguities that are not avoided with insertion-ordering?

Are these approaches (parameter in a list, or a ParameterVector) difficult or not feasible for the ansatz use case?

I think it's possible to implement the Ansatz with an additional list for bookkeeping. The insertion-ordering is also mainly required for backward compatibility and better visualization of the circuits.

The more elegant solution, however, is achieved with insertion-ordering. I think that's also the more natural method to store them. For instance converting a circuit to a gate and back to a circuit changes the parameter order of the initial circuit. And since the user never specified any sorting it is unexpected to happen (at least was for me, when programming the Ansatz 😄).

@kdk
Copy link
Member

kdk commented Mar 4, 2020

(I started typing this up before yesterday's discussion, so most of this has already been covered, but I'll add here for completeness.)

For context, the initial Parameter implementation avoided setting insertion order to be parameter order to avoid ambiguities when building and composing parameterized circuits (especially if the user did not know the circuit was parameterized).

How does name-sorting avoid ambiguities that are not avoided with insertion-ordering?

I agree sorting by name seems fairly arbitrary (and I agree it's worth changing), but it is consistent with regards to how the circuit was constructed. e.g. If I have two equivalent circuits, that were generated by different means, their name-sorted parameter list will always be the same.

(To maybe belabor the point, the reason this is important is that it helps to avoid a whole class of bugs that depend on how an object was created. e.g. When converting to a DAG and back, when editing gates or circuit operations in place, when reversing a circuit, etc., if tracking insertion order by dict, the order of operations becomes important in a way that wouldn't necessarily be obvious to someone not familiar with the parameters code.)

Are these approaches (parameter in a list, or a ParameterVector) difficult or not feasible for the ansatz use case?

I think it's possible to implement the Ansatz with an additional list for bookkeeping. The insertion-ordering is also mainly required for backward compatibility and better visualization of the circuits.

I'd be interested to find out more about how insertion ordering helps with backwards compatibility (just for a better understanding of the old and new ansatz structure on my part. We can talk this over offline if that's easier.)

The more elegant solution, however, is achieved with insertion-ordering. I think that's also the more natural method to store them. For instance converting a circuit to a gate and back to a circuit changes the parameter order of the initial circuit. And since the user never specified any sorting it is unexpected to happen (at least was for me, when programming the Ansatz 😄).

I agree providing a specified ordering (for circuit.parameters and circuit.to_gate().params) is better than returning a set (specifically because sets are harder to work with and more likely to confuse users).

@kdk kdk added the on hold Can not fix yet label Mar 10, 2020
@kdk kdk removed their assignment Mar 10, 2020
@Cryoris Cryoris marked this pull request as draft June 18, 2020 07:07
@Cryoris Cryoris closed this Aug 21, 2020
@kdk kdk mentioned this pull request Sep 22, 2020
@lcapelluto
Copy link
Contributor

Linked a PR above for which this has implications. Changes made to circuit should also be reflected in Schedule to maintain a consistent interface

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
on hold Can not fix yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants