Fluorine fixes#1656
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1656 +/- ##
==========================================
- Coverage 41.68% 41.67% -0.02%
==========================================
Files 176 176
Lines 29374 29374
Branches 6059 6059
==========================================
- Hits 12246 12241 -5
- Misses 16259 16263 +4
- Partials 869 870 +1
Continue to review full report at Codecov.
|
Otherwise when trying to make a sample molecule with an F groupAtom, it fails (and crashes with a segfault)
alongd
left a comment
There was a problem hiding this comment.
Thanks!
Please see a couple of comments
|
|
||
| #these are ordered on priority of picking if we encounter a more general atomType for make | ||
| allElements=['H', 'C', 'O', 'N', 'S', 'Si', 'Cl', 'Ne', 'Ar', 'He', 'X'] | ||
| allElements=['H', 'C', 'O', 'N', 'S', 'Si', 'Cl', 'F', 'Ne', 'Ar', 'He', 'X'] |
There was a problem hiding this comment.
While at it, and although beyond the scope of the PR title, perhaps add I as well?
BTW, if Ar is here, should we also consider He and Ne?
There was a problem hiding this comment.
Good point with I. Added in subsequent commit.
He and Ne are already listed.
| atomTypes['F' ] = AtomType('F', generic=['R','R!H','Val7'], specific=['F1s']) | ||
| atomTypes['F1s'] = AtomType('F1s', generic=['R','R!H','F','Val7'], specific=[], | ||
| single=[0,1], allDouble=[0], rDouble=[], oDouble=[], sDouble=[], triple=[0], benzene=[0], lonePairs=[3], charge=[0]) | ||
| single=[0,1], allDouble=[0], rDouble=[], oDouble=[], sDouble=[], triple=[0], quadruple=[0], benzene=[0], lonePairs=[3], charge=[0]) |
There was a problem hiding this comment.
The default value for quadruple is an empty list. Shouldn't that be enough (I guess not from your experience with F, but I wonder why)?
Perhaps a quicker fix for all elements would be to change the default behaviour of AtomType.
I suggest editing line 118 to:
self.quadruple = quadruple or [0]
so zero quadruple bonds becomes the default.
There was a problem hiding this comment.
Actually, we don't know that the default was causing a problem, we just thought we'd make Fluorine the same as Chlorine. I think our actual problem was solved by the other change.
This list should cover all elements used in atomTypes.
A couple of small atomType fixes for Fluorine, that were causing segfaults when testing the database ReactionMechanismGenerator/RMG-database#317