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

Sparse loop hafnian #245

Merged
merged 31 commits into from
Jun 24, 2021
Merged

Sparse loop hafnian #245

merged 31 commits into from
Jun 24, 2021

Conversation

ziofil
Copy link
Collaborator

@ziofil ziofil commented May 30, 2021

Context: Loop hafnian computation does not exploit sparsity

Description of the Change: Loop hafnian computation now handles sparse matrices

Benefits: Fast computation of sparse matrices (few ms for 1000x1000 matrix with ~1000 non-zero entries)

Possible Drawbacks: Slower than default loop hafnian on matrices with fill factor above ~50%

Related GitHub Issues: None

@ziofil ziofil requested a review from nquesada May 30, 2021 13:03
@ziofil ziofil changed the title Sparse hafnian Sparse loop hafnian May 30, 2021
@codecov
Copy link

codecov bot commented May 30, 2021

Codecov Report

Merging #245 (3f6fb5c) into master (2624d91) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #245   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           21        21           
  Lines         1293      1315   +22     
=========================================
+ Hits          1293      1315   +22     
Impacted Files Coverage Δ
thewalrus/__init__.py 100.00% <ø> (ø)
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 2624d91...3f6fb5c. Read the comment docs.

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

@ziofil : you will also need to update the CHANGELOG file.

ziofil and others added 2 commits May 30, 2021 13:29
Co-authored-by: Nicolas Quesada <zeitus@gmail.com>
.github/CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Nicolas Quesada <zeitus@gmail.com>
.github/CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Nicolas Quesada <zeitus@gmail.com>
Co-authored-by: Nicolas Quesada <zeitus@gmail.com>
Copy link
Contributor

@antalszava antalszava left a comment

Choose a reason for hiding this comment

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

Looks good to me! 👍 Mostly considered code quality, had some suggestions.

thewalrus/_hafnian.py 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
thewalrus/_hafnian.py Show resolved Hide resolved
thewalrus/tests/test_hafnian.py Outdated Show resolved Hide resolved
thewalrus/tests/test_hafnian.py Outdated Show resolved Hide resolved
.github/CHANGELOG.md Outdated Show resolved Hide resolved
.github/CHANGELOG.md Outdated Show resolved Hide resolved
ziofil and others added 9 commits June 10, 2021 20:59
Co-authored-by: antalszava <antalszava@gmail.com>
Co-authored-by: antalszava <antalszava@gmail.com>
Co-authored-by: antalszava <antalszava@gmail.com>
Co-authored-by: antalszava <antalszava@gmail.com>
Co-authored-by: antalszava <antalszava@gmail.com>
Co-authored-by: antalszava <antalszava@gmail.com>
Co-authored-by: antalszava <antalszava@gmail.com>
Co-authored-by: antalszava <antalszava@gmail.com>
Copy link
Collaborator Author

@ziofil ziofil left a comment

Choose a reason for hiding this comment

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

👍

@@ -0,0 +1,296 @@
# Copyright 2019 Xanadu Quantum Technologies Inc.
Copy link
Member

Choose a reason for hiding this comment

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

@ziofil don’t forget to delete the .vscode, .ipynb_checkpoints, and .DS_STORE directories before merging!

Comment on lines 1 to 3
{
"python.pythonPath": "/Users/filippo/miniforge3/envs/tf24/bin/python"
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

you'll need to delete this @ziofil

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.

Pending removing of unnecesary files this is ready to merge

thewalrus/__init__.py Outdated Show resolved Hide resolved
thewalrus/_hafnian.py Outdated Show resolved Hide resolved
Co-authored-by: Nicolas Quesada <zeitus@gmail.com>
.github/CHANGELOG.md Outdated Show resolved Hide resolved
@josh146
Copy link
Member

josh146 commented Jun 23, 2021

@ziofil @nquesada for some reason readthedocs won't let me double check, but is the new sparse_hafnian function appearing in the documentation, specifically the list of Hafnian algorithms?

@nquesada
Copy link
Collaborator

Thanks for catching this @josh146 ! You are right we need to update the algorithms docs to include this PR and also the previous one by Tim for banded matrices.

@nquesada nquesada merged commit 09e72aa into master Jun 24, 2021
@nquesada nquesada deleted the sparse_hafnian branch June 24, 2021 19:42
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.

4 participants