migrate symmetry perception algo's away from Molecule? #70
Labels
Status: Stale
This issue is out-of-date and may no longer be applicable
Comments
|
I think this is a good idea. Perhaps a new module rmgpy.symmetry? We could also make a whole rmgpy.molecule subpackage to encapsulate everything (symmetry, geometry, isomorphism, drawing, etc.) At any rate, looks like I haven't done much Cythonizing of those functions, so you can probably leave them as pure Python for the time being. |
enochd
added a commit
to enochd/RMG-Py
that referenced
this issue
May 16, 2014
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
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
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
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
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.
pierrelb
added
the
Status: Stale
|
Symmetry has been migrated to rmgpy/molecule/summetry |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Not an error but rather an 'idea' for rmg-py's architecture
I was currently revising the symmetry perception algorithms in rmg-py. They consist of several sub sections (atom, bond, axis, etc) as envisioned by Joanna Yu if I am not mistaken. In RMG-Java many of the molecule symmetry perception algorithms are slightly modified to internal rotors (as I am currently experiencing in trying to port those algorithm's to rmg-py).
Would you not consider removing all the perception algorithms from Molecule into a separate dedicated calculator type? Molecule is already getting quite heavy-weight (if you ask me) and symmetry perception is not really a core part. Moreover, then we could have a look how to unify both the rotor symmetry perception algo's and the regular molecule symmetry algo's into more generally applicable procedures.
Just my two cents... :)
The text was updated successfully, but these errors were encountered: