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

Fix: particle indexes passed to addParticle #17

Merged
merged 2 commits into from
Oct 20, 2020

Conversation

aizvorski
Copy link
Contributor

@aizvorski aizvorski commented Sep 8, 2020

Without this fix, it appears that the particle indexes passed to force.addParticle() in restraint_force() were incorrect when selectors are used.

I've been using this code to learn how to write my own steered MD protocol, and I ran into this part while going through it. The first argument to addParticle() is a particle index; it should always be equal to the index used to subscript positions[index]. When calling enumerate in this way, in general, i is not the same as index.

The particle index is also expected to be an int; MDTrajTopology select() returns an ndarray of int64, which needs an explicit cast as done here. Omitting the cast would case NotImplementedError: Wrong number or type of arguments for overloaded function 'CustomExternalForce_addParticle'..

The original code appears to works correctly if no selectors are used, since in that case i and index would be equal. But if any selectors (eg "backbone") are used, then it would add forces to the wrong particles.

I would be grateful if you can double-check this and let me know if my understanding of what is going on is correct.

Copy link
Member

@jaimergp jaimergp left a comment

Choose a reason for hiding this comment

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

Good catch! Thanks for your submission. Please see the comment below!

ommprotocol/md.py Outdated Show resolved Hide resolved
@jaimergp jaimergp merged commit 377f493 into insilichem:master Oct 20, 2020
@aizvorski
Copy link
Contributor Author

Good catch! Thanks for your submission. Please see the comment below!

Hi Jamie - Thanks! I may have imported nanometers in my test code somewhere; of course u.nanometers is correct

I think the int() cast is also needed, if the indices come from MDTrajTopology select(). The indices are an ndarray of int64, and those don't work as an argument to addParticle(). Try something like indices = np.zeros((1,), dtype=np.int64), it should throw an exception

Best,
Alex

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

2 participants