-
Notifications
You must be signed in to change notification settings - Fork 227
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.
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
Test that we can make a sample molecule for every atom type. #1661
Conversation
Just updated to RMG 3. |
This PR is abandoned but up-to-date and simple enough that it could be merged. I am going to change the base branch to |
4e78950
to
16b09b0
Compare
Rebased and CI is now running. |
Couldn't make sample molecules for types ['Ctc', 'N5bd', 'O4b', 'P3b', 'P5b', 'P5bd', 'S4b'] |
Now down to ['N5bd', 'O4b', 'P3b', 'P5b', 'P5bd', 'S4b']. |
Got several more working. |
Codecov Report
@@ Coverage Diff @@
## main #1661 +/- ##
==========================================
- Coverage 48.11% 48.11% -0.01%
==========================================
Files 110 110
Lines 30626 30625 -1
Branches 7988 7988
==========================================
- Hits 14735 14734 -1
+ Misses 14363 14362 -1
- Partials 1528 1529 +1 see 3 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
0b18290
to
dac9888
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for bringing this one back from the dead. Approving now (tests should pass w/o problems) and then feel free to merge.
This unit test was inspired by efforts to debug something in some changes to the database, which caused a segfault when running the database Tests, but was in fact caused by an undetected bug in RMG. This should detect similar bugs. The idea is that for every atomType, such as 'Cds', the method Group().make_sample_molecule() can create a molecule from the group definition, eg. 1 Cd ux should make the molecule 1 C u0 p0 c0 {2,D} {3,S} {4,S} 2 C u0 p0 c0 {1,D} {5,S} {6,S} 3 H u0 p0 c0 {1,S} 4 H u0 p0 c0 {1,S} 5 H u0 p0 c0 {2,S} 6 H u0 p0 c0 {2,S} Many work, but quite a few fail. Updated to RMG3/Python3.
We have a hard-coded list of charged atom types here. It was missing one.
The atomtypes each have a charge list - let's just see if it matches.
When you create a molecule from a functional group definition that declares implicit benzene rings by using a "benzene" atom type, it creates benzene rings. This algorithm contains a list of "benzene" atom types. It's hard-coded. And missing some. This adds one, to check that adding one fixes things. With this fix, the following line works: rmgpy.molecule.Group().from_adjacency_list('1 N5bd ux').make_sample_molecule()
Before it had a hard-coded list (that was missing some). Now any atomtype that is declared as having at least one benzene bond is counted.
This helps generating sample molecules for certain atomtypes. Now lines like this work: rmgpy.molecule.Group().from_adjacency_list('1 P3b ux').make_sample_molecule()
Fixing O4b and S4b will be nontrivial, so rather than fix them I'm marking them as "work in progress" (expected to fail)
Ideally we could make a molecule from every atom type definition that matches that atom type. This is a work in progress test, because we can't. Two (marked in previous tests) crash. Some others create molecules, but they don't seem to match the functional group they were designed to match.
Motivation or Problem
This unit test at the start of this PR was inspired by efforts to debug something in some changes to the database (ReactionMechanismGenerator/RMG-database#317) , which caused a segfault when running the database Tests, but was in fact caused by an undetected bug in RMG (#1656) not a problem in the database. I felt there ought to have been a unit test to prevent the bug.
Description of Changes
The goal is to make a group definition out of each atom type, and then make a sample molecule for each group definition. This should test the group definition and the
make_sample_molecule
method.At first, the test failed, and that was either because of
make_sample_molecule
method.I then fixed several of the bugs - mostly in the algorithm to make aromatic rings for sample molecules.
The remaining bugs I split into a "work in progress" unit test - they involve aromatic O and S atoms (['O4b', 'S4b'] ) which seem to be poorly supported currently.
I then made a more stringent test - that not only do we generate sample molecules without crashing, but that they're correct. This test is also currently marked a "work in progress". The list of problems is Xo, Cq, Sib, Sibf, Siq. Fixing those can be a separate issue.