-
Notifications
You must be signed in to change notification settings - Fork 56
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
Rewrites the fidelity function for clarity #226
Conversation
Codecov Report
@@ Coverage Diff @@
## master #226 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 20 20
Lines 1191 1188 -3
=========================================
- Hits 1191 1188 -3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice addition @nquesada. Was easy and clear to follow. Just a few minor comments on variable names and code, that you might want to address.
thewalrus/quantum/gaussian_checks.py
Outdated
sigma2 = cov2 / hbar | ||
deltar = (mu1 - mu2) / np.sqrt(hbar) | ||
|
||
Omega = sympmat(n0 // 2) # The symplectic matrix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend using lowercase for these variables, since capitalization would be used for classes, and thus get highlighted differently. Any reason for changing to capitalized versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I knew this would come up! So I am just trying to follow the paper and LaTeX naming, i.e., Omega is meant to be
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if you feel strongly about this I am happy to change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about this. It's a case of programming vs. academic conventions. I would say it's still better to keep lowercase, and follow Python syntax rules here. Either having it as omega
or even big_omega
(but I think the former is probably better).
thewalrus/quantum/gaussian_checks.py
Outdated
f = np.sqrt(np.linalg.det(Sigma_inv) * np.linalg.det(det_arg)) * np.exp( | ||
-0.5 * deltar @ Sigma_inv @ deltar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No transpose on the first deltar
? It seems like the paper writes it np.exp(-0.5 * deltar.T @ Sigma_inv @ deltar)
, but maybe that's the same in this case. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes no difference, although it could be added. If you do a @ b
and a
and b
are 1D arrays the numpy is smart enough to know that what you want is mathematically speaking a^T \cdot b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. That's what I suspected, just wanted to clarify. Thanks!
The actual implementation used here corresponds to the *square* of Eq. 96 of | ||
`'Gaussian states and operations - a quick reference', Brask <https://arxiv.org/abs/2102.05748>`_. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
Co-authored-by: Theodor <theodor@xanadu.ai>
Co-authored-by: Theodor <theodor@xanadu.ai>
The fidelity function in
quantum/gaussian_checks.py
is rewritten to add clarity and also a reference and test are added.This should clarify the (non-) bug posited in #223