From 0610c4207f9dd16cf073927bc44926840ca022a2 Mon Sep 17 00:00:00 2001 From: Richard West Date: Tue, 1 Apr 2014 23:27:58 -0400 Subject: [PATCH 1/3] Don't check atom labels when comparing Group Atoms. Label checking is done as part of the isomorphism routines. Enforcing the isSpecificTypeOf and Equivalent methods to check it just breaks the isomorphism routines, as demonstrated by the two unit tests that started failing. (Yay for unit tests!) --- rmgpy/molecule/group.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/rmgpy/molecule/group.py b/rmgpy/molecule/group.py index 33f6015a85..e3fbbe829f 100644 --- a/rmgpy/molecule/group.py +++ b/rmgpy/molecule/group.py @@ -326,8 +326,6 @@ def equivalent(self, other): if charge1 == charge2: break else: return False - #The label must be the same - if not self.label==other.label: return False # Otherwise the two atom groups are equivalent return True @@ -363,9 +361,6 @@ def isSpecificCaseOf(self, other): if charge1 == charge2: break else: return False - - #The label must be the same - if not self.label==other.label: return False # Otherwise self is in fact a specific case of other return True ################################################################################ From 2c1e7fcd9d9953cdc868010212aff9f0a5cbb2ec Mon Sep 17 00:00:00 2001 From: Richard West Date: Tue, 1 Apr 2014 23:31:47 -0400 Subject: [PATCH 2/3] Fix bugs in GroupAtom charge tests. Equivalent check needs to ensure the matches work both ways. isSpecificCaseOf only needs to check self is contained within other. (and both need to not just check self against self) --- rmgpy/molecule/group.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/rmgpy/molecule/group.py b/rmgpy/molecule/group.py index e3fbbe829f..e9eea060ae 100644 --- a/rmgpy/molecule/group.py +++ b/rmgpy/molecule/group.py @@ -320,8 +320,13 @@ def equivalent(self, other): if radical1 == radical2 and spin1 == spin2: break else: return False - #Each charge in self must have an equivalent in other + #Each charge in self must have an equivalent in other (and vice versa) for charge1 in self.charge: + for charge2 in other.charge: + if charge1 == charge2: break + else: + return False + for charge1 in other.charge: for charge2 in self.charge: if charge1 == charge2: break else: @@ -357,7 +362,7 @@ def isSpecificCaseOf(self, other): return False #Each charge in self must have an equivalent in other for charge1 in self.charge: - for charge2 in self.charge: + for charge2 in other.charge: if charge1 == charge2: break else: return False @@ -787,7 +792,6 @@ def isIdentical(self, other): The function `isIsomorphic` respects wildcards, while this function does not, make it more useful for checking groups to groups (as opposed to molecules to groups) - """ # It only makes sense to compare a Group to a Group for full # isomorphism, so raise an exception if this is not what was requested From 3af5c293a0339ba962e48b8db26cd52483aab976 Mon Sep 17 00:00:00 2001 From: Richard West Date: Tue, 1 Apr 2014 23:53:27 -0400 Subject: [PATCH 3/3] More specific unit tests for GroupAtom equivalence and isSpecificCaseOf tests. We check a variety of radicals, multiplicities, and charges. This helped find a bug I almost introduced :-) (and I think would have detected the ones I had just removed) --- rmgpy/molecule/groupTest.py | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/rmgpy/molecule/groupTest.py b/rmgpy/molecule/groupTest.py index 81e0999b87..ab4411630b 100644 --- a/rmgpy/molecule/groupTest.py +++ b/rmgpy/molecule/groupTest.py @@ -2,6 +2,7 @@ # -*- coding: utf-8 -*- import unittest +from external.wip import work_in_progress from rmgpy.molecule.group import * from rmgpy.molecule.atomtype import atomTypes @@ -153,7 +154,17 @@ def testEquivalent(self): else: self.assertFalse(atom1.equivalent(atom2), '{0!s} is equivalent to {1!s}'.format(atom1, atom2)) self.assertFalse(atom2.equivalent(atom1), '{0!s} is equivalent to {1!s}'.format(atom2, atom1)) - + # Now see if charge and radical count are checked properly + for charge in range(3): + for radicals in range(2): + atom3 = GroupAtom(atomType=[atomType1], radicalElectrons=[radicals], spinMultiplicity=[radicals + 1], charge=[charge], label='*1') + if radicals == 1 and charge == 0: + self.assertTrue(atom1.equivalent(atom3), '{0!s} is not equivalent to {1!s}'.format(atom1, atom3)) + self.assertTrue(atom1.equivalent(atom3), '{0!s} is not equivalent to {1!s}'.format(atom3, atom1)) + else: + self.assertFalse(atom1.equivalent(atom3), '{0!s} is equivalent to {1!s}'.format(atom1, atom3)) + self.assertFalse(atom1.equivalent(atom3), '{0!s} is equivalent to {1!s}'.format(atom3, atom1)) + def testIsSpecificCaseOf(self): """ Test the GroupAtom.isSpecificCaseOf() method. @@ -162,10 +173,17 @@ def testIsSpecificCaseOf(self): for label2, atomType2 in atomTypes.iteritems(): atom1 = GroupAtom(atomType=[atomType1], radicalElectrons=[1], spinMultiplicity=[2], charge=[0], label='*1') atom2 = GroupAtom(atomType=[atomType2], radicalElectrons=[1], spinMultiplicity=[2], charge=[0], label='*1') + # And make more generic types of these two atoms + atom1gen = GroupAtom(atomType=[atomType1], radicalElectrons=[0, 1], spinMultiplicity=[1, 2], charge=[0, 1], label='*1') + atom2gen = GroupAtom(atomType=[atomType2], radicalElectrons=[0, 1], spinMultiplicity=[1, 2], charge=[0, 1], label='*1') if label1 == label2 or atomType2 in atomType1.generic: self.assertTrue(atom1.isSpecificCaseOf(atom2), '{0!s} is not a specific case of {1!s}'.format(atom1, atom2)) + self.assertTrue(atom1.isSpecificCaseOf(atom2gen), '{0!s} is not a specific case of {1!s}'.format(atom1, atom2gen)) + self.assertFalse(atom1gen.isSpecificCaseOf(atom2), '{0!s} is a specific case of {1!s}'.format(atom1gen, atom2)) else: - self.assertFalse(atom1.isSpecificCaseOf(atom2), '{0!s} is not a specific case of {1!s}'.format(atom1, atom2)) + self.assertFalse(atom1.isSpecificCaseOf(atom2), '{0!s} is a specific case of {1!s}'.format(atom1, atom2)) + self.assertFalse(atom1.isSpecificCaseOf(atom2gen), '{0!s} is a specific case of {1!s}'.format(atom1, atom2gen)) + self.assertFalse(atom1gen.isSpecificCaseOf(atom2), '{0!s} is a specific case of {1!s}'.format(atom1gen, atom2)) def testCopy(self): """