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

Migrate StrategyExecutor to new transformer infrastructure #5644

Merged
merged 20 commits into from
Jul 1, 2022

Conversation

ammareltigani
Copy link
Contributor

Migrated the StrategyExecutor class to strategy_executor() function that uses transformer_primitives. Depreciated StrategyExecutor.

@ammareltigani ammareltigani requested review from a team, vtomole and cduck as code owners June 29, 2022 20:18
@CirqBot CirqBot added the size: M 50< lines changed <250 label Jun 29, 2022
@tanujkhattar tanujkhattar self-assigned this Jun 29, 2022
Copy link
Collaborator

@tanujkhattar tanujkhattar left a comment

Choose a reason for hiding this comment

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

Looks like a good start, I've left some comments. Please let me know if you have questions!

cirq-core/cirq/contrib/acquaintance/executor.py Outdated Show resolved Hide resolved
cirq-core/cirq/contrib/acquaintance/executor.py Outdated Show resolved Hide resolved
cirq-core/cirq/contrib/acquaintance/executor.py Outdated Show resolved Hide resolved
cirq-core/cirq/contrib/acquaintance/executor.py Outdated Show resolved Hide resolved
cirq-core/cirq/contrib/acquaintance/executor.py Outdated Show resolved Hide resolved
cirq-core/cirq/contrib/acquaintance/executor.py Outdated Show resolved Hide resolved
cirq-core/cirq/contrib/acquaintance/executor.py Outdated Show resolved Hide resolved
cirq-core/cirq/contrib/acquaintance/executor.py Outdated Show resolved Hide resolved
@tanujkhattar tanujkhattar marked this pull request as draft June 30, 2022 00:08
Copy link
Collaborator

@tanujkhattar tanujkhattar left a comment

Choose a reason for hiding this comment

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

Left another round of comments.

cirq-core/cirq/contrib/acquaintance/executor.py Outdated Show resolved Hide resolved
cirq-core/cirq/contrib/acquaintance/executor.py Outdated Show resolved Hide resolved
cirq-core/cirq/contrib/acquaintance/executor.py Outdated Show resolved Hide resolved
cirq-core/cirq/contrib/acquaintance/executor.py Outdated Show resolved Hide resolved
cirq-core/cirq/contrib/acquaintance/executor.py Outdated Show resolved Hide resolved
cirq-core/cirq/contrib/acquaintance/executor.py Outdated Show resolved Hide resolved
cirq-core/cirq/contrib/acquaintance/executor.py Outdated Show resolved Hide resolved
cirq-core/cirq/contrib/acquaintance/executor.py Outdated Show resolved Hide resolved
cirq-core/cirq/contrib/acquaintance/executor.py Outdated Show resolved Hide resolved
@ammareltigani
Copy link
Contributor Author

I changed the argument and return type for the circuit that is being modified in the __call__ function of StrategyExecutorTransformer to circuits.Circuit (instead of accepting circuits.AbstractCircuit and returning circuits.Circuit for the reasons we discussed) and also reflected this change in the __call__ function of ExecutionStrategy.

This is because the method expose_acquaintance_gates accepts a circuit of type circuits.Circuit (not circuits.AbstractCircuit), so the type checker was complaining when I made the argument of type circuits.AbstractCircuit.

I thought of changing the type of the circuit in the method signature of __call__ in ExposeAcquaintanceGates but it seems like this could be a breaking change since with are accessing _moments in a method of that class, which is a property of circuits.Circuit but not of circuits.AbstractCircuit

@ammareltigani ammareltigani marked this pull request as ready for review June 30, 2022 23:34
syncing with latest version of main Cirq repo
@pavoljuhas
Copy link
Collaborator

...
FAILED dev_tools/docker_test.py::test_docker_pre - AssertionError: assert 1 == 0
FAILED dev_tools/docker_test.py::test_docker_stable - AssertionError: assert ...
2 failed, 16372 passed, 93 skipped, 132 xfailed, 504 warnings in 277.64s (0:04:37)

I also get those 2 failures when running tests locally, I suspect they need docker installation to pass.
You can ignore those two unless your PR makes some changes to the Dockerfile.

Copy link
Collaborator

@tanujkhattar tanujkhattar left a comment

Choose a reason for hiding this comment

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

Looking pretty good; a final round of comments!

cirq-core/cirq/contrib/acquaintance/executor.py Outdated Show resolved Hide resolved
cirq-core/cirq/contrib/acquaintance/executor.py Outdated Show resolved Hide resolved
cirq-core/cirq/contrib/acquaintance/executor.py Outdated Show resolved Hide resolved
cirq-core/cirq/contrib/acquaintance/executor.py Outdated Show resolved Hide resolved
cirq-core/cirq/contrib/acquaintance/executor.py Outdated Show resolved Hide resolved
cirq-core/cirq/contrib/acquaintance/executor.py Outdated Show resolved Hide resolved
cirq-core/cirq/contrib/acquaintance/executor.py Outdated Show resolved Hide resolved
cirq-core/cirq/contrib/acquaintance/executor.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@tanujkhattar tanujkhattar left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for all the iterations!

@tanujkhattar tanujkhattar merged commit c828652 into quantumlib:master Jul 1, 2022
@ammareltigani ammareltigani deleted the deprec-strategyexecutor branch July 11, 2022 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: M 50< lines changed <250
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants