Added a species constraint for the max number of rings fused together (revived)#2868
Added a species constraint for the max number of rings fused together (revived)#2868
Conversation
alongd
force-pushed
the
fused_ring_constraints
branch
from
89486d1 to
8aabf21
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds a new species constraint maximumFusedRings to limit the complexity of ring systems in generated species, allowing users to exclude molecules with complex fused rings without restricting species based solely on molecular weight. The implementation correctly distinguishes between monocycles (single rings) and fused ring systems (multiple rings sharing edges), returning 0 for monocycles and the actual ring count for fused systems.
Key changes:
- Implemented
get_ring_count_in_largest_fused_ring_system()method inMoleculeclass to count rings within fused ring systems - Added
maximumFusedRingsconstraint to species constraint system with appropriate validation - Cleaned up documentation by removing references to the unimplemented
maximumIsotopicAtomsconstraint
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| rmgpy/molecule/molecule.py | Adds new method to count rings in largest fused ring system using existing polycycle and SSSR methods |
| rmgpy/constraints.py | Integrates the new constraint check with early return optimization for acyclic molecules |
| rmgpy/rmg/input.py | Adds maximumFusedRings to list of valid species constraints |
| test/rmgpy/molecule/moleculeTest.py | Adds unit test covering acyclic, monocyclic, and fused ring molecules |
| documentation/source/users/rmg/input.rst | Documents the new constraint and removes unimplemented maximumIsotopicAtoms reference |
| examples/rmg/commented/input.py | Removes unimplemented maximumIsotopicAtoms from example |
| examples/rmg/MR_test/input.py | Removes unimplemented maximumIsotopicAtoms from example |
| documentation/source/users/rmg/modules/isotopes.rst | Removes documentation for unimplemented maximumIsotopicAtoms constraint |
| scripts/rmg2to3.py | Removes conversion mapping for unimplemented maximumIsotopicAtoms constraint |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
alongd
force-pushed
the
fused_ring_constraints
branch
from
8aabf21 to
78dd1ba
Compare
Regression Testing Results
Detailed regression test results.Regression test aromatics:Reference: Execution time (DD:HH:MM:SS): 00:00:00:57 aromatics Passed Core Comparison ✅Original model has 15 species. aromatics Passed Edge Comparison ✅Original model has 106 species. DetailsObservables Test Case: Aromatics Comparison✅ All Observables varied by less than 0.500 on average between old model and new model in all conditions! aromatics Passed Observable Testing ✅Regression test liquid_oxidation:Reference: Execution time (DD:HH:MM:SS): 00:00:01:59 liquid_oxidation Passed Core Comparison ✅Original model has 37 species. liquid_oxidation Passed Edge Comparison ✅Original model has 214 species. DetailsObservables Test Case: liquid_oxidation Comparison✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! liquid_oxidation Passed Observable Testing ✅Regression test nitrogen:Reference: Execution time (DD:HH:MM:SS): 00:00:01:02 nitrogen Passed Core Comparison ✅Original model has 41 species. nitrogen Failed Edge Comparison ❌Original model has 133 species. Non-identical thermo! ❌
thermo: Thermo group additivity estimation: group(O2s-CdN3d) + group(N3d-OCd) + group(Cd-HN3dO) + ring(Cyclopropene) + radical(CdJ-NdO) Non-identical kinetics! ❌
kinetics: DetailsObservables Test Case: NC Comparison✅ All Observables varied by less than 0.200 on average between old model and new model in all conditions! nitrogen Passed Observable Testing ✅Regression test oxidation:Reference: Execution time (DD:HH:MM:SS): 00:00:01:50 oxidation Passed Core Comparison ✅Original model has 59 species. oxidation Passed Edge Comparison ✅Original model has 230 species. DetailsObservables Test Case: Oxidation Comparison✅ All Observables varied by less than 0.500 on average between old model and new model in all conditions! oxidation Passed Observable Testing ✅Errors occurred during observable testing
WARNING:root:Initial mole fractions do not sum to one; normalizing.
|
mjohnson541
left a comment
mjohnson541
left a comment
There was a problem hiding this comment.
The algorithmic aspects all LGTM. I think we might need to be careful about how we name and describe this constraint though...I think it's easy for a reader to get confused about exactly how this constrains fused rings.
| Note that this should be done with caution, since the constraints will still apply to subsequent | ||
| products that form. | ||
|
|
||
| ``maximumFusedRings`` is the maximum number of rings in any fused ring system in the species |
There was a problem hiding this comment.
The language is tricky here. I had to think about the wording for this comment. I don't think it's clear here the distinction between the maximum number of groups of inter-fused rings allowed in the molecule (which I don't think this is constraining) and the maximum number of SSSR rings in each set of inter-fused rings. Maybe the constraint name should be something more specific too?
alongd
force-pushed
the
fused_ring_constraints
branch
2 times, most recently
from
543fd25 to
1dd122b
Compare
|
@mjohnson541, I addressed your comment, thanks! |
alongd
force-pushed
the
fused_ring_constraints
branch
from
1dd122b to
8094852
Compare
This feature was never merged into main and causes RMG to crush if specified Also removed here from the rmg2to3 script
alongd
force-pushed
the
fused_ring_constraints
branch
from
8094852 to
1cc721e
Compare
Regression Testing Results
Detailed regression test results.Regression test aromatics:Reference: Execution time (DD:HH:MM:SS): 00:00:00:56 aromatics Passed Core Comparison ✅Original model has 15 species. aromatics Failed Edge Comparison ❌Original model has 106 species. Non-identical thermo! ❌
thermo: Thermo group additivity estimation: group(Cs-(Cds-Cds)CsCsH) + group(Cs-(Cds-Cds)CsCsH) + group(Cs-(Cds-Cds)(Cds-Cds)CsH) + group(Cs-(Cds-Cds)CsHH) + group(Cds-CdsCsCs) + group(Cds-CdsCsH) + group(Cds-CdsCsH) + group(Cds-CdsCsH) + polycyclic(s3_4_5_ene_3) + polycyclic(s2_4_5_diene_1_5) + polycyclic(s3_5_5_ene_1) - ring(Cyclobutene) - ring(Cyclopentane) - ring(Cyclopentene) + radical(cyclopentene-allyl) Non-identical thermo! ❌
thermo: Thermo group additivity estimation: group(Cs-(Cds-Cds)(Cds-Cds)CsCs) + group(Cs-(Cds-Cds)CsCsH) + group(Cs-(Cds-Cds)CsCsH) + group(Cs-CsCsHH) + group(Cds-CdsCsH) + group(Cds-CdsCsH) + group(Cds-CdsCsH) + group(Cds-CdsCsH) + polycyclic(s2_4_4_ene_1) + polycyclic(s2_4_5_diene_1_5) + polycyclic(s3_4_5_ene_1) - ring(Cyclobutene) - ring(Cyclobutane) - ring(Cyclopentene) + radical(bicyclo[2.1.1]hex-2-ene-C5) Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: DetailsObservables Test Case: Aromatics Comparison✅ All Observables varied by less than 0.500 on average between old model and new model in all conditions! aromatics Passed Observable Testing ✅Regression test liquid_oxidation:Reference: Execution time (DD:HH:MM:SS): 00:00:02:00 liquid_oxidation Passed Core Comparison ✅Original model has 37 species. liquid_oxidation Failed Edge Comparison ❌Original model has 214 species. DetailsObservables Test Case: liquid_oxidation Comparison✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! liquid_oxidation Passed Observable Testing ✅Regression test nitrogen:Reference: Execution time (DD:HH:MM:SS): 00:00:01:04 nitrogen Failed Core Comparison ❌Original model has 41 species. nitrogen Failed Edge Comparison ❌Original model has 133 species. DetailsObservables Test Case: NC Comparison✅ All Observables varied by less than 0.200 on average between old model and new model in all conditions! nitrogen Passed Observable Testing ✅Regression test oxidation:Reference: Execution time (DD:HH:MM:SS): 00:00:01:46 oxidation Passed Core Comparison ✅Original model has 59 species. oxidation Passed Edge Comparison ✅Original model has 230 species. DetailsObservables Test Case: Oxidation Comparison✅ All Observables varied by less than 0.500 on average between old model and new model in all conditions! oxidation Passed Observable Testing ✅Errors occurred during observable testing
WARNING:root:Initial mole fractions do not sum to one; normalizing.
|
Motivation or Problem
(reviving abandoned PR #2606)
Sometimes users know they are not interested in ring growth in their model, yet RMG spends significant time on suggesting species with complex fused rings. Using the maximum heavy atoms constraints isn't always helpful since other species with the same Mw could be relevant for the model.
Description of Changes
Added a
maximumFusedRingskey to the constraints dict.A new method was implemented in Molecule along with a test.
Testing
A unit test was added.
FWIW, the feature was also tested locally by generating a model that previously gave many fused ring structures and verifying that they are absent now.
Reviewer Tips
Note that a previously non-implemented constraint was removed from the docs