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

Hafnian modifications #333

Merged
merged 35 commits into from
Mar 1, 2022
Merged

Conversation

nquesada
Copy link
Collaborator

@nquesada nquesada commented Feb 22, 2022

The new hafnian functions now use the Labudde method to calculate power traces instead of using diagonalization.

@nquesada nquesada marked this pull request as draft February 22, 2022 15:36
@codecov
Copy link

codecov bot commented Feb 23, 2022

Codecov Report

Merging #333 (9610392) into master (8d56ecf) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #333   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           24        24           
  Lines         1722      1723    +1     
=========================================
+ Hits          1722      1723    +1     
Impacted Files Coverage Δ
thewalrus/_hafnian.py 100.00% <100.00%> (ø)

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 8d56ecf...9610392. Read the comment docs.

thewalrus/_hafnian.py Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@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.

Some benchmarking is also needed.

@@ -261,17 +261,16 @@ def f_loop_odd(E, AX_S, XD_S, D_S, n, oddloop, oddVX_S): # pragma: no cover
Returns:
array: polynomial coefficients
"""
E_k = E.copy()

count = 0
comb = np.zeros((2, n + 1), dtype=np.complex128)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since now the power traces inherit the type of your matrix, you don't need to make this complex, I believe. Worth checking this in a separate branch before doing the change here.

@@ -640,7 +640,9 @@ def loop_hafnian(A, D=None, reps=None, glynn=True):

def input_validation(A, rtol=1e-05, atol=1e-08):
"""Checks that the matrix A satisfies the requirements for Hafnian calculation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This seems to be the same issue as the last time. For some reason the text editor of one of you @brandonpolymtl @JQZ1111 @benjaminlanthier @dleclerc33 is adding empty lines. This need to be corrected!

Copy link
Contributor

Choose a reason for hiding this comment

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

Would suggest to check the wrapping settings of your editors. Also, check if the documentation is rendering correctly.

@brandonpolymtl
Copy link
Contributor

Benchmark_hafnian_eig_vs_charpoly
Here is a benchmark of how long it takes to calculate the hafnian of different matrix sizes with the eigenvalues method and with the powertraces method

thewalrus/_hafnian.py Outdated Show resolved Hide resolved
thewalrus/_hafnian.py Outdated Show resolved Hide resolved
thewalrus/_hafnian.py Outdated Show resolved Hide resolved
thewalrus/_hafnian.py Outdated Show resolved Hide resolved
thewalrus/_hafnian.py Outdated Show resolved Hide resolved
@nquesada nquesada marked this pull request as ready for review February 25, 2022 16:24
@nquesada nquesada requested a review from sduquemesa February 25, 2022 17:01
.gitignore Outdated
@@ -26,3 +26,5 @@ docs/code/api/*
.idea
.mypy_cache/
.DS_Store

venv
Copy link
Contributor

Choose a reason for hiding this comment

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

Should add a line at the end of the file.

Suggested change
venv
venv

Thinking of it again, maybe it is not a good idea to have this on the project's gitignore file — every developer can have a different name for its dev environment.
I guess the best option is to add venv (or whatever is the name of your virtual environment) to your global gitignore file.

thewalrus/_hafnian.py Outdated Show resolved Hide resolved
thewalrus/_hafnian.py Outdated Show resolved Hide resolved
thewalrus/_hafnian.py Outdated Show resolved Hide resolved
thewalrus/_hafnian.py Outdated Show resolved Hide resolved
@@ -640,7 +640,9 @@ def loop_hafnian(A, D=None, reps=None, glynn=True):

def input_validation(A, rtol=1e-05, atol=1e-08):
"""Checks that the matrix A satisfies the requirements for Hafnian calculation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would suggest to check the wrapping settings of your editors. Also, check if the documentation is rendering correctly.

JQZ1111 and others added 12 commits February 27, 2022 18:37
Co-authored-by: Sebastián Duque Mesa <675763+sduquemesa@users.noreply.github.com>
Co-authored-by: Sebastián Duque Mesa <675763+sduquemesa@users.noreply.github.com>
Co-authored-by: Sebastián Duque Mesa <675763+sduquemesa@users.noreply.github.com>
Co-authored-by: Sebastián Duque Mesa <675763+sduquemesa@users.noreply.github.com>
Co-authored-by: Nicolas Quesada <991946+nquesada@users.noreply.github.com>
Co-authored-by: Nicolas Quesada <991946+nquesada@users.noreply.github.com>
Co-authored-by: Nicolas Quesada <991946+nquesada@users.noreply.github.com>
Co-authored-by: Nicolas Quesada <991946+nquesada@users.noreply.github.com>
Co-authored-by: Nicolas Quesada <991946+nquesada@users.noreply.github.com>
@nquesada nquesada requested a review from sduquemesa February 28, 2022 16:02
thewalrus/_hafnian.py Outdated Show resolved Hide resolved
@nquesada nquesada merged commit cf3f887 into XanaduAI:master Mar 1, 2022
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.

6 participants