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

Remove qubit pair ordering requirement in SerializableDevice #5642

Conversation

verult
Copy link
Collaborator

@verult verult commented Jun 29, 2022

This call site was missed after #5169 was fixed.

If gate_definitions contain a pair of the same qubit, GridDeviceMetadata will throw a ValueError.

@mpharrigan

@verult verult requested a review from mpharrigan June 29, 2022 19:04
@verult verult requested review from wcourtney, a team, vtomole and cduck as code owners June 29, 2022 19:04
@CirqBot CirqBot added the Size: XS <10 lines changed label Jun 29, 2022
@@ -121,7 +121,7 @@ def test_metadata_correct():
pairs = [
(qubits[0], qubits[1]),
(qubits[0], qubits[3]),
(qubits[1], qubits[4]),
(qubits[4], qubits[1]),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, can you describe what was happening here and what should be happening here? Was this a latent bug? What this pair just not added to the device topology because the constructor expects ordered pairs but got unordered pairs? What defense do you have against passing unordered pairs to the GridMetadata constructor?

Copy link
Collaborator Author

@verult verult Jul 6, 2022

Choose a reason for hiding this comment

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

Looked into this again, and turns out the previous code was fine. For each SYMMETRIC target in the DeviceSpecification proto, two tuples are created in the gate definition, one for each direction of the pair:

target_set.add(qid_tuple[::-1])

so the pair[0] < pair[1] deduplicates the pair, and no qubit pairs should be omitted.

I'll go ahead and close this PR (including the test case change), since SerializableDevice will be removed soon. But let me know if you feel otherwise.

What defense do you have against passing unordered pairs to the GridMetadata constructor?

In the current GridDeviceMetadata implementation, qubit pairs passed into the constructor are considered to be unordered

Copy link
Collaborator

Choose a reason for hiding this comment

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

That works as long as you're coming via that proto code path, but if you're just using the bare __init__ method, could you get yourself into hot water?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah that is true. I am OK either way since the preferred method to construct SerializableDevice is from_proto() (as mentioned in the __init__ docstring).

Copy link
Collaborator

Choose a reason for hiding this comment

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

that seems dangerous to me.

When is this class getting removed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

bump

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This class is now gone in Cirq 1.0. PR: #5743

@pavoljuhas
Copy link
Collaborator

I did not dig into the detail, but the updated test passes with the unchanged serializable_device.py module.
Perhaps the test needs to be adjusted?

@verult
Copy link
Collaborator Author

verult commented Jul 6, 2022

Closing (rationale in #5642 (comment))

@verult verult closed this Jul 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Size: XS <10 lines changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants