-
Notifications
You must be signed in to change notification settings - Fork 45
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
Alignment issues in Macrocyles from two building blocks. #330
Comments
An additional issue, but not a breaking issue. Building blocks are not aligned such that their core atoms are away from the centre of the topology. This can be fixed with something like this for def place_building_block(self, building_block, edges):
assert (
building_block.get_num_functional_groups() == 2
), (
f'{building_block} needs to have exactly 2 functional '
'groups but has '
f'{building_block.get_num_functional_groups()}.'
)
building_block = building_block.with_centroid(
position=self._position,
atom_ids=building_block.get_placer_ids(),
)
fg0, fg1 = building_block.get_functional_groups()
fg0_position = building_block.get_centroid(
atom_ids=fg0.get_placer_ids(),
)
fg1_position = building_block.get_centroid(
atom_ids=fg1.get_placer_ids(),
)
building_block = building_block.with_rotation_between_vectors(
start=fg1_position - fg0_position,
target=[-1 if self._flip else 1, 0, 0],
origin=self._position,
)
building_block = building_block.with_rotation_about_axis(
angle=self._angle-(np.pi/2),
axis=np.array([0, 0, 1]),
origin=self._position,
)
# Orient building blocks away from centre of cycle.
core_centroid = building_block.get_centroid(
atom_ids=building_block.get_core_atom_ids(),
)
edge_centroid = (
sum(edge.get_position() for edge in edges) / len(edges)
)
building_block = building_block.with_rotation_between_vectors(
start=edge_centroid-core_centroid,
target=self._position,
origin=self._position,
)
return building_block.get_position_matrix() |
Your second issue is probably a duplicate of #15 |
TBH I would rather fix this edge case then just raise an error |
So, the topology graph defines vertices and edges based on the init. In this case, it has two edges that are at the same position, hence the overlap (because edge-fg assignment is incorrect). |
Ah right. Cool, so if this was to be fixed, we would have to change the positions of the edges when this case comes up. I guess that's not too complicated? What do you think? |
That was the plan - to basically have a "number of cycles == 2" case for the edges. But did not have time, wanted to just have the warning for users until then. |
In that case, if you change the exception in #332 to a warning, then yeah I think that's fine for it to be merged. The exception prevents users from using it, which I think is too restrictive, because I imagine that many optimization algos, like MD for example, would pretty easily be able to undo the "twist". |
Done! |
When making a Macrocycle from only two building blocks, there is no handling of edge assignment when both edges are on the same position (calculated in Macrocyle.py on init based on the number of bbs in the chain).
MWE on my machine shows crossing bonds:
The text was updated successfully, but these errors were encountered: