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

Adding batchhafnian code #21

Merged
merged 43 commits into from
Jul 17, 2019
Merged

Adding batchhafnian code #21

merged 43 commits into from
Jul 17, 2019

Conversation

bgupt
Copy link
Contributor

@bgupt bgupt commented Jun 28, 2019

Description of the Change: Code to compute photon number statistics for given Gaussian state with a covariance matrix.

Benefits: Computes probabilities of all photon statistics upto a given cutoff

Possible Drawbacks: None

Related GitHub Issues: None

…googletest and python test modules. Reformattes with black and astyle.
@bgupt bgupt requested review from josh146 and nquesada June 28, 2019 11:53
@nquesada
Copy link
Collaborator

@bgupt could the following functionality be built into the C++ code?

Generating an example:

from hafnian import batchhafnian
import numpy as np
V =  np.array([[ 1.5 , 1.41421356,  0. , 0.],
     [ 1.41421356, 1.5  ,0.  ,0.        ],
     [ 0. ,0. ,1.5 , -1.41421356],
     [ 0. ,0. ,-1.41421356,  1.5       ]])
rs = np.zeros(2*n)
res = 4
indv = res*np.ones(2*n,dtype=int)
A = batchhafnian(V,rs,res)

This is the part that would be great to move into the C++ code

If I understand your code correctly you encode a tensor $T_{i_1,\ldots,i_n}$ of $n$ indices each with local dimensions base into an a linear array $A$ of size $M = \text{base}^n$.

The function find_rep below allows you to find, given the position of an element in the linear array $A$ to which element of the tensor $T_{i_1,\ldots,i_n}$ it corresponds.

Note that $n = 2 \times \ell$ where $\ell$ is the number of modes

def find_rep(val, base, n):
    x = np.empty([n]) # In C++ this would be a vector of size n
    local_val = val
    x[0] = 1
    for i in range(1,n):
        x[i] = x[i-1]*base
    digits = np.zeros([n], dtype = int)
    for i in range(n):
        digits[i] = local_val // x[-i-1] # Careful here this is integer division, not float division, although I think C++ does this by default using / 
        local_val = local_val - digits[i]*x[-i-1]

    return digits
find_rep(85,4,4) # the 85th element of the array with 2 modes (n = 2*2) and with local base size base = 4 corrsponds to the 1,1,1,1 element of the tensor
array([1, 1, 1, 1])

The function below does the following: We know that each element of $A$ is connected to an element of $T_{i_1,\ldots,i_n}$. The following function transform $A$ so that it now corresponds to the element
$T_{i_1,\ldots,i_n} \to S_{i_1,\ldots,i_n} = T_{i_1,\ldots,i_n}/\sqrt{i_!i_2! \ldots i_n!}$

def renorm(tn, m, res):
    facts = np.ones([res])
    for i in range(1,res):
        facts[i] = i*facts[i-1]
    for i in range(1,res):
        facts[i] = 1.0/np.sqrt(facts[i])
    for i in range(res**m):
        digits = find_rep(i, res, m)
        pref = 1.0
        for j in digits:
            pref = pref*facts[j]
        tn[i] = tn[i]*pref 
    return tn
B = np.copy(A)
renorm(B, 2*n, res);


For example, the last element of $A$ which corresponds to the 3,3,3,3 element of T is


