Fix nitrogen symmetry #1064
Fix nitrogen symmetry #1064
Conversation
|
For some reason, the travis build threw the same error as before. It also cited the same line (116 in symmetry.py). This confuses me since now that I fixed the issue (runs fine on my computer), line 116 of symmetry.py is blank on the branch. I don't know why travis is throwing the same error as before when it runs fine on my branch. Maybe I am not understanding something or maybe Travis somehow didn't fully update/recompile properly. @KEHANG, can you check out the travis build for this PR? The commit numbers for the two branches are the same 'abb37e8', and I am not sure if that indicates there was some issue updating/syncing. All the other builds listed 2 commit numbers (example). Any idea what only having one commit number indicates? |
|
The failing message is from running NC job using benchmark version which is current master, we know that has the bug. In this case, we probably have to make sure the NC example runs successfully locally using the new branch and merge it into master. The merged master should pass RMG-tests. |
rmgpy/species.py
Outdated
| logging.warning('Resonance Hybrid: {}'.format(resonanceHybrid.toAdjacencyList())) | ||
| for index, mol in enumerate(self.molecule): | ||
| logging.warning("Resonance Structure {}: {}".format(index,mol.toAdjacencyList())) | ||
| raise |
There was a problem hiding this comment.
I guess this line is just for your debugging purpose. Maybe add exception with helpful information.
There was a problem hiding this comment.
I added the helpful note : Wrong bond order generated by resonance hybrid. Replacing the old exception with a new one will hide information within the key error.
There was a problem hiding this comment.
modified to logging.error to be consistent with error checking in #907
rmgpy/molecule/symmetry.py
Outdated
| @@ -113,7 +113,7 @@ def calculateAtomSymmetryNumber(molecule, atom): | |||
| # Two single bonds | |||
| if count == [2]: | |||
| symmetryNumber *= 2 | |||
|
|
|||
There was a problem hiding this comment.
Definitely. I just make this to try to re-trigger RMG-tests, since it wasn't functioning properly.
There was a problem hiding this comment.
commit should be removed.
|
I just tested this branch locally with NC example, as you mentioned, it runs smoothly. So I agree to merge soon. |
goldmanm
force-pushed
the
fix_nitrogen_symmetry
branch
from
fba52ab to
5119e4f
Compare
|
I just removed the commit. Let me know if anything else should be modified before rebasing onto the master branch. |
The special case for NO2 atom symmetry was removed since the updated symmetry algorithm can correctly identify the atom symmetry, with the addition to the if/elif block.
goldmanm
force-pushed
the
fix_nitrogen_symmetry
branch
from
5119e4f to
044f0d8
Compare
This fixes a Nitrogen symmetry bug by removing the special exception for NO2 (since it is no longer necessary with the updated algorithm).