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

Default Grid Assesment #210

Merged
merged 25 commits into from
Jan 5, 2024
Merged

Conversation

Ali-Tehrani
Copy link
Collaborator

@Ali-Tehrani Ali-Tehrani commented Jan 4, 2024

This pull-requests addresses Issue #179.
The Default rgrid was obtained from Horton. This corresponds to using the UniformInteger grid and PowerRTransform with a "fine" preset for the angular component. This is stored in grid/utils.py in a dictionary with the following:

r"""
Obtained from theochem/horton/data/grids/tv-13.5.txt with the folllowing preamble

These grids were created by Toon Verstraelen in July 2013 in a second effort
to generate tuned grids based on a series of diatomic benchmarks computed
with the QZVP basis of Ahlrichs and coworkers. They are constructed to give
on average 5 digits of precision for a variety of molecular integrals, using
the Becke scheme with k=3.
"""
# Items are (rmin, rmax, npts) in angstrom, Meant for PowerRTransform

If the user provided rgrid as None, then it generates the default radial grid.
The following functions and their tests (with two added) were changed to have default rgrid options:

  • AtomGrid.from_preset
  • MolGrid.from_preset
  • MolGrid.from_size
  • MolGrid.from_pruned

@FarnazH FarnazH requested review from FarnazH and marco-2023 January 4, 2024 15:09
@Ali-Tehrani
Copy link
Collaborator Author

Added default BeckeWeights to aim_weights to all static, constructor methods

Copy link
Member

@FarnazH FarnazH left a comment

Choose a reason for hiding this comment

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

Thanks @Ali-Tehrani.

  1. Shouldn't we make the rgrid in from_pruned method optional as well?
  2. Thinking more after our discussion, I agree that we should remove *_ in arguments. It seems to have been used arbitrarily and as we are not using it anywhere else, it is best to remove it for now. Can you please remove them?

src/grid/molgrid.py Outdated Show resolved Hide resolved
@Ali-Tehrani
Copy link
Collaborator Author

The *_ is removed.

If you're referring to AtomGrid.from_prune, default radial grid isn't added because atomic numbers is not included in its signature.

@FarnazH FarnazH self-requested a review January 4, 2024 20:37
Copy link
Member

@FarnazH FarnazH left a comment

Choose a reason for hiding this comment

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

I removed * from the rest of AtomGrid class, which caused an API change (see cc61d59). If you have no further comments, feel free to merge this PR.

@marco-2023
Copy link
Collaborator

marco-2023 commented Jan 4, 2024

I did not find a way to add a commit to this PR. The only change that I would make is to substitute the values in line 198 of AtomGrid:

                rmin, rmax = rmin * 1.8897259885789, rmax * 1.8897259885789

by the corresponding scipy constants

                # convert angstrom to bohr
                ang2bohr = scipy.constants.angstrom / scipy.constants.value("atomic unit of length")
                rmin, rmax = rmin * ang2bohr, rmax * ang2bohr

just for consistency with the molgrid module.

@FarnazH FarnazH merged commit 6ae8e6f into theochem:master Jan 5, 2024
7 checks passed
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