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

add stimcirq tags #862

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

emma-louise-rosenfeld
Copy link

When a particular cirq operation has a tag, convert it to stim according to the tag's value.

Copy link
Collaborator

@Strilanc Strilanc left a comment

Choose a reason for hiding this comment

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

I can't accept this as currently designed.

The basic issue is you are hardcoding your particular usage of tags into the library (replace a gate based on a tag), rather than making a tool that other people would also use. Stim and stimcirq should attach as little semantic meaning to tags as possible; there should be no kind of replacement logic triggered by the presence of a tag. Instead, the conversion should attempt to preserve the tags while making as few changes as possible.

For example, when stimcirq is converting from a stim circuit to a cirq circuit, it could add the tags appearing on the stim instruction to the resulting cirq gates as a string or as a custom class stimcirq.StimTag. Then, when converting from cirq to stim, it could look for stimcirq.CirqTag tags and turn them into tags in the stim circuit. For handling more general cases (since cirq allows a wide variety of tagging), the user should be able to specify custom tag conversion functions that take a cirq tag and return a stim tag (or vice versa).

@emma-louise-rosenfeld
Copy link
Author

emma-louise-rosenfeld commented Nov 26, 2024

I can't accept this as currently designed.

The basic issue is you are hardcoding your particular usage of tags into the library (replace a gate based on a tag), rather than making a tool that other people would also use. Stim and stimcirq should attach as little semantic meaning to tags as possible; there should be no kind of replacement logic triggered by the presence of a tag. Instead, the conversion should attempt to preserve the tags while making as few changes as possible.

For example, when stimcirq is converting from a stim circuit to a cirq circuit, it could add the tags appearing on the stim instruction to the resulting cirq gates as a string or as a custom class stimcirq.StimTag. Then, when converting from cirq to stim, it could look for stimcirq.CirqTag tags and turn them into tags in the stim circuit. For handling more general cases (since cirq allows a wide variety of tagging), the user should be able to specify custom tag conversion functions that take a cirq tag and return a stim tag (or vice versa).

The use case here is not related to the tags in STIM. Instead, here I'm working on the cirq to stim conversion, when the cirq circuit has non-standard gates

glue/cirq/stimcirq/_cirq_to_stim.py Outdated Show resolved Hide resolved
}
"""
REPEAT 3 {
H 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do a test where the stim circuit ends up containing a tag.

Choose a reason for hiding this comment

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

@Strilanc this required some extra functionality (persisting cirq tags through to stim tags) beyond our use case here but I gave a stab at it. See my most recent commit for an example of what I'm thinking. I haven't run/written tests on it yet.

Choose a reason for hiding this comment

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

@Strilanc , done, check my latest commit. Note that tags on cirq.CircuitOperation's aren't persisted through - only tags associated with cirq.TaggedOperations are maintained

glue/cirq/stimcirq/_cirq_to_stim_test.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants