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

Faster hermite #280

Merged
merged 17 commits into from
Sep 3, 2021
Merged

Faster hermite #280

merged 17 commits into from
Sep 3, 2021

Conversation

ziofil
Copy link
Collaborator

@ziofil ziofil commented Sep 2, 2021

fully numbified filling of termite multitimensional arrays and gradients. It turns out np.ndindex is supported in numba.

@ziofil ziofil requested a review from nquesada September 2, 2021 18:44
@nquesada
Copy link
Collaborator

nquesada commented Sep 2, 2021

@ziofil --- Could you update the CHANGELOG? Also is this giving a speed increase? If so it would be nice to report it.

@ziofil
Copy link
Collaborator Author

ziofil commented Sep 2, 2021

@ziofil --- Could you update the CHANGELOG? Also is this giving a speed increase? If so it would be nice to report it.

Do you have a preferred speed test to compare it against?

@nquesada
Copy link
Collaborator

nquesada commented Sep 2, 2021

You could run the basic calculation that we use to test the correctness of the numba hermite vs the C++ hermites?

@ziofil
Copy link
Collaborator Author

ziofil commented Sep 2, 2021

You could run the basic calculation that we use to test the correctness of the numba hermite vs the C++ hermites?

Sure, where do I find it?

@nquesada
Copy link
Collaborator

nquesada commented Sep 2, 2021

In the test folder :) in the test_hermite file

@ziofil
Copy link
Collaborator Author

ziofil commented Sep 2, 2021

cutoff = 5 and 1,2,3,4,5 indices

Screen Shot 2021-09-02 at 4 25 38 PM

@ziofil
Copy link
Collaborator Author

ziofil commented Sep 2, 2021

sped it up a bit
Screen Shot 2021-09-02 at 5 03 03 PM

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.

Awesome addition @ziofil . Really nice to see numba defeat C++ :)

Comment on lines 271 to 273
def _grad_hermite_multidimensional_numba(R, y, array, dG_dR, dG_dy, cutoffs):
indices = np.ndindex(cutoffs)
next(indices) # skip the first index (0,...,0)
Copy link
Collaborator

@nquesada nquesada Sep 2, 2021

Choose a reason for hiding this comment

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

can you add a docstring here?

Comment on lines 223 to 224
def _hermite_multidimensional_numba(R, array, y, cutoffs):
indices = np.ndindex(cutoffs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here, a docstring would be nice

@codecov
Copy link

codecov bot commented Sep 2, 2021

Codecov Report

Merging #280 (3bc0dca) into master (e75ade1) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #280   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           21        21           
  Lines         1441      1428   -13     
=========================================
- Hits          1441      1428   -13     
Impacted Files Coverage Δ
thewalrus/_hafnian.py 100.00% <100.00%> (ø)
thewalrus/_hermite_multidimensional.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 e75ade1...3bc0dca. Read the comment docs.

@josh146
Copy link
Member

josh146 commented Sep 3, 2021

Really nice to see numba defeat C++ :)

🤯 How is Numba able to do this?

@ziofil
Copy link
Collaborator Author

ziofil commented Sep 3, 2021

Really nice to see numba defeat C++ :)

🤯 How is Numba able to do this?

probably the C++ code is not well written 😅

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