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

Do not generate default repetition ids if use_repetition_ids=False #5419

Merged
merged 5 commits into from
May 31, 2022
Merged

Do not generate default repetition ids if use_repetition_ids=False #5419

merged 5 commits into from
May 31, 2022

Conversation

maffoo
Copy link
Contributor

@maffoo maffoo commented May 31, 2022

Fixes #5418

@maffoo maffoo requested review from a team, vtomole and cduck as code owners May 31, 2022 07:54
@maffoo maffoo requested a review from MichaelBroughton May 31, 2022 07:54
@daxfohl
Copy link
Collaborator

daxfohl commented May 31, 2022

I think the repeat function is also affected.

daxfohl
daxfohl reviewed May 31, 2022
View reviewed changes
cirq-core/cirq/circuits/circuit_operation.py Outdated
if self.use_repetition_ids:
loop_size = abs(self.repetitions)
if not self.repetition_ids:
object.__setattr__(self, 'repetition_ids', self._default_repetition_ids())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could all these be consolidated by having default_repetition_ids return None if use_repetition_ids is false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to do the self.use_repetition_ids check in self._default_repetition_ids.

@CirqBot CirqBot added the size: S 10< lines changed <50 label May 31, 2022
@maffoo
Copy link
Contributor Author

maffoo commented May 31, 2022

@daxfohl, I can't seem to re-request review from you, but this is ready for review again after addressing your comments. PTAL

@daxfohl
Copy link
Collaborator

daxfohl commented May 31, 2022

lgtm

@maffoo
Copy link
Contributor Author

maffoo commented May 31, 2022

@daxfohl, thanks for the review! Looks like you commented instead of approving.

dabacon
dabacon approved these changes May 31, 2022
View reviewed changes
Copy link
Collaborator

@dabacon dabacon left a comment

Choose a reason for hiding this comment

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

LGTM

@maffoo maffoo merged commit 90df56a into master May 31, 2022
@maffoo maffoo deleted the u/maffoo/lazy-rep-ids branch May 31, 2022 19:37
@daxfohl daxfohl mentioned this pull request Jun 7, 2022
Closed
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
@maffoo @rht
Do not generate default repetition ids if use_repetition_ids=False (q…
3062c8d 
…uantumlib#5419)

Fixes quantumlib#5418
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
@maffoo
Do not generate default repetition ids if use_repetition_ids=False (q…
14b887c 
…uantumlib#5419)

Fixes quantumlib#5418
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dabacon dabacon dabacon approved these changes

@daxfohl daxfohl daxfohl approved these changes

@dstrain115 dstrain115 dstrain115 approved these changes

@vtomole vtomole Awaiting requested review from vtomole

@cduck cduck Awaiting requested review from cduck

@MichaelBroughton MichaelBroughton Awaiting requested review from MichaelBroughton MichaelBroughton was automatically assigned from quantumlib/cirq-maintainers

Assignees

@daxfohl daxfohl

Labels
size: S 10< lines changed <50
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Don't construct huge repetition id lists for CircuitOperation when use_repetition_ids=False
5 participants
@maffoo @daxfohl @dabacon @dstrain115 @CirqBot