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

Add warning when converting global phase operator to OpenQASM 2.0 #6476

Merged
merged 10 commits into from
Feb 29, 2024

Conversation

kenya-sk
Copy link
Contributor

@kenya-sk kenya-sk commented Feb 26, 2024

OpenQASM 2.0 does not support global phase (Qiskit/qiskit#7167 (comment)).
Therefore, the current implementation generated an error and terminated the program. Since the global phase does not affect the observation results, I have changed it to be supported as a warning and the program can be executed.

The following is how to handle the situation.

  1. Determine if the operator passed to qasm() is an instance of ops.GlobalPhaseGate. If a Circuit is passed, it is deployed to each operator by existing implementations and a decision is made for each.
  2. If it is an instance of ops.GlobalPhaseGate, generate a warning and return an empty string for compatibility with previous implementations.
  3. Otherwise, the operation is the same as before.

fixes #6457

@CirqBot CirqBot added the size: M 50< lines changed <250 label Feb 26, 2024
@kenya-sk kenya-sk marked this pull request as ready for review February 26, 2024 09:57
@kenya-sk kenya-sk requested review from vtomole, cduck and a team as code owners February 26, 2024 09:58
@kenya-sk kenya-sk requested a review from dabacon February 26, 2024 09:58
Copy link

codecov bot commented Feb 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.76%. Comparing base (eef7c5c) to head (4281e86).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6476   +/-   ##
=======================================
  Coverage   97.75%   97.76%           
=======================================
  Files        1105     1105           
  Lines       94924    94936   +12     
=======================================
+ Hits        92793    92810   +17     
+ Misses       2131     2126    -5     

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

@NoureldinYosri
Copy link
Collaborator

@kenya-sk thanks for taking care of this

Copy link
Collaborator

@NoureldinYosri NoureldinYosri left a comment

Choose a reason for hiding this comment

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

why do the measurements need to change position?

@kenya-sk
Copy link
Contributor Author

@NoureldinYosri
Thank you for review.
I have removed the changes that are not relevant to this issue.

@@ -191,6 +191,24 @@ def test_h_gate_with_parameter():
)


def test_qasm_global_pahse():
(q0,) = _make_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.

please change this test into testing a use case. the example from the original issue should suffice

def test_qasm_global_pahse():
    c = cirq.Circuit([cirq.global_phase_operation(np.exp(1j * 5))])
    assert cirq.qasm(c) == ... 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for comment.
I changed test case for suitable issue. An if statement is added to qasm_output.py to maintain compatibility of the output string with other cases. If this branch was not included, the output of a circuit consisting only of global phase gates would look like this. In this case, the global phase gate has no qubit to apply, but two newlines are added at the end because it is recognized as one of the operations.

Any comments on which representation is more appropriate would be appreciated.

"""OPENQASM 2.0;
include "qelib1.inc";


// Qubits: []


"""

Copy link
Collaborator

@NoureldinYosri NoureldinYosri left a comment

Choose a reason for hiding this comment

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

just one more small change

output_line_gap(2)
# In OpenQASM 2.0, the transformation of global phase gates is ignored.
# Therefore, no newline is created when the operations contained in a circuit consist only of global phase gates.
if not (
Copy link
Collaborator

Choose a reason for hiding this comment

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

change the condition so that it works for the when the circuit has multiple global phases

if all(isinstance(op, ops.GlobalPhaseGate) for op in self.operations):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I overlooked that pattern. I've addressed it now.
Thank you for pointing it out!

@NoureldinYosri NoureldinYosri merged commit d75d43f into quantumlib:main Feb 29, 2024
34 checks passed
@kenya-sk kenya-sk deleted the global_phase_warning branch February 29, 2024 01:54
@rht
Copy link

rht commented Feb 29, 2024

Thank you @kenya-sk @NoureldinYosri.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: M 50< lines changed <250
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ValueError: Cannot output operation as QASM: cirq.global_phase_operation
4 participants