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

Deprecate TaggedOperations #4967

Open
vtomole opened this issue Feb 9, 2022 · 5 comments
Open

Deprecate TaggedOperations #4967

vtomole opened this issue Feb 9, 2022 · 5 comments
Labels
area/tags kind/health For CI/testing/release process/refactoring/technical debt items status/needs-agreed-design We want to do this, but it needs an agreed upon design before implementation triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on

Comments

@vtomole
Copy link
Collaborator

vtomole commented Feb 9, 2022

Description of the issue
Moved tags to the Operation class

Cirq version
master version

@vtomole vtomole added kind/health For CI/testing/release process/refactoring/technical debt items area/tags triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on labels Feb 9, 2022
@daxfohl
Copy link
Collaborator

daxfohl commented Feb 18, 2022

Note I tried this a while back and it's hugely invasive. For example TaggedOperation intercepts several protocol methods like diagram_info and resolve_params, and adorns them with its own behavior before forwarding inward. If it's in the base class, this behavior is impossible, as the subop class's protocol implementation simply overrides it. All these protocol methods need to be changed to abstract/virtual methods of a different name, and have the base class's protocol call them, or something.

Also since op classes aren't dataclasses, we need to bring in the copy module to implement with_tags. Which, we do use copy elsewhere so maybe that's not as much of a problem, but my impression is that people with a strong Python background tend to be wary of depending on copy. Also the scale on which this would use copy would extend to all subclasses of Operation, including custom ops, which is much more than anything we currently use it for.

@mpharrigan
Copy link
Collaborator

Is this actually triage/accepted?

@dstrain115 dstrain115 added triage/discuss Needs decision / discussion, bring these up during Cirq Cynque and removed triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on labels Mar 31, 2022
@dstrain115
Copy link
Collaborator

I think this is worth another discussion, especially given Dax's comments on how this is not as straightforward as it seems on face value.

@daxfohl
Copy link
Collaborator

daxfohl commented Apr 10, 2022

One option that may actually be easier is if we go all in and change all existing operations to gates. I've done a few of those for #4683. The ones remaining are those from #2626 (PauliString, QFT), and the higher level ones like CircuitOperation and ClassicallyControlledOperation. (POC for the former was working at #4725).

If we do that, then GateOperation is all that's left. No inheritance hierarchy or anything. So we can simply add the tag to that class and be done.

Still a lot of work to get to this point but I think it would result in a nice design. @95-martin-orion @tanujkhattar

@vtomole vtomole added time/after-1.0 status/needs-agreed-design We want to do this, but it needs an agreed upon design before implementation and removed time/before-1.0 labels Apr 20, 2022
@tanujkhattar
Copy link
Collaborator

This is a large design issue and would require a considerable thought. Given that we are already past the threshold of considering large design items for Cirq 1.0, we should mark it as post 1.0 and work on it for Cirq 2.0.

@95-martin-orion 95-martin-orion added triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on and removed triage/discuss Needs decision / discussion, bring these up during Cirq Cynque labels Apr 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/tags kind/health For CI/testing/release process/refactoring/technical debt items status/needs-agreed-design We want to do this, but it needs an agreed upon design before implementation triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on
Projects
Status: No status
Development

No branches or pull requests

8 participants