theochem / grid

Python library for numerical integration, interpolation, and differentiation on (molecular) grids.
https://grid.qcdevs.org/
GNU Lesser General Public License v3.0
46 stars 19 forks source link

Update Lebedev Module #111

Closed FarnazH closed 3 years ago

FarnazH commented 3 years ago

The lebedev module is updated, based on our earlier code review. The commits are bite-sized, so they should be self-explanatory. Please feel free to let me know if you have any questions or comments.

I was not sure whether we decided to merge lebedev_npoints & lebedev_degrees lists into a dictionary (with keys corresponding to degrees and values corresponding to size), so I left it as it is.

codecov[bot] commented 3 years ago

Codecov Report

Merging #111 (63856bc) into master (d87db88) will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #111   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           14        14           
  Lines         1388      1388           
=========================================
  Hits          1388      1388           
Impacted Files Coverage Δ
src/grid/atomgrid.py 100.00% <100.00%> (ø)
src/grid/lebedev.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 d87db88...63856bc. Read the comment docs.

PaulWAyers commented 3 years ago

I have a preference for using a dictionary for degrees/number-of-points but perhaps there was a good reason for doing it this way instead.

I did a very fast look and it seems like a big improvement.

tczorro commented 3 years ago

@PaulWAyers You're right. Using dict to link the degree and n_points on each shell could accelerate the lookup process. In order to support arbitrary input, I implemented the "linear search" during the refactor. But I think we can apply binary search to accelerate this procedure as well.

So I will enhance this module by applying these two ideas. Add two dict to map degree -> n_points and n_points -> degree when given a number, lookup the corresponding dict O(1), if not in it, using binary search to find the degree/n_points one level larger than given one O(log(n))

But the above idea is beyond the scope of this PR, I will merge this one first then work on the improvement