Refactor thewalrus.quantum
#197
Conversation
…AI/thewalrus into ch328-the-walrus-quantum-refactor
Codecov Report
@@ Coverage Diff @@
## master #197 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 14 20 +6
Lines 1087 1131 +44
=========================================
+ Hits 1087 1131 +44
Continue to review full report at Codecov.
|
Co-authored-by: Nicolas Quesada <zeitus@gmail.com>
There was a problem hiding this comment.
Looks great @thisac! I'm happy to approve this in its current form, as it is already an organization improvement. The only things I have really earmarked are some module names which could be modified slightly.
Apart from that, everything else marked 'minor' are just possible minor improvements I've noticed that are worth considering, but could be more breaking for SF.
| return (hbar / 2) * cov | ||
|
|
||
|
|
||
| def Amat(cov, hbar=2, cov_is_qmat=False): |
There was a problem hiding this comment.
@nquesada how is the Amat related to the adjacency matrix again?
There was a problem hiding this comment.
the Amat is the adjacency matrix. Well, one of them, in the case of a pure state A = B \directsum B^* and sometimes we also call B the adjacency matrix. One way to get around this is to call A the Kernel matrix and B the adjacency matrix.
There was a problem hiding this comment.
One way to get around this is to call A the Kernel matrix and B the adjacency matrix.
This sounds quite nice!
| cov (array): :math:`2N\times 2N` covariance matrix | ||
| hbar (float): the value of :math:`\hbar` in the commutation | ||
| relation :math:`[\x,\p]=i\hbar`. | ||
| cov_is_qmat (bool): if ``True``, it is assumed that ``cov`` is in fact the Q matrix. |
There was a problem hiding this comment.
This is another inconsistency; the Amat function accepts either a covariance matrix or Q matrix depending on keyword arguments.
However, we have two separate Qmat functions; one that accepts an adjacency matrix, and another accepting a covariance matrix.
There was a problem hiding this comment.
Are you suggesting to merge the later or separate the former?
There was a problem hiding this comment.
I don't know 😆 just something I noticed. Would you have a preference?
| Functions for constructing/calculating the means, variances and covariances of | ||
| Gaussian states. |
There was a problem hiding this comment.
Perhaps a better name for this module is gaussian_statistics.py? with statistics implying means, variances, covariances, etc.
There was a problem hiding this comment.
The problem with that name is that Gaussian statistics in the lay world simple means normally distributed things. The GBS distribution is anything but normal.
There was a problem hiding this comment.
Hmm... gaussian_state_statistics.py?
| raise ValueError("The Gaussian state is not pure") | ||
|
|
||
|
|
||
| def _squeezed_state_distribution(s, cutoff=50, N=1): |
There was a problem hiding this comment.
Curious why this is hidden (has it always been hidden?) Sounds like it could be quite useful in some contexts
There was a problem hiding this comment.
It wasn't actually hidden before. I think that @nquesada mentioned it could be hidden, but I might have misunderstood.
There was a problem hiding this comment.
It used to be not hidden. The main idea behind this is that most of the functions should take as arguments a cov matrix, this one does not.
|
|
||
| .. autosummary:: | ||
|
|
||
| pure_state_distribution |
There was a problem hiding this comment.
I almost want to suggest just making the two private functions public and putting them here. The Walrus is quite low-level, so it could always be the case that someone wants that functionality, but ends up coding it themselves just because they don't know it exists
There was a problem hiding this comment.
My argument against that is that almost all the Walrus functions take as arguments a covariance matrix (and possibly a vector of means). These other two functions take one or a few squeezing parameters.
Co-authored-by: Josh Izaac <josh146@gmail.com>
Co-authored-by: Josh Izaac <josh146@gmail.com>
Look in the
quantum/__init__.pyfile to get an overview for how the functions have been moved around.Context:
The Walrus quantum module (
thewalrus.quantum) is currently a bit unwieldy due to the large number of functions in the same file. It needs to be refactored to a package with better organization.Description of the Change:
thewalrus.quantum.pyis split up into several files collected in a folder/submodule namedquantum. This folder contains the following files:fock_tensors.pySet of functions for calculating various state representations, probabilities and classical subsystems of Gaussian states.
adjacency_matrices.pyFunctions for rescaling adjacency matrices as well as calculating the Qmat covariance matrix from an adjacency matrix.
gaussian_state_tests.pyTests for various properties of covariance matrices as well as fidelity calculations for Gaussian states.
means_and_variances.pyFunctions for constructing/calculating the means, variances and covariances of Gaussian states.
photon_number_distributions.pyFunctions for calculating the photon number distributions of various states.
conversions.pyFunctions for transforming one type of covariance-matrix-like object into another as well as reducing a Gaussian state.
Furthermore, the following functions have been renamed, although the old names are still functional:
Means->real_to_complex_displacementsBeta->complex_to_real_displacementsprefactor->_prefactorgen_multi_mode_dist->_convolve_squeezed_state_distributiongen_single_mode_dist->_squeezed_state_distributiontotal_photon_num_dist_pure_state->pure_state_distributionBenefits:
The quantum submodule is much cleaner and clearer. Everything works the same way as it did before, only the names are updated.
Possible Drawbacks:
The function renaming shouldn't cause any issues since the old names still function, although removing the old names shouldn't either since neither TW nor SF seem to use any of the renamed functions.
There are currently no circular imports, although this could be an issue if adding more functions to these files. One has to be careful when doing so!
Related GitHub Issues:
N/A