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 AtomGrid #115

Closed FarnazH closed 3 years ago

FarnazH commented 3 years ago

The current changes update the docstrings of AtomGrid to make them complete and consistent. In addition, a few argument names are changes to add clarity. Before making any further changes, I wanted to clarify a few things regarding the AtomGrid constructors which are exemplifiedd below:

from grid.atomgrid import AtomGrid
from grid.onedgrid import GaussChebyshev
from grid.rtransform import BeckeTF

oned = GaussChebyshev(30)
rgrid = BeckeTF(1.e-4, 1.5).transform_1d_grid(oned)

# option 1 (using degrees of Levedev grid)
g = AtomGrid(rgrid, degrees=[11])

# option 2 (using atomic number & preset)
g = AtomGrid.from_predefined(rgrid, 1, 'coarse')

# option 2 (using atomic radius & sectors)
g = AtomGrid.from_pruned(rgrid, 0.5, sectors_r=[0.5, 1., 1.5], sectors_degree=[3, 7, 5, 3])
  1. Considering the fact that the AtomGrid.from_predefined loaded precomputed sectors_r & sectors_size, shouldn't it just call AtomGrid.from_pruned upon loading the sectors information? This is not how AtomGrid.from_predefined is currently implemented. So, I am a bit confused.
  2. In HORTON2, the coarse built-in grid specifies the radial grid points as well. See: http://theochem.github.io/horton/2.1.1/tech_ref_grids.html?highlight=insane This is not the case for AtomGrid, so how are we going to support HORTOn2-type built-in grids?

Any comments are welcomed @tczorro, @tovrstra, and @PaulWAyers.

tczorro commented 3 years ago
  1. You are right. It should just call AtomGrid.from_pruned to construct the instance. I think I wrote that code just to double check the result are consistent between the two methods incase errors or bugs in one or the other.

  2. We still support the same grid defined in HT2 as long as the radial is provided by the user. When I design grid, I believe, the new radial grids we implemented gonna perform better than the old power or exp radial. So it's less meaningful to offer an inferior default choice.

PaulWAyers commented 3 years ago

Pruned grids should have a specified number of radial points for each atom; the specific choice of radial grid and transformation should be user-specified, though one could argue for using the same default as is usually done.

So

  1. We need to specify not only sectors and the Lebedev degrees/sector, but the total number of radial points for each atom (in each pruned grid). This tends to be relatively simple rules. The Ochsenfeld pruned grids are a nice example, as they come in a format that is convenient for us.
  2. Optionally we could also keep track of what the pruned grid from the literature used as an underlying radial grid+transformation. I would tend not to do this, but instead to default to the best radial grid (whatever @rayhe88 finds) and use that instead.
tczorro commented 3 years ago

@FarnazH Could you update the black style so I can merge the commit asap

codecov[bot] commented 3 years ago

Codecov Report

Merging #115 (0022aa2) into master (a2bb522) will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #115   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           14        14           
  Lines         1389      1389           
=========================================
  Hits          1389      1389           
Impacted Files Coverage Δ
src/grid/molgrid.py 100.00% <ø> (ø)
src/grid/atomgrid.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 a2bb522...0022aa2. Read the comment docs.