Alignment issues in Macrocyles from two building blocks.

[andrewtarzia]

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:

import stk

bb1 = stk.BuildingBlock(
    'BrCCOCc1ccc(COCCBr)cc1',
    functional_groups=[stk.BromoFactory()],
)
bb2 = stk.BuildingBlock(
    'BrCOBr',
    functional_groups=[stk.BromoFactory()],
)
macrocycle = stk.ConstructedMolecule(
    topology_graph=stk.macrocycle.Macrocycle(
        building_blocks=(bb1, bb2),
        repeating_unit='AB',
        num_repeating_units=1,
    )
)
macrocycle.write('macrocycle.mol')

[andrewtarzia]

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 class _CycleVertex(Vertex)::

    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()

[lukasturcani]

Your second issue is probably a duplicate of #15

[lukasturcani]

Cool, I ran the example from the first post and got this

To make sure I understand, the topology graph in this case has two nodes, connected by a single edge. As a result, any two functional groups of the building blocks may get connected.

[lukasturcani]

TBH I would rather fix this edge case then just raise an error

[andrewtarzia]

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).

[lukasturcani]

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?

[andrewtarzia]

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.

[lukasturcani]

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".

[andrewtarzia]

Done!