Skip to content

RDKit change introduced some failing unit tests. #141

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.

Sign up for GitHub

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

Jump to bottom
Closed
rwest opened this issue Aug 8, 2013 · 1 comment
Closed

RDKit change introduced some failing unit tests. #141

rwest opened this issue Aug 8, 2013 · 1 comment

Comments

@rwest
Copy link
Member

rwest commented Aug 8, 2013

The change to RDKit (in #135) introduced/revealed some errors.
These can be seen in the automatic build robot's unit test log:
448e852#diff-6

They are mostly molecule and atomtype checking.
@rwest
Copy link
Member Author

rwest commented Nov 25, 2013

My series of commits this weekend has fixed most of these.
What remains is symmetry and thermo checking of benzene, because the unit test specifies it via SMILES='c1ccccc' and we Kekulize it when reading.

@rwest rwest closed this as completed Nov 25, 2013
enochd added a commit to enochd/RMG-Py that referenced this issue May 16, 2014
@enochd
added aromaticity recognition for symmetry calcs
3a73cff 
Aromaticity is now perceived in calculating cyclic symmetry numbers.
This commit copies a molecule inside `calculateCyclicSymmetryNumber`,
turns it into an rdkit object, finds all the aromatic bonds and atoms.
The same bonds and atoms in the corresponding rmg molecule object are
then redifined so that the alogorithm is using the correct information.

There is one strange thing about this fix that I dont yet understand.
The unittest thinks cyclic symmetry number of dimethylbenzene is 1
(should be 2). On my local machine, the same
`calculateCyclicSymmetryNumber` function gets it correct, at 2. Also,
i'm inadvertantly printing a bit to stdout, and not sure why it's
happening?

relevant to issues ReactionMechanismGenerator#12, ReactionMechanismGenerator#24, ReactionMechanismGenerator#119, ReactionMechanismGenerator#70, ReactionMechanismGenerator#141
connie pushed a commit to connie/RMG-Py that referenced this issue Jul 17, 2014
@enochd @connie
added aromaticity recognition for symmetry calcs
7cddd07 
Aromaticity is now perceived in calculating cyclic symmetry numbers.
This commit copies a molecule inside `calculateCyclicSymmetryNumber`,
turns it into an rdkit object, finds all the aromatic bonds and atoms.
The same bonds and atoms in the corresponding rmg molecule object are
then redifined so that the alogorithm is using the correct information.

There is one strange thing about this fix that I dont yet understand.
The unittest thinks cyclic symmetry number of dimethylbenzene is 1
(should be 2). On my local machine, the same
`calculateCyclicSymmetryNumber` function gets it correct, at 2. Also,
i'm inadvertantly printing a bit to stdout, and not sure why it's
happening?

relevant to issues ReactionMechanismGenerator#12, ReactionMechanismGenerator#24, ReactionMechanismGenerator#119, ReactionMechanismGenerator#70, ReactionMechanismGenerator#141

fixup! added aromaticity recognition for symmetry calcs

Better to avoid casting to string if we can - just check if it is None.

fixup! added aromaticity recognition for symmetry calcs

I think this could have been nasty!
atomType is a mutable object. Suppose it was a Cds atom type. What your code would have done,
is change the label on all Cds atom types to be 'Cb'.
   atom1 -> AtomTypeCds -> label -> 'Cds'
would become
   atom1 -> AtomTypeCds -> label -> 'Cb'
instead of
   atom1 -> AtomTypeCb -> label -> 'Cb'

Because the AtomType objects are shared, I think it would have changed them everywhere,
not just in this copy of this molecule.
enochd added a commit to enochd/RMG-Py that referenced this issue Jul 17, 2014
@enochd
added aromaticity recognition for symmetry calcs
d14d7fc 
Aromaticity is now perceived in calculating cyclic symmetry numbers.
This commit copies a molecule inside `calculateCyclicSymmetryNumber`,
turns it into an rdkit object, finds all the aromatic bonds and atoms.
The same bonds and atoms in the corresponding rmg molecule object are
then redifined so that the alogorithm is using the correct information.

There is one strange thing about this fix that I dont yet understand.
The unittest thinks cyclic symmetry number of dimethylbenzene is 1
(should be 2). On my local machine, the same
`calculateCyclicSymmetryNumber` function gets it correct, at 2. Also,
i'm inadvertantly printing a bit to stdout, and not sure why it's
happening?

relevant to issues ReactionMechanismGenerator#12, ReactionMechanismGenerator#24, ReactionMechanismGenerator#119, ReactionMechanismGenerator#70, ReactionMechanismGenerator#141
connie pushed a commit to connie/RMG-Py that referenced this issue Jul 18, 2014
@enochd @connie
added aromaticity recognition for symmetry calcs
0f4f605 
Aromaticity is now perceived in calculating cyclic symmetry numbers.
This commit copies a molecule inside `calculateCyclicSymmetryNumber`,
turns it into an rdkit object, finds all the aromatic bonds and atoms.
The same bonds and atoms in the corresponding rmg molecule object are
then redifined so that the alogorithm is using the correct information.

There is one strange thing about this fix that I dont yet understand.
The unittest thinks cyclic symmetry number of dimethylbenzene is 1
(should be 2). On my local machine, the same
`calculateCyclicSymmetryNumber` function gets it correct, at 2. Also,
i'm inadvertantly printing a bit to stdout, and not sure why it's
happening?

relevant to issues ReactionMechanismGenerator#12, ReactionMechanismGenerator#24, ReactionMechanismGenerator#119, ReactionMechanismGenerator#70, ReactionMechanismGenerator#141

fixup! added aromaticity recognition for symmetry calcs

Better to avoid casting to string if we can - just check if it is None.

fixup! added aromaticity recognition for symmetry calcs

I think this could have been nasty!
atomType is a mutable object. Suppose it was a Cds atom type. What your code would have done,
is change the label on all Cds atom types to be 'Cb'.
   atom1 -> AtomTypeCds -> label -> 'Cds'
would become
   atom1 -> AtomTypeCds -> label -> 'Cb'
instead of
   atom1 -> AtomTypeCb -> label -> 'Cb'

Because the AtomType objects are shared, I think it would have changed them everywhere,
not just in this copy of this molecule.
nickvandewiele pushed a commit to nickvandewiele/RMG-Py that referenced this issue Mar 19, 2015
@enochd @nickvandewiele
added aromaticity recognition for symmetry calcs
a883b3c 
Aromaticity is now perceived in calculating cyclic symmetry numbers.
This commit copies a molecule inside `calculateCyclicSymmetryNumber`,
turns it into an rdkit object, finds all the aromatic bonds and atoms.
The same bonds and atoms in the corresponding rmg molecule object are
then redifined so that the alogorithm is using the correct information.

There is one strange thing about this fix that I dont yet understand.
The unittest thinks cyclic symmetry number of dimethylbenzene is 1
(should be 2). On my local machine, the same
`calculateCyclicSymmetryNumber` function gets it correct, at 2. Also,
i'm inadvertantly printing a bit to stdout, and not sure why it's
happening?

relevant to issues ReactionMechanismGenerator#12, ReactionMechanismGenerator#24, ReactionMechanismGenerator#119, ReactionMechanismGenerator#70, ReactionMechanismGenerator#141

fixup! added aromaticity recognition for symmetry calcs

Better to avoid casting to string if we can - just check if it is None.

fixup! added aromaticity recognition for symmetry calcs

I think this could have been nasty!
atomType is a mutable object. Suppose it was a Cds atom type. What your code would have done,
is change the label on all Cds atom types to be 'Cb'.
   atom1 -> AtomTypeCds -> label -> 'Cds'
would become
   atom1 -> AtomTypeCds -> label -> 'Cb'
instead of
   atom1 -> AtomTypeCb -> label -> 'Cb'

Because the AtomType objects are shared, I think it would have changed them everywhere,
not just in this copy of this molecule.
rwest added a commit to rwest/RMG-Py that referenced this issue Aug 15, 2015
@rwest
Catch and avoid another "Unable to calculate degeneracy" error.
d92f288 
Hopefully this will be fixed one day...
(ref ReactionMechanismGenerator#140 and ReactionMechanismGenerator#141)
rwest added a commit to rwest/RMG-Py that referenced this issue Aug 18, 2015
@rwest
Catch and avoid another "Unable to calculate degeneracy" error.
7d970cb 
Hopefully this will be fixed one day...
(ref ReactionMechanismGenerator#140 and ReactionMechanismGenerator#141)
nateharms pushed a commit to nateharms/RMG-Py that referenced this issue Oct 18, 2018
@rwest
Catch and avoid another "Unable to calculate degeneracy" error.
21dc5c3 
Hopefully this will be fixed one day...
(ref ReactionMechanismGenerator#140 and ReactionMechanismGenerator#141)
rwest added a commit that referenced this issue Oct 18, 2019
@rwest
Catch and avoid another "Unable to calculate degeneracy" error.
245b40a 
Hopefully this will be fixed one day...
(ref #140 and #141)
rwest added a commit that referenced this issue Oct 19, 2019
@rwest
Catch and avoid another "Unable to calculate degeneracy" error.
aaecae3 
Hopefully this will be fixed one day...
(ref #140 and #141)
rwest added a commit that referenced this issue Oct 19, 2019
@rwest
Catch and avoid another "Unable to calculate degeneracy" error.
8e94c4a 
Hopefully this will be fixed one day...
(ref #140 and #141)
rwest added a commit that referenced this issue Oct 19, 2019
@rwest
Catch and avoid another "Unable to calculate degeneracy" error.
06abd90 
Hopefully this will be fixed one day...
(ref #140 and #141)
rwest added a commit to rwest/RMG-Py that referenced this issue Nov 21, 2019
@rwest
Catch and avoid another "Unable to calculate degeneracy" error.
b2da57e 
Hopefully this will be fixed one day...
(ref ReactionMechanismGenerator#140 and ReactionMechanismGenerator#141)
rwest added a commit to rwest/RMG-Py that referenced this issue Dec 1, 2019
@rwest
Catch and avoid another "Unable to calculate degeneracy" error.
29fae73 
Hopefully this will be fixed one day...
(ref ReactionMechanismGenerator#140 and ReactionMechanismGenerator#141)
rwest added a commit to rwest/RMG-Py that referenced this issue Mar 6, 2020
@rwest
Catch and avoid another "Unable to calculate degeneracy" error.
4a1fa8c 
Hopefully this will be fixed one day...
(ref ReactionMechanismGenerator#140 and ReactionMechanismGenerator#141)
rwest added a commit to rwest/RMG-Py that referenced this issue Apr 16, 2020
@rwest
Catch and avoid another "Unable to calculate degeneracy" error.
f17980f 
Hopefully this will be fixed one day...
(ref ReactionMechanismGenerator#140 and ReactionMechanismGenerator#141)
rwest added a commit to rwest/RMG-Py that referenced this issue Sep 27, 2020
@rwest
Catch and avoid another "Unable to calculate degeneracy" error.
da7f531 
Hopefully this will be fixed one day...
(ref ReactionMechanismGenerator#140 and ReactionMechanismGenerator#141)
xiaoruiDong pushed a commit that referenced this issue Jul 27, 2021
@rwest @xiaoruiDong
Catch and avoid another "Unable to calculate degeneracy" error.
5ff73b3 
Hopefully this will be fixed one day...
(ref #140 and #141)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@rwest