Classifying geometry based on RMSD #213
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
… geometry_changes
…ment to allow for higher than 6-coordinated structures.
There was a problem hiding this comment.
Not quite sure why, but get_geometry_type_distance() is also limited to a maximum coordination number of 6. I tracked it down to the use of get_first_shell() to determine the coordination number. Is get_first_shell() somehow limited to a maximum coordination number of 6?
molSimplify/Classes/mol3D.py
Outdated
| distances.append(distance) | ||
| positions[idx, :] = np.array(atom.coords()) - np.array(metal_atom.coords()) #shift so the metal is at (0, 0, 0) | ||
|
|
||
| def permutations(list): |
There was a problem hiding this comment.
testing this function, it seems that it could be replaced by itertools.permutations()
There was a problem hiding this comment.
Which I have done in my recent changes
There was a problem hiding this comment.
I believe that get_first_shell is limited to a maximum coordination number of 6: it uses the function obtain_truncation_metal to get the first shell (before applying any edge/sandwich logic), which determines neighbors using the getBondedAtomsSmart function. This function, as shown in commit 79272c1, assumes a maximum coordination number of 6. If the default behavior of getBondedAtomsSmart is set to not assume a maximum of 6-coordinated structures, some of the tests will fail for geometry checks, so I do not want to change that behavior. However, changing the oct argument to False will allow for get_geometry_type_distance to classify geometries with coordination numbers higher than 6, which I have tested. I can introduce a workaround variable to allow for different behavior (such as in commit 46abd16), but I do not want to add that argument to more functions than needed (since readability on those extra arguments would be difficult to interpret). I would argue that we should change the test cases to add oct=True to getBondedAtomsSmart (since that function should not assume a maximum coordination number), but I am not sure if that would also require adding arguments to many functions to ensure that the test cases work as intended.
There was a problem hiding this comment.
I just spent some time trying to understand why these 4 test cases:
test_geocheck_oct[broken_ligands]test_geocheck_oct[H_transfer]test_geocheck_oct[rotational_group]test_geocheck_oct[iodine_sulfur]
fail when we set oct=False as default in getBondedAtomsSmart and got nowhere... My recommendation for now is just to merge what you have and address this issue during the hackathon on higher coordination complexes.
Added a new geometry classification function that calculates the RMSDs between the first coordination shell of mononuclear complexes and ideal geometries with the same coordination number, and classifies based on that. Should improve classifications compared to the angle deviation approach previously implemented.