Skip to content

Correct unitary in the docstring of the FSimGate#6288

Merged
NoureldinYosri merged 2 commits intoquantumlib:masterfrom
bramathon:fix-fsimgate-docstring-matrix
Sep 15, 2023
Merged

Correct unitary in the docstring of the FSimGate#6288
NoureldinYosri merged 2 commits intoquantumlib:masterfrom
bramathon:fix-fsimgate-docstring-matrix

Conversation

@bramathon
Copy link
Copy Markdown
Contributor

In the docstring for the FSimGate, the unitary is reported to be:

    a = cos(theta)
    b = -1j*sin(theta)
    c = exp(+1j*phi)
        [
            [1, 0, 0, 0],
            [0, a, b, 0],
            [0, b, a, 0],
            [0, 0, 0, c],
        ]

However, the actual unitary produced by the gate is slightly different. Specifically, the term c is exp(-1j*phi). The snippet below demonstrates:

import numpy as np
import cirq

theta = np.pi/4
phi = np.pi/6

# Unitary of FSIM gate is:
        
cirq.unitary(cirq.FSimGate(theta, phi))

actual_c = cirq.unitary(cirq.FSimGate(theta, phi))[3,3]
doc_c = np.exp(1j*phi)

print(f"FSimGate element (3,3) is {actual_c:.4f}, Documentation is {doc_c:.4f}")

The snippet reports that the value of c is 0.8660-0.5000j while the documentation indicates it should be 0.8660+0.5000j.

I believe the gate definition is correct and the documentation is wrong for the following reasons:

  1. The documentation also states that the FSim is equal to FSimGate(θ, φ) = ISWAP**(-2θ/π) CZPowGate(exponent=-φ/π) which is consistent with c=exp(-i*φ).
  2. The PhasedFSimGate docstring has the conditional phase term as exp(-i*φ) and it would make sense for FSim and PhasedFSim to be consistent.

Thus, this MR modifies the docstring to reflect the unitary produced by the gate.

@bramathon bramathon requested review from a team, cduck and vtomole as code owners September 13, 2023 19:49
@CirqBot CirqBot added the Size: XS <10 lines changed label Sep 13, 2023
@NoureldinYosri
Copy link
Copy Markdown
Collaborator

Hi @bramathon thanks for catching this.

@codecov
Copy link
Copy Markdown

codecov bot commented Sep 13, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.01% ⚠️

Comparison is base (cf005c2) 97.88% compared to head (d4e6486) 97.88%.
Report is 1 commits behind head on master.

❗ Current head d4e6486 differs from pull request most recent head 1056161. Consider uploading reports for the commit 1056161 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6288      +/-   ##
==========================================
- Coverage   97.88%   97.88%   -0.01%     
==========================================
  Files        1104     1104              
  Lines       95819    95819              
==========================================
- Hits        93794    93793       -1     
- Misses       2025     2026       +1     
Files Changed Coverage Δ
cirq-core/cirq/ops/fsim_gate.py 100.00% <ø> (ø)

... and 1 file with indirect coverage changes

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

@NoureldinYosri NoureldinYosri enabled auto-merge (squash) September 14, 2023 22:47
@NoureldinYosri NoureldinYosri merged commit f715527 into quantumlib:master Sep 15, 2023
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Size: XS <10 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants