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

Vector gradient #78

Merged
merged 7 commits into from
Jul 20, 2019
Merged

Vector gradient #78

merged 7 commits into from
Jul 20, 2019

Conversation

psi-rking
Copy link
Contributor

@psi-rking psi-rking commented Jun 26, 2019

I added function to align the derivatives of vector components. For example, the code
will align dipole moment derivatives. It can convert CFOUR's DIPDER with PSI4's file17 values.

A remaining danger is that CFOUR reorders atoms when computing the gradients. Unless one attends carefully to the atomic numbers in DIPDER, or processes carefully the symmetry sorting presented in the output file, one might not even notice. I've attached a sample output and dipder file. The geometry does not appear to be printed anywhere in the order that was used in DIPDER, which means reliable atom mapping will be a challenge.

c4-vib.txt
c4-dipder.txt

The dipole moment derivatives are presented in the output file too. Only to 6 decimals.

Does the same problem emerge in a Hessian computation? Perhaps you have solved it.

a function to align for dipole moment derivatives
a function to align for dipole moment derivatives
…o vectorGradient

Conflicts:
	qcelemental/models/align.py
	qcelemental/molutil/test_align.py
Copy link
Collaborator

@loriab loriab left a comment

Choose a reason for hiding this comment

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

glad to have further alignment functionality. especially with test!

qcelemental/molutil/test_align.py Show resolved Hide resolved
al_mu_x[self.atommap[at]][:] = Datom[0,:]
al_mu_y[self.atommap[at]][:] = Datom[1,:]
al_mu_z[self.atommap[at]][:] = Datom[2,:]
return (al_mu_x, al_mu_y, al_mu_z)
Copy link
Collaborator

Choose a reason for hiding this comment

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

since inputs and outputs are xyz components, would it make sense to either do three separate calls or pass as (3 * nat, 3)? like to take better advantage of broadcasting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I like about the 3 explicit arguments is that it is obvious to any user what should be passed, particularly for a mixed-derivative like this - versus the challenge of a new person (or myself in 6 months:) trying to figure out what the single variable data structure should be. For the Hessian you would guess correctly, maybe not for this. But I'm fine changing it if you think there's a compelling reason and relying on examples, tests, and documentation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry for the delay -- it's been a crazy month. talking it over, we think the packed arrangement would be more in keeping with the other functions. the first line of the fn can be mu_x, mu_y, mu_z = *mu and the return can be return al_mu and the test call can be p2c_dipder_x, p2c_dipder_y, p2c_dipder_z = mill.align_vector_gradient(...)

qcelemental/models/align.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jun 27, 2019

Codecov Report

Merging #78 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Copy link
Collaborator

@loriab loriab left a comment

Choose a reason for hiding this comment

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

Thanks much, especially for the careful test case. We'll run yapf over it and other formatting details later.

@@ -1 +1,3 @@
from .align import B787, kabsch_align, compute_scramble
from .test_align import test_hessian_align, test_vector_gradient_align
Copy link
Collaborator

Choose a reason for hiding this comment

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

this shouldn't be needed as we don't want the tests to be part of the module proper. if you let me know the error message that led to this addition, I can probably suggest an alternate solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just wanted to be able to run the tests from an external directory. If they are not here, how do you do that? I was just testing with:

sys.path.insert(0, '/home/rking/programs/QCElemental')
import qcelemental as qcel
qcel.molutil.test_vector_gradient_align()

Copy link
Collaborator

Choose a reason for hiding this comment

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

Try just “pytest” once “import qcelemental as qcel” works.

@dgasmith dgasmith merged commit 7422123 into MolSSI:master Jul 20, 2019
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.

None yet

3 participants