Charged atom types #1206
Merged
Charged atom types #1206
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nyee
added
Type: Feature
Status: Ready for Review
Codecov Report
@@ Coverage Diff @@
## master #1206 +/- ##
==========================================
- Coverage 43.8% 43.52% -0.28%
==========================================
Files 170 170
Lines 28166 28242 +76
Branches 5496 5495 -1
==========================================
- Hits 12338 12293 -45
- Misses 14994 15116 +122
+ Partials 834 833 -1
Continue to review full report at Codecov.
|
alongd
force-pushed
the
chargedAT
branch
12 times, most recently
from
b953d14 to
ee7c31e
Compare
alongd
force-pushed
the
chargedAT
branch
3 times, most recently
from
9124fa7 to
a3174a2
Compare
|
@nyee , a possible cause to the slower code compilation is the large amount of new atomTypes. I merged some charged atomTypes which are only important for resonance structures but not for reactivity/thermo (many charged resonance structures will be filtered out in a future PR), and made the new list as short as possible... |
|
@alongd, did it make much difference to the compilation speed? I know some other members have noticed compilation slow-downs unrelated to the atomtype changes, so it may not just be that. |
This method of generating all combination of atomtypes with all combination of charges/radicals does not work anymore because we now define charge/radical by atomType. Although the previous test was comprehensive, it also tested some unphysical combinations which are now forbidden (i.e., don't have corresponding atomTypes). For now I just commented-out the tests. Perhaps this should be revisited at some point.
Previously we would transform and lone pairs to biradicals in solvation thermo estimation. Now that we have more specific atomTypes with lone pairs specified it is unnecessary, although we may still lack the data for carbenes. This commit avoids saturating lonePairs if the atomType of C is C2tc, which means the molecule is carbon monoxide or isoelectronic to carbon monoxide.
This method is used to repalce radicals with an H for HBI corrections. The new name is less ambigious.
Now that we update chargedAtoms when we do Molecule.update(), some functions for the Heuristic ring poly ring corrections are broken. One thing that breaks is that previously correctly atomtyped atoms in submols get charges unless we saturate with Hs. This commit adds H saturation to these cases. This block of code was added to the functions: thermo.__addPolyRingCorrectionThermoDataFromHeuristic thermo.getBicyclicCorrectionThermoDataFromHeuristic
Removed some trailing white space. Already refactored three functions: saturate_radicals saturate_unfilled_valence saturate_rng_bonds from camel case to underscores as agreed upon for style.
Regardless of the direction in generateN5dd_N5tsResonanceStructures(), atom2 is always the one onto which the incrementLonePairs() action is applied. This commit verifies that atom2 is not carbon so no carbenes are formed.
The 'C#[O-]' structure is already forbidden since its O atom doesn't fit any current atomType.
since it isn't physical from test_data NC chemkin file O2sn is an oxygen atom with 2 lone pairs, 3 single bonds and a negative charge -1
mliu49
previously approved these changes
Feb 2, 2018
|
Ok, you can remove the temporary commits here and on RMG-database. I'll merge both branches after that's done. |
|
Done! |
This was referenced Jun 12, 2018
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This must be pulled in simultaneously with the RMG-database PR also named chargedAT. Be sure to remove the first commit redirecting the travis build before merging
This PR adds the charge attribute to atomtypes with the goal to increase the diversity of nitrogen and sulfur groups we can represent. This also had some effect on the atomtypes for carbon monoxide. Additionally, we have renamed carbon and oxygen atomtypes to conform to the format of nitrogen and sulfur.
This was a major functional change, so many coding edits (besides refactoring atom type names in code and test database) were also included. Some of the major ones are listed below: