Skip to content

Promote FrozenCircuit.tags to AbstractCircuit#7476

Merged
pavoljuhas merged 6 commits intoquantumlib:mainfrom
andbe91:master
Jul 10, 2025
Merged

Promote FrozenCircuit.tags to AbstractCircuit#7476
pavoljuhas merged 6 commits intoquantumlib:mainfrom
andbe91:master

Conversation

@andbe91
Copy link
Copy Markdown
Collaborator

@andbe91 andbe91 commented Jul 9, 2025 •
edited by pavoljuhas
Loading

First step towards fixing #7454. This PR just adds the tags attribute to AbstractCircuit, along with some of the logic (e.g. __eq__, _json_dict_, etc). After this we have to add support in the proto serialization itself.

Partially implements #7454

@andbe91 andbe91 requested review from a team and vtomole as code owners July 9, 2025 16:20
@andbe91 andbe91 requested a review from tanujkhattar July 9, 2025 16:20
@github-actions github-actions bot added the size: M 50< lines changed <250 label Jul 9, 2025
@codecov
Copy link
Copy Markdown

codecov bot commented Jul 9, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.68%. Comparing base (68c5741) to head (f886f32).
Report is 2 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff            @@
##             main    #7476    +/-   ##
========================================
  Coverage   98.68%   98.68%            
========================================
  Files        1092     1092            
  Lines       96694    96799   +105     
========================================
+ Hits        95425    95529   +104     
- Misses       1269     1270     +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

dstrain115
dstrain115 approved these changes Jul 9, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@dstrain115 dstrain115 left a comment

Choose a reason for hiding this comment

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

Thanks for fixing my half-written feature!

pavoljuhas
pavoljuhas requested changes Jul 9, 2025
View reviewed changes
Copy link
Copy Markdown
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.

Couple of small fixes needed, please see inline.
Otherwise LGTM, thank you for contributing this!

Comment thread cirq-core/cirq/circuits/circuit.py
Comment thread cirq-core/cirq/circuits/circuit.py
Comment thread cirq-core/cirq/circuits/circuit.py
Comment thread cirq-core/cirq/circuits/circuit.py
Comment thread cirq-core/cirq/circuits/circuit_test.py Outdated
@andbe91 andbe91 requested a review from pavoljuhas July 9, 2025 22:51
@andbe91
Copy link
Copy Markdown
Collaborator Author

andbe91 commented Jul 9, 2025

@pavoljuhas addressed all comments, PTAL

pavoljuhas
pavoljuhas reviewed Jul 9, 2025
View reviewed changes
Comment thread cirq-core/cirq/circuits/circuit.py Outdated
pavoljuhas
pavoljuhas reviewed Jul 9, 2025
View reviewed changes
Comment thread cirq-core/cirq/circuits/circuit.py Outdated
pavoljuhas
pavoljuhas requested changes Jul 10, 2025
View reviewed changes
Copy link
Copy Markdown
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.

Sorry about more comments, but I realized we are somewhat inconsistent in how are circuit tags propagated in methods that produce new circuits. For example, factorize copies tags from self to the split circuits, but zip and concat_ragged drop them. The addition c1 + c1 preserves tags from c1, but 2 * c1 does not.

I suppose the simplest consistent behavior would be to propagate tags from the first, ie, self Circuit in all methods that return one or more new Circuits.

Can you please preserve circuit tags from self also for

  • __mul__
  • __rmul__
  • __pow__
  • __radd__
  • concat_ragged
  • with_noise
  • zip

@github-actions github-actions bot added size: L 250< lines changed <1000 and removed size: M 50< lines changed <250 labels Jul 10, 2025
@andbe91
Copy link
Copy Markdown
Collaborator Author

andbe91 commented Jul 10, 2025

Can you please preserve circuit tags from self also for

  • __mul__
  • __rmul__
  • __pow__
  • __radd__
  • concat_ragged
  • with_noise
  • zip

Done, also added tests for this. PTAL

@andbe91 andbe91 requested a review from pavoljuhas July 10, 2025 02:56
pavoljuhas
pavoljuhas approved these changes Jul 10, 2025
View reviewed changes
Copy link
Copy Markdown
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.

LGTM with a two final nits. Thank you for adding this!

Comment thread cirq-core/cirq/circuits/circuit.py Outdated
Comment thread cirq-core/cirq/circuits/circuit.py Outdated
@andbe91 @pavoljuhas
Apply suggestions from code review
f886f32 
Co-authored-by: Pavol Juhas <pavol.juhas@gmail.com>
@pavoljuhas pavoljuhas added this pull request to the merge queue Jul 10, 2025
Merged via the queue into quantumlib:main with commit 346c95b Jul 10, 2025
35 checks passed
ddddddanni pushed a commit to ddddddanni/Cirq that referenced this pull request Jul 15, 2025
@andbe91 @pavoljuhas @ddddddanni
Promote FrozenCircuit.tags to AbstractCircuit (quantumlib#7476)
6153cb1 
First step towards fixing
quantumlib#7454. This PR just adds the
`tags` attribute to `AbstractCircuit`, along with some of the logic
(e.g. `__eq__`, `_json_dict_`, etc). After this we have to add support
in the proto serialization itself.

Partially implements quantumlib#7454

---------

Co-authored-by: Pavol Juhas <pavol.juhas@gmail.com>
This was referenced Aug 5, 2025
Closed
Merged
@NoureldinYosri NoureldinYosri mentioned this pull request Sep 11, 2025
Merged
@NoureldinYosri NoureldinYosri mentioned this pull request Sep 25, 2025
Merged
@NoureldinYosri NoureldinYosri mentioned this pull request Oct 21, 2025
Merged
This was referenced Jan 12, 2026
Closed
Closed
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

@dstrain115 dstrain115 dstrain115 approved these changes

@vtomole vtomole Awaiting requested review from vtomole vtomole is a code owner

@tanujkhattar tanujkhattar Awaiting requested review from tanujkhattar tanujkhattar is a code owner automatically assigned from quantumlib/cirq-maintainers

Assignees

No one assigned

Labels

size: L 250< lines changed <1000

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@andbe91 @pavoljuhas @dstrain115