Skip to content

Conversation

@shubhamachary56821
Copy link
Contributor

done some minor changes for fixing negative shape in Gaussian Cube, if issue still persist, please inform and guide

done some minor changes for  fixing negative shape in Gaussian Cube, if issue still persist, please inform and guide
Copy link
Collaborator

@Ali-Tehrani Ali-Tehrani left a comment

Choose a reason for hiding this comment

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

Thank you for the pull-request and for implementing this feature!
My only other comment is that
"atcoords": coordinates, will also need its unit changed and the method documentation should include that its units are converted to atomic units (Bohr) length.

# shape = np.array([shape0, shape1, shape2], int)
# axes = np.array([axis0, axis1, axis2])
# Detect Bohr vs. Angstrom units
BOHR_TO_ANGSTROM = 0.529177
Copy link
Collaborator

Choose a reason for hiding this comment

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

I recommend extending the precision to 16 digits, since our calculations often rely on 64-bit floating point representations.
Using scipy and mpmath, the value for converting 1 Bohr to Angstrom is 0.529177210902999962.

But since we have SciPy as a prerequisite, i think it's better to do

import scipy.constants as spc
BOHR_TO_ANGSTROM = spc.value('atomic unit of length') / spc.angstrom

@Ali-Tehrani
Copy link
Collaborator

@PaulWAyers I think that it is more straightforward to just return a flag, like units: "bohr" (or "angstrom"), to cube_data instead of defaulting to converting everything to atomic units.

If user specifically cares about atomic units, then they can handle the conversion themselves. For example I know in protein-related work, they would prefer to keep the units that they are in.

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.

2 participants