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

Far Away Grid Points #102

Open
FarnazH opened this issue Jul 29, 2020 · 7 comments
Open

Far Away Grid Points #102

FarnazH opened this issue Jul 29, 2020 · 7 comments
Labels
low-priority Low Priority Issue

Comments

@FarnazH
Copy link
Member

FarnazH commented Jul 29, 2020

The following example computes the maximum distance between grid points, and it looks unusually large to me! I noticed it when testing Hirshfeld weights (instead of Becke weights) for integration. They go unnoticed when using Becke weights for integration. I haven't checked whether that is the case for HORTON2, but we need to add tests for grid points coordinates and weights against HORTON2.

import numpy as np
from scipy.spatial.distance import pdist

from grid.molgrid import MolGrid
from grid.becke import BeckeWeights
from grid.onedgrid import GaussChebyshev
from grid.rtransform import BeckeTF

atnums = np.array([8, 1, 1])
atcoords = np.array([[0.0, 0.0, 0.0], [0.0, 0.0, 1.79523983], [1.69257101, 0.0, -0.59840635]])

oned = GaussChebyshev(30)
rgrid = BeckeTF(1.e-4, 1.5).transform_1d_grid(oned)
grid = MolGrid.horton_molgrid(atcoords, atnums, rgrid, 110, BeckeWeights())

# compute pairwise distance between grid points
pdist = pdist(grid.points)
print('min & max of pdist = ', np.min(pdist), np.max(pdist))

The output is: min & max of pdist = 0.0002980577 4377.9871590172

FarnazH added a commit to FarnazH/grid that referenced this issue Mar 4, 2021
It was noticed that aim_weights computed for Hirshfeld are not within
[0, 1] range (there was large negative and positive values for points
far away from the nucleus). So, instead of interpolating density,

1. The log of pro-atom density was interpolated.
2. The value of log-interpolation for points that are far away (as
reported in theochem#102) are set to
-np.inf (i.e., density at far away points are assumed to be zero).
@PaulWAyers
Copy link
Member

I think that the correct way to treat this may be to

  • prune out points that have very small weights.
  • prune out points at which some easy-to-evaluate benchmark function is sufficiently large (or small). It is difficult for me to conceive of a need to include grid points ~10 Angstrom from any atom.

@FarnazH FarnazH added this to the release milestone May 14, 2021
@tczorro
Copy link
Collaborator

tczorro commented May 20, 2021

Add rmax to OneDGrid functions

@tczorro
Copy link
Collaborator

tczorro commented May 21, 2021

@FarnazH @PaulWAyers @tovrstra I think I need a little bit clearer implementation idea. So I am thinking add a rmax to OneDGrid default init, so any points after rmax will be trimmed, or a min_wt to truncate any points with less than certain given weight.

This argument can be default to None. Then we need add a rmax or min_wt args (can be **kwargs) to be add to each onedgrid functions. Since these changes are a little duplicate repetitive, so I wander are we making any change as mentioned in #118 so that each onedgrid function are turned into a class so some common behavior can be shared.

@tovrstra
Copy link
Member

I see this would produce a lot of repetition in the onedgrid module, while the problem with far-away points is only appearing when creating molecular grids (because of the Hirshfeld partitioning). This suggests that it would be fine to support the rmax or min_wt only in higher-level classes such as AtomicGrid. One could even go further and say this is a Hirshfeld-specific issue and should therefore be resolved at that stage, just dropping points that are insensible from the Hirshfeld perspective. That would also make it easier to define a criterion for which point to discard safely.

@tczorro
Copy link
Collaborator

tczorro commented May 21, 2021

That makes a lot of sense by putting at AtomicGrid level. This could be easily handled at high end. So without really type any code, I have two implementation ideas.

  1. when constructing AtomicGrid or MolecularGrid, we set the limit and drop extra radial points
  2. Add a method(function) which return a subset of points from the complete AtomicGrid. Since we have the pt_ind ( the indices of each shell), this can also be easily implemented. Therefore, we can have more flexibility at controlling dropping points without really drop them. (more memory usage to store the complete grid.)

@tczorro tczorro removed this from the release milestone Jul 16, 2021
@tczorro
Copy link
Collaborator

tczorro commented May 17, 2022

Todo: check whether this problem exits in preset atomic grid (commented by Derrick, Farnaz, Paul)

@PaulWAyers
Copy link
Member

Add documentation. Warn users that there are (very) far away points and points with zero weights in some cases.

This isn't easy to fix for molecular grids because the indexing refers to the number of points for each atom. So it isn't easy to fix in general.

We should give some examples of how this is dealt with in examples.

@PaulWAyers PaulWAyers added wontfix This will not be worked on low-priority Low Priority Issue and removed wontfix This will not be worked on labels May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
low-priority Low Priority Issue
Projects
None yet
Development

No branches or pull requests

4 participants