```python
A[-1]
(4.499999999999993+0j)
B[-1]
(0.12499999999999988+0j)
A[-1]/B[-1] # which is also sqrt(3! 3! 3! 3!)
(35.999999999999986+0j)

Would it be possible to make the function batch hafnian in C++ take care of the division by all the factorials, i.e. have it return the array B constructed Above directly?

@nquesada
Copy link
Collaborator

One final thing is that it would be great if the inclusion of this factorial parameters could be made optional, i.e., add an extra argument to the C++ function...

hafnian/__init__.py Outdated Show resolved Hide resolved
hafnian/__init__.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 1, 2019

Codecov Report

Merging #21 into master will decrease coverage by 0.01%.
The diff coverage is 96.9%.

@@            Coverage Diff            @@
##           master     #21      +/-   ##
=========================================
- Coverage   95.21%   95.2%   -0.02%     
=========================================
  Files           8       9       +1     
  Lines         564     625      +61     
=========================================
+ Hits          537     595      +58     
- Misses         27      30       +3
Impacted Files Coverage Δ
hafnian/_hafnian.py 100% <ø> (ø) ⬆️
hafnian/__init__.py 100% <100%> (ø) ⬆️
hafnian/quantum.py 97.82% <95.65%> (-0.82%) ⬇️
hafnian/_hermite_multidimensional.py 98% <98%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0bed206...ee2ca7b. Read the comment docs.

@@ -117,6 +136,10 @@ inline std::vector<T> hermite_multidimensional(std::vector<T> &R_mat, std::vecto
std::transform(jumpFrom.begin(), jumpFrom.end(), prevJump.begin(), tmpjump.begin(), std::minus<int>());
ullint prevCoordinate = vec2index(tmpjump, resolution);
H[nextCoordinate] = H[nextCoordinate] - (static_cast<T>(jumpFrom[ii]-1))*static_cast<T>(R(k,ii))*H[prevCoordinate];

if (renorm) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

the renormalization need to occur outside the loop for (ullint jj = 0; jj < Hdim-1; jj++) otherwise you break the recursion relation. So you need a second loop for (ullint jj = 0; jj < Hdim-1; jj++) in which do the renormalization after the first loop is completed.

Copy link
Contributor Author

@bgupt bgupt Jul 10, 2019

Choose a reason for hiding this comment

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

Computed the renormalization factor in a separate array in the same loop and used a separate loop to renormalize if renorm = 1.

Copy link
Collaborator

@nquesada nquesada left a comment

Choose a reason for hiding this comment

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

Hi @bgupt . I left a few comments that should be addressed. In particular because you renomarlize inside the recursion loops you get wrong answers. Also, how hard would it be allow for these calculation to also be done in quad precision?


if (renorm) {
H[nextCoordinate] = H[nextCoordinate]/renorm_factor(nextPos);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, you should not be calling a function to calculate factorials everytime. Instead just save the square root factorials all the way to resolution and just multiply those that you need for each element of the tensor. For this you will need to write a function that allows you to find the indices correspoding to a given element of the tensor inside the vector H. Such function in python is:

def find_rep(val, base, n):
    x = np.empty([n]) # In C++ this would be a vector of size n
    local_val = val
    x[0] = 1
    for i in range(1,n):
        x[i] = x[i-1]*base
    digits = np.zeros([n], dtype = int)
    for i in range(n):
        digits[i] = local_val // x[-i-1] # Careful here this is integer division, not float division, although I think C++ does this by default using / 
        local_val = local_val - digits[i]*x[-i-1]

    return digits

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. Implemented that.

Copy link
Collaborator

@nquesada nquesada left a comment

Choose a reason for hiding this comment

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

I am not sure if it is a good idea to use integers to represent factorials.


}

ullint factorial(int n)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the largest factorial that you can calculate in integer precision ullint

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 am not too sure about that. But factorial of 21 exceeds the range of unsigned long long int so I suppose the max would be 20.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I changed the factorial function from ullint to long double. So, it should be better now.

@bgupt
Copy link
Contributor Author

bgupt commented Jul 10, 2019

I am not sure if it is a good idea to use integers to represent factorials.

Changed it to long double.

hafnian/_hafnian.py Outdated Show resolved Hide resolved
hafnian/__init__.py Outdated Show resolved Hide resolved
raise ValueError("Input matrix must be symmetric.")
if check_symmetry:
if np.linalg.norm(A - A.T) >= tol:
raise ValueError("Input matrix must be symmetric.")
Copy link
Member

Choose a reason for hiding this comment

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

How comes this check was added? Is there a use case for attempting to calculate the hafnian of non-symmetric matrices?

Copy link
Contributor Author

@bgupt bgupt Jul 15, 2019

Choose a reason for hiding this comment

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

That is just to make sure that the input matrix is symmetric. It raises error if not.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I meant the check_symmetry flag. Is there a reason to turn off this check?

Copy link
Collaborator

Choose a reason for hiding this comment

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

There was because I thought multidimensional hermite polynomials are defined for any matrix. In reality they only make sense for symmetric matrices. Will remove this option

hafnian/_hermite_multidimensional.py Show resolved Hide resolved
hafnian/_hermite_multidimensional.py Outdated Show resolved Hide resolved
hafnian/tests/test_hermite_multidimensional.py Outdated Show resolved Hide resolved
setup.py Show resolved Hide resolved
src/hermite_multidimensional.hpp Show resolved Hide resolved
src/hermite_multidimensional.hpp Outdated Show resolved Hide resolved
src/tests/hafnian_unittest.cpp Show resolved Hide resolved
extra_dll_dir = os.path.join(os.path.dirname(__file__), ".libs")
if os.path.isdir(extra_dll_dir):
os.environ["PATH"] += os.pathsep + extra_dll_dir

Copy link
Member

Choose a reason for hiding this comment

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

isort probably moved this down, but this will have to be moved back to before the line

from ._hafnian import (

to make sure it imports correctly on windows :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@bgupt can you look into this?

r"""
THIS NEEDS TO BE WRITTEN

Returns the hafnian of matrix with repeated rows/columns.
Copy link
Member

Choose a reason for hiding this comment

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

Is this a copy/paste mistake 🙂 Probably should undo this change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is. Thanks for catching it up!

hafnian/_hermite_multidimensional.py Outdated Show resolved Hide resolved
C (array): An array
index (array): A set of indices
Returns:
complex: The product of the array elements determines by index
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
complex: The product of the array elements determines by index
complex: The product of the array elements determined by index

hafnian/_hermite_multidimensional.py Outdated Show resolved Hide resolved
def expansion_coeff(alpha, resolution, renorm=True):
"""
Returns the (quasi) geometric series as a vector with components alpha^i/sqrt(i!) for 0 <= i < resolution.
If renorm is false it omits the division by factorials
Copy link
Member

Choose a reason for hiding this comment

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

Can probably delete this line since it's already discussed in the args: list.

Suggested change
If renorm is false it omits the division by factorials

hafnian/_hermite_multidimensional.py Outdated Show resolved Hide resolved
renorm (bool): If True returns :math:`H_k^{(R)}(y)/\prod(\prod_i k_i!)`
make_tensor: If False returns a flattened one dimensional array instead of a tensor with the values of the polynomial
Returns:
(array): The multidimensional Hermite polynomials.
Copy link
Member

Choose a reason for hiding this comment

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

Note that you'll need blank lines in three places in order for this to render properly in the docs.

  1. After the first line of the docstring. So after "Returns the multidimensional Hermite polynomials :math:H_k^{(R)}(y).", there should be an empty line.

  2. Before "Args:"

  3. Before "Returns"

make_tensor: If False returns a flattened one dimensional array instead of a tensor with the values of the polynomial
Returns:
(array): The values of the hafnians.
"""
Copy link
Member

Choose a reason for hiding this comment

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

same comment as above re: line breaks. You can build the docs locally (make doc) to ensure the docstrings render correctly :)

for i in range(n):
y_mat.push_back(d[i])

return hermite_multidimensional_cpp(R_mat, y_mat, resolution, renorm)
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit confused; if this is the 'batch_hafnian', should we just call the function 'batch_hafnian' instead of hermite_multidimensional?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No. As far as I know this connection has not been made and I think is better to keep them separated. The multidimensional hermite polynomials are a thing that has existed for centuries. The fact that they can be used to calculate batches of hafnians is a new finding...

@nquesada nquesada merged commit fa17e0b into master Jul 17, 2019
@nquesada nquesada deleted the batchhafnian branch July 17, 2019 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants