-
Notifications
You must be signed in to change notification settings - Fork 49
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
lots of code to identity and flip TMCs to new symmetry groups #297
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #297 +/- ##
==========================================
- Coverage 37.23% 37.11% -0.13%
==========================================
Files 89 90 +1
Lines 29979 30374 +395
==========================================
+ Hits 11163 11273 +110
- Misses 18816 19101 +285
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Can you add some brief comments around line 6376 indicating what that code does? It seems there is code there that is copy+pasted throughout the function, so it would be better to make a helper function and just call that. Also, adding a test for the new flip_symmetry
function would also be good
Code that shows up a lot in the flip_symmetry
function:
vec1 = np.array(tmc_mol.atoms[lig1_catoms[swap_idx_1]].coords()) - np.array(tmc_mol.atoms[metal_idx].coords())
vec3 = np.array(tmc_mol.atoms[lig3_catoms[swap_idx_3]].coords()) - np.array(tmc_mol.atoms[metal_idx].coords())
atoms_to_move_b = lig1_atoms[swap_idx_1] + lig3_atoms[swap_idx_3]
and
vec_reflect_b = vec1 / np.linalg.norm(vec1, 2) + vec3 / np.linalg.norm(vec3, 2)
vec_reflect_b = vec_reflect_b / np.linalg.norm(vec_reflect_b, 2)
for atom_idx in atoms_to_move_b:
vec_atoms = np.array(tmc_mol.atoms[atom_idx].coords()) - np.array(tmc_mol.atoms[metal_idx].coords())
vec_proj = np.dot(vec_atoms, vec_reflect_b) * vec_reflect_b
reflected_coords = np.array(tmc_mol.atoms[metal_idx].coords()) + 2 * vec_proj - vec_atoms
tmc_mol.atoms[atom_idx].setcoords(reflected_coords)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Down the line, a pytest for flip_symmetry
would be nice, but the changes you have made I think have improved the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! A few small changes would be nice:
- In
get_symmetry
, if the user asks fordetails
, but the structure does not have 2 or 3 unique ligands, you will run into an error. Adding in a check for this would be good. - In
reflect_coords
, I believe that you edit themol3D
object passed in to the function. This may lead to unintended behavior, and I recommend usingcopymol3D
instead. However, as long as you document it, this should be fine.
lots of code to identity and flip TMCs to new symmetry groups