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

Trim coordinate_transform.py, new features for spin method #1054

Merged
merged 6 commits into from
Nov 2, 2022

Conversation

daico007
Copy link
Member

@daico007 daico007 commented Sep 9, 2022

PR Summary:

Remove the removed methods from coordinate_transform.py. Add anchor option for Compound.spin(), so now the compound can choose an anchor to be any of its successors instead of just its center.

Pending unit tests.

PR Checklist


  • Includes appropriate unit test(s)
  • Appropriate docstring(s) are added/updated
  • Code is (approximately) PEP8 compliant
  • Issue(s) raised/addressed?

@codecov
Copy link

codecov bot commented Sep 9, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.56%. Comparing base (ba6fead) to head (9bf4c3b).
Report is 145 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1054      +/-   ##
==========================================
+ Coverage   90.53%   90.56%   +0.02%     
==========================================
  Files          60       60              
  Lines        8423     8406      -17     
==========================================
- Hits         7626     7613      -13     
+ Misses        797      793       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@daico007 daico007 merged commit 2a27c10 into mosdef-hub:main Nov 2, 2022
Copy link
Contributor

@CalCraven CalCraven left a comment

Choose a reason for hiding this comment

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

Was testing out the PR on some more complicated stuff. Feel free to add these tests in!
This works:

mol = mb.load("CCCCCCO", smiles=True)
com = mol.center
for i in range(100):
    mol.spin(np.pi/2, [1,1,1], None)
    assert np.allclose(mol.center, com)

Which is to be expected because spinning by the center of mass shouldn't change the center of mass.
And this works

import mbuild as mb
import numpy as np

cpd = mb.Compound(name="center")
part = mb.Compound(name="C", pos=[0,0,0])
cpd.add(part)
mol = mb.load("CCCCCCO", smiles=True)
pos = [0,2,2]
mol.translate(pos)
com = mol.center
cpd.add(mol)

theta = np.pi/2
around = [0,0,1]
anchor = cpd.children[-1]

cpd.spin(theta, around, anchor)
assert np.allclose(cpd.children[0].pos, [2,2,0])
assert np.allclose(cpd.children[-1].center, com)

cpd.spin(theta, around, anchor)
assert np.allclose(cpd.children[0].pos, [0,4,0])
assert np.allclose(cpd.children[-1].center, com)

anchor = cpd.children[0] #set a new anchor
cpd.spin(theta, around, anchor)
assert np.allclose(cpd.children[0].pos, [0,4,0])
assert np.allclose(cpd.children[-1].center, com+np.array([2,2,0]))

@@ -2425,12 +2425,23 @@ def spin(self, theta, around):
The angle by which to rotate the Compound, in radians.
around : np.ndarray, shape=(3,), dtype=float
The axis about which to spin the Compound.
anchor : mb.Compound, optional, default=None (self)
Copy link
Contributor

Choose a reason for hiding this comment

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

If you pass it a compound, it should spin around the center of mass of that compound. This way you don't have to specify particle/compound, since it'll still use the center of mass of a single bead compound (the synonym for a particle in mbuild). Also, what does the "(self)" mean? I'm inferring that this means the value of None really will use self. Not sure if this is standard to put in the function definition, but maybe could add a bit of uncertainty?

Copy link
Member Author

Choose a reason for hiding this comment

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

so if None is provided for the anchor, the Compound will rotate around its own self.pos, which will be its center of mass, this option is for when you want rotate a compound but around one particular particle instead the center of mass (e.g., rotate a chain attached to a surface, but you want the particle attached to the surface to be fixed, or else that bond will be wonky)

self.rotate(theta, around)
self.translate(center_pos)
self.translate(anchor_pos)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is smart.

thangckt added a commit to thangckt/mbuild that referenced this pull request Nov 5, 2022
Trim coordinate_transform.py, new features for spin method (mosdef-hub#1054)
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