Fix to xyz/2D loading and atom reordering in ARCSpecies #96
Conversation
Codecov Report
@@ Coverage Diff @@
## master #96 +/- ##
==========================================
- Coverage 41.48% 41.19% -0.29%
==========================================
Files 22 22
Lines 5048 5042 -6
Branches 1304 1303 -1
==========================================
- Hits 2094 2077 -17
- Misses 2619 2632 +13
+ Partials 335 333 -2
Continue to review full report at Codecov.
|
arc/species/speciesTest.py
Outdated
| self.assertTrue(mol.atomIDValid()) | ||
| self.assertTrue(res.atomIDValid()) | ||
|
|
||
| print(mol.toAdjacencyList()) |
There was a problem hiding this comment.
Were the print statements left here on purpose?
There was a problem hiding this comment.
Sorry, they're left over from debugging. I'll remove them.
arc/species/species.py
Outdated
| _, self.mol = molecules_from_xyz(xyz) | ||
| for i, atom in enumerate(self.mol.atoms): | ||
| atom.id = id_mol.atoms[i].id | ||
| # The generate_resonance_structures methods changes atom order |
There was a problem hiding this comment.
This is my typo originally. Can you change methods to method in this comment?
|
|
||
| self.assertTrue(mol.isIsomorphic(res)) | ||
| self.assertTrue(mol.isIdentical(res)) | ||
|
|
There was a problem hiding this comment.
Can you add another test for a species with more than one entry in mol_list?
For example, HSO3 has two resonance structures, you can use it's xyz:
S -0.12383700 0.10918200 -0.21334200
O 0.97332200 -0.98800100 0.31790100
O -1.41608500 -0.43976300 0.14487300
O 0.32370100 1.42850400 0.21585900
H 1.84477700 -0.57224200 0.35517700
Could we check that S and H have the same ID and are in the same order in self.mol and in all entries of self.mol_list?
| if self.mol is not None: | ||
| # self.mol should have come from another source, e.g. SMILES or yml | ||
| original_mol = self.mol | ||
| self.mol = molecules_from_xyz(xyz)[1] |
There was a problem hiding this comment.
We have a problem here (in the original code, not introduced by this PR) that when we override self.mol we might change the multiplicity. For example, molecules_from_xyz() will return multiplicity 3 for birads, but self.mol could originally be singlet. Then the ESS calculation will be affected. I'm trying to solve these issues on the master branch, it also affects isomorphism checks after optimization. If you get this in first, you won't have to rebase.
There was a problem hiding this comment.
I've had that issue, but it will usually fail the isomorphism comparison with the 2D structure. Have you found a way to fix the molecule generated from the XYZ to match the expected multiplicity?
There was a problem hiding this comment.
Yes, I'll fix that after merging this branch
Clean up code in __init__ and ensure self.mol_list is updated Fix atom id copying when generating resonance structures Add unit test for atom ids from mol_to_xyz
Before
In
__init__:self.molwasNone, then a molecule would be generated from the xyz coordinates but not saved to self, andself.mol_listwould also not be set.In
mol_from_xyz:self.molandself.mol_list, which was not the case, since the atom orders changed due to resonance structure generation.After
__init__code to callmol_from_xyzfor all non-TS speciesself.molexists moved tomol_from_xyzReviewer Notes
Please double check that my interpretation of
mol_from_xyzis correct. If you had tests for that method, it might be good to run them/add them to unit tests.