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.

Sign up for GitHub

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

Jump to bottom

Add ZipLongest to cirq_google #6074

Merged
merged 4 commits into from
Apr 25, 2023
Merged

Add ZipLongest to cirq_google #6074

merged 4 commits into from
Apr 25, 2023

Conversation

dstrain115
Copy link
Collaborator

  • This class is similar to cirq.Zip but repeats the last value if the combined sweeps are not the same length.

@dstrain115
Add ZipLongest to cirq_google
560d9c5 
- This class is similar to cirq.Zip but repeats the
last value if the combined sweeps are not the same
length.
@dstrain115 dstrain115 requested review from wcourtney, a team, vtomole, cduck and verult as code owners April 21, 2023 19:58
@dstrain115 dstrain115 requested review from maffoo and zchen088 April 21, 2023 20:16
@maffoo
Copy link
Contributor

maffoo commented Apr 21, 2023

Can this go into cirq.study? Seems generally useful and not specific to cirq_google

pavoljuhas
pavoljuhas requested changes Apr 22, 2023
View reviewed changes
Copy link
Collaborator

@pavoljuhas pavoljuhas left a comment

Choose a reason for hiding this comment

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

please see the inline comments

@dstrain115 dstrain115 requested a review from pavoljuhas April 24, 2023 19:52
pavoljuhas
pavoljuhas reviewed Apr 24, 2023
View reviewed changes
cirq-core/cirq/study/sweeps_test.py Outdated
sweep1 = cirq.ZipLongest(cirq.Points('a', [1, 2, 3]), cirq.Points('b', [4, 5, 6, 7]))
sweep2 = cirq.ZipLongest(cirq.Points('a', [1, 2, 3]), cirq.Points('b', [4, 5, 6, 7]))
sweep3 = cirq.ZipLongest(cirq.Points('a', [1, 2]), cirq.Points('b', [4, 5, 6, 7]))
sweep4 = cirq.Zip(cirq.Points('a', [1, 2]), cirq.Points('b', [4, 5, 6, 7]))
Copy link
Collaborator

@pavoljuhas pavoljuhas Apr 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

As it is, both sweep3 == sweep4 and hash(sweep3) == hash(sweep4) evaluate to True.
Perhaps we need to use a strict type(other) is Zip in the Zip.__eq__ function

Cirq/cirq-core/cirq/study/sweeps.py

Lines 294 to 297 in 58d0205

def __eq__(self, other):
if not isinstance(other, Zip):
return NotImplemented
return self.sweeps == other.sweeps

and insert ZipLongest or a similar type-marker to the hashed tuple at

Cirq/cirq-core/cirq/study/sweeps.py

Lines 364 to 365 in 58d0205

def __hash__(self) -> int:
return hash(tuple(self.sweeps))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed to use 'is not Zip' and also changed the test to use Cirq's equality tester.

pavoljuhas
pavoljuhas requested changes Apr 24, 2023
View reviewed changes
Copy link
Collaborator

@pavoljuhas pavoljuhas left a comment

Choose a reason for hiding this comment

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

Looks good, but I noticed yet another corner case we should clean up.

@dstrain115 dstrain115 requested a review from pavoljuhas April 24, 2023 23:10
@dstrain115 dstrain115 added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Apr 25, 2023
@dstrain115 dstrain115 merged commit c1d6451 into quantumlib:master Apr 25, 2023
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
@dstrain115
Add ZipLongest to cirq_google (quantumlib#6074)
d3616b7 
* Add ZipLongest to cirq_google

- This class is similar to cirq.Zip but repeats the
last value if the combined sweeps are not the same
length.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pavoljuhas pavoljuhas pavoljuhas approved these changes

@wcourtney wcourtney Awaiting requested review from wcourtney

@vtomole vtomole Awaiting requested review from vtomole

@cduck cduck Awaiting requested review from cduck

@verult verult Awaiting requested review from verult

@maffoo maffoo Awaiting requested review from maffoo

@zchen088 zchen088 Awaiting requested review from zchen088

Assignees
No one assigned
Labels
automerge Tells CirqBot to sync and merge this PR. (If it's running.)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dstrain115 @maffoo @pavoljuhas