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

Refactor ActOnArgs.kron/factor/transpose to reduce code duplication #4463

Merged
merged 10 commits into from Sep 1, 2021

Conversation

daxfohl
Copy link
Contributor

@daxfohl daxfohl commented Aug 24, 2021

Step 3 for feedforward, outlined in https://tinyurl.com/cirq-feedforward.

This PR reduces duplication by moving redundant code from ActOnArgs.kron etc, such as qubits = self.qubits + other.qubits into the ActOnArgs base class, and then using handlers in subclasses to append subclass-specific information. It also updates these methods with an inplace option to allow them to be consistent with the swap/rename methods added in #4169

@95-martin-orion has the most context here.

@daxfohl daxfohl requested review from cduck, vtomole and a team as code owners August 24, 2021 19:42
@CirqBot CirqBot added the size: L 250< lines changed <1000 label Aug 24, 2021
@google-cla google-cla bot added the cla: yes Makes googlebot stop complaining. label Aug 24, 2021
Copy link
Collaborator

@MichaelBroughton MichaelBroughton left a comment

Choose a reason for hiding this comment

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

This looks good. Just a few high level questions in some areas.

cirq-core/cirq/sim/act_on_args.py Show resolved Hide resolved
cirq-core/cirq/sim/act_on_args.py Show resolved Hide resolved
cirq-core/cirq/sim/act_on_density_matrix_args.py Outdated Show resolved Hide resolved
cirq-core/cirq/sim/act_on_density_matrix_args.py Outdated Show resolved Hide resolved
cirq-core/cirq/sim/act_on_state_vector_args.py Outdated Show resolved Hide resolved
cirq-core/cirq/sim/act_on_args.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@MichaelBroughton MichaelBroughton left a comment

Choose a reason for hiding this comment

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

LGTM

@MichaelBroughton MichaelBroughton added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Sep 1, 2021
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Sep 1, 2021
@CirqBot CirqBot merged commit 3b6ddfb into quantumlib:master Sep 1, 2021
@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Sep 1, 2021
@daxfohl daxfohl deleted the split3 branch September 1, 2021 13:48
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
…uantumlib#4463)

Step 3 for feedforward, outlined in https://tinyurl.com/cirq-feedforward.

This PR reduces duplication by moving redundant code from ActOnArgs.kron etc, such as `qubits = self.qubits + other.qubits` into the ActOnArgs base class, and then using handlers in subclasses to append subclass-specific information. It also updates these methods with an `inplace` option to allow them to be consistent with the swap/rename methods added in quantumlib#4169

@95-martin-orion has the most context here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Makes googlebot stop complaining. size: L 250< lines changed <1000
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants