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

Introduces renormalized Hermite Polynomials #108

Merged
merged 15 commits into from
Dec 30, 2019
Merged

Introduces renormalized Hermite Polynomials #108

merged 15 commits into from
Dec 30, 2019

Conversation

nquesada
Copy link
Collaborator

@nquesada nquesada commented Dec 24, 2019

This PR removes the extra parameter renorm from the hermite_multimendional_cpp and instead introduce a new renorm_hermite_multimendional_cpp . These new polynomials are given by rH^B_n(y) = H^B_n(y)/\sqrt(n!). Instead of calculating the ratio of H^B_n(y) and \sqrt(n!) the renormalized polynomials use a modified recursion relation that avoids overflows. At the same the renormalized polynomials are faster to calculate than the regular polynomials + dividing by factorials thus speeding up state_vector, density_matrix, fock_tensor and all the functions in fock_gradients.

@nquesada nquesada added the WIP label Dec 24, 2019
@codecov
Copy link

codecov bot commented Dec 24, 2019

Codecov Report

Merging #108 into master will increase coverage by 0.21%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #108      +/-   ##
==========================================
+ Coverage   97.74%   97.95%   +0.21%     
==========================================
  Files          12       12              
  Lines         885      978      +93     
==========================================
+ Hits          865      958      +93     
  Misses         20       20
Impacted Files Coverage Δ
thewalrus/_hermite_multidimensional.py 100% <100%> (ø) ⬆️
thewalrus/fock_gradients.py 100% <0%> (ø) ⬆️

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 f53727e...dd7f1f5. Read the comment docs.

@nquesada nquesada added review-ready and removed WIP labels Dec 28, 2019
@nquesada
Copy link
Collaborator Author

This PR addresses #86

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.

Really cool! Not sure if this is ready yet, but it looks like it's still missing C++ and Python tests.
Also, don't forget to run ASTYLE on the C++ code! (it's the C++ formatter we were using)

include/hermite_multidimensional.hpp Show resolved Hide resolved
include/hermite_multidimensional.hpp Outdated Show resolved Hide resolved
for (int i = 1; i < n; i++)
x[i] = x[i-1]*base;
eg::Matrix<T, eg::Dynamic, eg::Dynamic> R = eg::Map<eg::Matrix<T, eg::Dynamic, eg::Dynamic>, eg::Unaligned>(R_mat.data(), dim, dim);
eg::Matrix<T, eg::Dynamic, eg::Dynamic> y = eg::Map<eg::Matrix<T, eg::Dynamic, eg::Dynamic>, eg::Unaligned>(y_mat.data(), dim, dim);
Copy link
Member

Choose a reason for hiding this comment

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

Just out of curiousity, do these two variables R and y need to be eigen matrices? I scanned through the code below, and couldn't see a spot that uses eigen methods (unless I missed it)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are correct. Eigen is not used here, at all.

Copy link
Member

Choose a reason for hiding this comment

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

Might get even faster then by avoiding this conversion 😆

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 might be worth checking...

include/hermite_multidimensional.hpp Show resolved Hide resolved
thewalrus/libwalrus.pyx Outdated Show resolved Hide resolved
thewalrus/libwalrus.pyx Outdated Show resolved Hide resolved
thewalrus/libwalrus.pyx Outdated Show resolved Hide resolved
thewalrus/libwalrus.pyx Outdated Show resolved Hide resolved
thewalrus/libwalrus.pyx Show resolved Hide resolved
nquesada and others added 3 commits December 27, 2019 23:32
Co-Authored-By: Josh Izaac <josh146@gmail.com>
Co-Authored-By: Josh Izaac <josh146@gmail.com>
Co-Authored-By: Josh Izaac <josh146@gmail.com>
@nquesada
Copy link
Collaborator Author

Hey Josh --- The new functions reuse the tests from the old functions so no new test are needed. IN a way I am just replacing existing functionality with much better implementations.


for (int i = 0; i < res; i++)
expected_re[i*res+i] = pow(0.5, static_cast<double>(i)/2.0);

out = libwalrus::hermite_multidimensional_cpp(B, d, res, renorm);
out = libwalrus::renorm_hermite_multidimensional_cpp(B, d, res);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

here we test the new renorm_hermite_multidimensional_cpp

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.

Awesome 👍

The only question I have left is with respect to the Python tests (since they don't show up in this PR). But I suppose the Python tests were already testing both renorm=True and renorm=False, so nothing is needed to be added there?

@nquesada
Copy link
Collaborator Author

Exactly! For example all the test in fock_gradients and the tests for fock_tensor in quantum are already testing for the new polys. Maybe I should have not used the word "introduces" since they were already there.

@nquesada nquesada merged commit 3483f47 into master Dec 30, 2019
@nquesada nquesada deleted the new_polys branch December 30, 2019 03:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants