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

Test round trip of gate to operation to gate #5354

Merged
merged 14 commits into from
May 19, 2022

Conversation

dabacon
Copy link
Collaborator

@dabacon dabacon commented May 12, 2022

We decided that we would do this in lieu of making everything a gate operation #2626

Note that currently PauliStringPhasorGate fails this #5167

This also uncovers some missing gates that should be json serializable, opened #5353 for this.

Also notice that two of the issues in this comment are of the for #XYXY and isn't that amazing.

@dabacon dabacon requested review from a team, vtomole and cduck as code owners May 12, 2022 00:19
@dabacon dabacon requested a review from viathor May 12, 2022 00:19
@CirqBot CirqBot added the size: M 50< lines changed <250 label May 12, 2022
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 after questions.

Comment on lines 519 to 521
for gate_cls in gate_subclasses:
if gate_cls in exceptions:
continue
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just loop over gate_subclass less exceptions for clarity sake ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Comment on lines 523 to 528
try:
gates = cirq.read_json(filename)
except FileNotFoundError:
if gate_cls in skip_classes:
skipped.add(gate_cls)
continue
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we do an existance check with file.exists instead of a try catch ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Comment on lines +536 to +537
if not isinstance(gates, collections.abc.Iterable):
gates = [gates]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this needed because some json files have lists of things instead of one single thing in them ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep...classic python, object or list of objects :(

Comment on lines +474 to +477
def all_subclasses(cls):
return set(cls.__subclasses__()).union(
[s for c in cls.__subclasses__() for s in all_subclasses(c)]
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't at risk for infinite recursion right ? (if we had two gates with circular dependencies of some kind)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this is fine since the type hierarchy has to be a dag.

@MichaelBroughton MichaelBroughton self-assigned this May 18, 2022
@dabacon dabacon added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label May 19, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label May 19, 2022
@CirqBot
Copy link
Collaborator

CirqBot commented May 19, 2022

Automerge cancelled: A required status check is not present.

Missing statuses: ['Pytest Windows (3.7)', 'Pytest Windows (3.8)', 'Pytest Windows (3.9)', 'Typescript lint check', 'Typescript tests']

@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 May 19, 2022
@dabacon dabacon merged commit a39609a into quantumlib:master May 19, 2022
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
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.

3 participants