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

Numba permanent #300

Merged
merged 54 commits into from
Nov 25, 2021
Merged

Numba permanent #300

merged 54 commits into from
Nov 25, 2021

Conversation

nquesada
Copy link
Collaborator

Implements the Ryser and BBFG algorithms for the calculation of the permanent using numba.

JQZ1111 and others added 28 commits November 9, 2021 14:16
Added Ryser algorithm to find the permanent of a matrix. Note that perm_ryser(A) can find the permanent of a real matrix as well as the permanent of a complex one.
Changed to possibly return ryser and bbfg instead of complex and real ryser and bbfg
Used black and pylint
Removed imports of "perm_real", "perm_complex", "perm_BBFG_real" and "perm_BBFG_complex".
Corrected function names to be only perm.
Removed #include <permanent.hpp>
@nquesada
Copy link
Collaborator Author

The numba code is also faster than the C++:
image
at least on my machine.

@nquesada nquesada requested a review from sduquemesa November 16, 2021 02:35
thewalrus/_permanent.py Outdated Show resolved Hide resolved
Comment on lines +51 to +59

* [Jiaqi Zhao](https://github.com/JQZ1111) (Polytechnique Montréal) - Photon squeezer

* [Dominic Leclerc](https://github.com/dleclerc33) (Polytechnique de Montréal) :fleur_de_lis: King of vaccum

* [Benjamin Lanthier](https://github.com/benjaminlanthier) (Polytechnique de Montréal) :mage: Warlock of eigenvalues

* [Brandon Turcotte](https://github.com/brandonpolymtl) (Polytechnique de Montréal) - :star2: The Quantum-plators
Copy link
Contributor

Choose a reason for hiding this comment

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

🎆 🏅 🎊

Comment on lines 15 to 16
Permanent Python interface
"""
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 need to have a little bit more documentation as for example in lines 14-36 of https://github.com/XanaduAI/thewalrus/blob/master/thewalrus/fock_gradients.py

Co-authored-by: Josh Izaac <josh146@gmail.com>
Comment on lines 85 to 87
if a != b:
raise Exception("Not a square matrix")
n = len(M)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@josh146 : how do feel about raising exception from numba code? Note that these checks are also done by wrapper function.

Copy link
Member

Choose a reason for hiding this comment

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

I have no great opinion 🙂 If the checks are already being done by the wrapper function, makes sense to remove it from here though I would think? Saves the validation being done twice.

Copy link
Member

Choose a reason for hiding this comment

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

Note though it's best not to raise a bare Exception, but to always use a subclass e.g., ValueError

@thisac
Copy link
Contributor

thisac commented Nov 23, 2021

[sc-9848]

thewalrus/_permanent.py Outdated Show resolved Hide resolved
thewalrus/_permanent.py Outdated Show resolved Hide resolved
thewalrus/_permanent.py Outdated Show resolved Hide resolved
@nquesada nquesada requested a review from josh146 November 24, 2021 18:12
Copy link
Member

@josh146 josh146 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 @nquesada! Only one question about the tests and the bbfg method :)

Comment on lines +25 to +26
perm_BBFG_real = lambda x: perm(x, method="bbfg")
perm_BBFG_complex = lambda x: perm(x, method="bbfg")
Copy link
Member

Choose a reason for hiding this comment

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

Are these used anywhere below? Since it looks like the rest of the test file is not modified 🤔

@josh146
Copy link
Member

josh146 commented Nov 25, 2021

Oh also don't forget to add an item to the changelog before merging!

@nquesada nquesada merged commit dfabc84 into XanaduAI:master Nov 25, 2021
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.

7 participants