BruteForceOrderMatching class in molecule_matcher module - very basic example - error

[mhsiron]

Describe the bug The BruteForceOrderMatching class in the molecule_matcher module appears to return erroneous results for simple molecule matching examples (ie with 2 molecule, with some simple transformations between them).

To Reproduce Example 1:

from pymatgen.core import Molecule
from pymatgen.analysis.molecule_matcher import BruteForceOrderMatcher
from pymatgen.core import SymmOp
c_cl_x = Molecule(['C', 'Cl'], [[0,0,0], [1.14,0,0]])
c_cl_y = Molecule(['C', 'Cl'], [[0,0,0], [0,1.14,0]])
bfom = BruteForceOrderMatcher(c_cl_x)
c_cl_y_tr = c_cl_y.copy()
c_cl_y_tr.apply_operation(SymmOp.from_rotation_and_translation(bfom.match(c_cl_y)[1], bfom.match(c_cl_y)[2]))

The resulting rotation matrix leads to a c_cl_y_tr which is incorrectly flipped. Here one could apply the inverse of the rotation matrix, but this can lead to worse results for other simple roations

Example 2:

c_cl_x = Molecule(['C', 'Cl'], [[0,0,-1], [1.14,0,-1]])
c_cl_y = Molecule(['C', 'Cl'], [[1,0,1], [1,1.14,1]])
bfom = BruteForceOrderMatcher(c_cl_x)
c_cl_y_tr = c_cl_y.copy()
c_cl_y_tr.apply_operation(SymmOp.from_rotation_and_translation(bfom.match(c_cl_y)[1], bfom.match(c_cl_y)[2]))

This leads to an incorrect translation vector for the y component, here, taking the negative of the rotation matrix does not help, like in the previous example. The issue this time is with the translation vector and rotation matrix.

Expected behavior The following code:

c_cl_y_tr = c_cl_y.copy()
c_cl_y_tr.apply_operation(SymmOp.from_rotation_and_translation(bfom.match(c_cl_y)[1], bfom.match(c_cl_y)[2]))

Should reproduce c_cl_x

Desktop (please complete the following information):

• OS: Windows
• Version 2022.0.8

[mkhorton]

Thanks for the report @mhsiron.

@fekad, do you have any thoughts on this one?

[fekad]

Thanks for the report @mhsiron.

I have some notes:

• Be careful to use BruteForceOrderMatcher with SymmOp.from_rotation_and_translation. This method also matches the indices of atoms so if you ignore them you can easily get weird results. You can use KabschMatcher insteed.
• The order of rotation and translation are matter. You can use always use fit method to do the actual fitting:

from pymatgen.core import Molecule
from pymatgen.analysis.molecule_matcher import BruteForceOrderMatcher
from pymatgen.core import SymmOp

c_cl_x = Molecule(['C', 'Cl'], [[0,0,0], [1.14,0,0]])
c_cl_y = Molecule(['C', 'Cl'], [[0,0,0], [0,1.14,0]])
bfom = BruteForceOrderMatcher(c_cl_x)
c_cl_x, bfom.fit(c_cl_y)[0]

c_cl_x = Molecule(['C', 'Cl'], [[0,0,-1], [1.14,0,-1]])
c_cl_y = Molecule(['C', 'Cl'], [[1,0,1], [1,1.14,1]])

bfom = BruteForceOrderMatcher(c_cl_x)
c_cl_x, bfom.fit(c_cl_y)[0]

• if you still want to use SymmOp.from_rotation_and_translation please check the "direction" of the rotation:

import numpy as np
from pymatgen.core import Molecule
from pymatgen.analysis.molecule_matcher import BruteForceOrderMatcher
from pymatgen.core import SymmOp

c_cl_x = Molecule(['C', 'Cl'], [[0,0,0], [1.14,0,0]])
c_cl_y = Molecule(['C', 'Cl'], [[0,0,0], [0,1.14,0]])
bfom = BruteForceOrderMatcher(c_cl_x)
inds, U, V, rmsd = bfom.match(c_cl_y)


c_cl_y_tr = c_cl_y.copy()
c_cl_y_tr.apply_operation(SymmOp.from_rotation_and_translation(np.linalg.inv(U), V))
c_cl_x, c_cl_y_tr

c_cl_x = Molecule(['C', 'Cl'], [[0,0,-1], [1.14,0,-1]])
c_cl_y = Molecule(['C', 'Cl'], [[1,0,1], [1,1.14,1]])

bfom = BruteForceOrderMatcher(c_cl_x)
inds, U, V, rmsd = bfom.match(c_cl_y)

c_cl_y_tr = c_cl_y.copy()
c_cl_y_tr.apply_operation(SymmOp.from_rotation_and_translation(np.linalg.inv(U), V))
c_cl_x, c_cl_y_tr

[fekad]

it can work with the other direction namely rotating "x" into "y":

c_cl_x = Molecule(['C', 'Cl'], [[0,0,-1], [1.14,0,-1]])
c_cl_y = Molecule(['C', 'Cl'], [[1,0,1], [1,1.14,1]])

bfom = BruteForceOrderMatcher(c_cl_x)
inds, U, V, rmsd = bfom.match(c_cl_y)

c_cl_x_tr = c_cl_x.copy()
c_cl_x_tr.apply_operation(SymmOp.from_rotation_and_translation(U, -np.dot(U, V)))
c_cl_y, c_cl_x_tr

Here we also need to replace the order of translation and rotation.

Although the choice of directions/order was determined based on the previous implementations and it is just a convention, it would make sense to the same one as in SymmOp.from_rotation_and_translation.