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 reset to QASM parser #6710

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Add reset to QASM parser #6710

wants to merge 8 commits into from
+50 −2

Conversation

rscircus
Copy link

@rscircus rscircus commented Aug 22, 2024 •
edited
Loading

This PR solves #6704

Note / 20240914:

After further inspection, I wonder why resets are implemented quite different to regular gates, despite following a somewhat similar terminology (cirq.R or cirq.reset) however, returning a ResetChannel. It appears to me that the reset is more similar to a measurement than compared to say a Hadamard gate.

@CirqBot CirqBot added the size: M 50< lines changed <250 label Aug 24, 2024
@rscircus
Copy link
Author

Rebased on top of main.

@rscircus rscircus force-pushed the add-reset-gate branch 2 times, most recently from 44689b7 to bfac925 Compare September 14, 2024 12:41
@rscircus rscircus marked this pull request as ready for review September 14, 2024 12:42
@rscircus rscircus requested review from vtomole and a team as code owners September 14, 2024 12:42
@rscircus rscircus mentioned this pull request Sep 14, 2024
Open
@rscircus rscircus force-pushed the add-reset-gate branch 2 times, most recently from a8cdb0b to dc4dffa Compare September 14, 2024 13:07
@rscircus
Copy link
Author

Squashing everything into one commit after we finish review. Thus I leave the generic 'Fix comments & typos' as it is for now.

@rscircus
Add reset to QasmParser
e1e1730 
Add a test which can serve as example including the modifications to
support the `reset` keyword in the import of QASM files in the PLY based
lexer/parser.
@rscircus
Fix comments & typos
9d2be2d
@rscircus
Copy link
Author

Hey guys, you have any feedback on the solution proposal here?

natestemen
natestemen approved these changes Sep 27, 2024
View reviewed changes
Copy link

@natestemen natestemen left a comment

Choose a reason for hiding this comment

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

My review doesn't mean much, since I'm not very familiar with this codebase, but the diff looks reasonable! Hope this can get reviewed by a cirq maintainer soon :)

cirq-core/cirq/contrib/qasm_import/_parser.py Show resolved Hide resolved
@github-actions github-actions bot added the Stale label Oct 28, 2024
@dstrain115
Copy link
Collaborator

It looks like there is an error in the parser:

ERROR: /home/runner/work/Cirq/Cirq/cirq-core/cirq/contrib/qasm_import/_parser.py:508: Symbol 'qreg' used, but not defined as a token or a rule

Also, please run check/formatting in order to pass the format check.

@mhucka mhucka added status/stale This has been closed due to inactivity for an extended period of time. and removed Stale labels Nov 18, 2024
@github-actions github-actions bot removed the status/stale This has been closed due to inactivity for an extended period of time. label Nov 19, 2024
@gluonhiggs
Copy link

It looks like there is an error in the parser:

ERROR: /home/runner/work/Cirq/Cirq/cirq-core/cirq/contrib/qasm_import/_parser.py:508: Symbol 'qreg' used, but not defined as a token or a rule

Also, please run check/formatting in order to pass the format check.

Hello everyone, is there anything remaining to address in this PR before it can be finalized?

@pavoljuhas
Copy link
Collaborator

@rscircus - thank you for contributing this and apologies for slow response.

I have fixed the typo in parser definition and complaints from format checker.
I also added a test of the reset instruction with OpenQASM 3.

Please take a look if you are fine with the PR as is. LGTM from my POV.

@codecov Codecov
Copy link

codecov bot commented Nov 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.85%. Comparing base (d760f00) to head (7c93f77).

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #6710      +/-   ##
==========================================
- Coverage   97.85%   97.85%   -0.01%     
==========================================
  Files        1084     1084              
  Lines       93742    93759      +17     
==========================================
+ Hits        91733    91749      +16     
- Misses       2009     2010       +1

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

@rscircus
Copy link
Author

Thanks. I'll pull at the weekend and give it a final run.

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

@natestemen natestemen natestemen approved these changes

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

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

Assignees
No one assigned
Labels
size: M 50< lines changed <250
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@rscircus @dstrain115 @gluonhiggs @pavoljuhas @natestemen @mhucka @CirqBot