Improve isodesmic and add element balance constraints #2596
Conversation
hwpang
force-pushed
the
isodesmic_2024
branch
from
42ab939 to
808b3f2
Compare
Regression Testing Results
Detailed regression test results.Regression test aromatics:Reference: Execution time (DD:HH:MM:SS): 00:00:01:07 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)(Cds-Cds)(Cds-Cds)H) + group(Cds-Cds(Cds-Cds)(Cds-Cds)) + group(Cds-CdsCsH) + group(Cds-CdsCsH) + group(Cds-Cds(Cds-Cds)H) + group(Cds-Cds(Cds-Cds)H) + group(Cds-CdsCsH) + group(Cdd-CdsCds) + Estimated bicyclic component: polycyclic(s4_6_6_ane) - ring(Cyclohexane) - ring(Cyclohexane) + ring(124cyclohexatriene) + ring(124cyclohexatriene) Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics:
Observables 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:13 liquid_oxidation Failed Core Comparison ❌Original model has 37 species. liquid_oxidation Failed Edge Comparison ❌Original model has 202 species.
Observables 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:24 nitrogen Failed Core Comparison ❌Original model has 41 species. nitrogen Failed Edge Comparison ❌Original model has 132 species. Non-identical thermo! ❌
thermo: Thermo group additivity estimation: group(O2s-CdN3d) + group(N3d-OCd) + group(Cd-HN3dO) + ring(oxirene) + radical(CdJ-NdO) Non-identical kinetics! ❌
kinetics:
Observables 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:02:37 oxidation Passed Core Comparison ✅Original model has 59 species. oxidation Passed Edge Comparison ✅Original model has 230 species.
Observables 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 ✅Regression test sulfur:Reference: Execution time (DD:HH:MM:SS): 00:00:00:56 sulfur Passed Core Comparison ✅Original model has 27 species. sulfur Failed Edge Comparison ❌Original model has 89 species.
Observables Test Case: SO2 Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! sulfur Passed Observable Testing ✅Regression test superminimal:Reference: Execution time (DD:HH:MM:SS): 00:00:00:35 superminimal Passed Core Comparison ✅Original model has 13 species. superminimal Passed Edge Comparison ✅Original model has 18 species. Regression test RMS_constantVIdealGasReactor_superminimal:Reference: Execution time (DD:HH:MM:SS): 00:00:02:29 RMS_constantVIdealGasReactor_superminimal Passed Core Comparison ✅Original model has 13 species. RMS_constantVIdealGasReactor_superminimal Passed Edge Comparison ✅Original model has 13 species.
Observables Test Case: RMS_constantVIdealGasReactor_superminimal Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! RMS_constantVIdealGasReactor_superminimal Passed Observable Testing ✅Regression test RMS_CSTR_liquid_oxidation:Reference: Execution time (DD:HH:MM:SS): 00:00:06:15 RMS_CSTR_liquid_oxidation Failed Core Comparison ❌Original model has 36 species. RMS_CSTR_liquid_oxidation Failed Edge Comparison ❌Original model has 199 species. Non-identical kinetics! ❌
kinetics:
Observables Test Case: RMS_CSTR_liquid_oxidation Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! RMS_CSTR_liquid_oxidation Passed Observable Testing ✅Regression test fragment:Reference: Execution time (DD:HH:MM:SS): 00:00:00:41 fragment Passed Core Comparison ✅Original model has 10 species. fragment Passed Edge Comparison ✅Original model has 33 species.
Observables Test Case: fragment Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! fragment Passed Observable Testing ✅Regression test RMS_constantVIdealGasReactor_fragment:Reference: Execution time (DD:HH:MM:SS): 00:00:03:12 RMS_constantVIdealGasReactor_fragment Passed Core Comparison ✅Original model has 10 species. RMS_constantVIdealGasReactor_fragment Passed Edge Comparison ✅Original model has 27 species.
Observables Test Case: RMS_constantVIdealGasReactor_fragment Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! RMS_constantVIdealGasReactor_fragment Passed Observable Testing ✅beep boop this comment was written by a bot 🤖 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2596 +/- ##
==========================================
+ Coverage 54.79% 55.46% +0.66%
==========================================
Files 125 124 -1
Lines 37156 37047 -109
==========================================
+ Hits 20361 20549 +188
+ Misses 16795 16498 -297 ☔ View full report in Codecov by Sentry. |
hwpang
force-pushed
the
isodesmic_2024
branch
from
808b3f2 to
a34e5f6
Compare
hwpang
force-pushed
the
isodesmic_2024
branch
from
d0ebc42 to
8482786
Compare
Regression Testing Results
Detailed regression test results.Regression test aromatics:Reference: Execution time (DD:HH:MM:SS): 00:00:01:05 aromatics Passed Core Comparison ✅Original model has 15 species. aromatics Failed Edge Comparison ❌Original model has 106 species. Non-identical thermo! ❌
Identical thermo comments: Non-identical thermo! ❌
thermo: Thermo group additivity estimation: group(Cs-(Cds-Cds)(Cds-Cds)(Cds-Cds)H) + group(Cds-Cds(Cds-Cds)(Cds-Cds)) + group(Cds-CdsCsH) + group(Cds-CdsCsH) + group(Cds-Cds(Cds-Cds)H) + group(Cds-Cds(Cds-Cds)H) + group(Cds-CdsCsH) + group(Cdd-CdsCds) + Estimated bicyclic component: polycyclic(s4_6_6_ane) - ring(Cyclohexane) - ring(Cyclohexane) + ring(124cyclohexatriene) + ring(1,4-Cyclohexadiene) Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics:
Observables 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:09 liquid_oxidation Failed Core Comparison ❌Original model has 37 species. Non-identical kinetics! ❌
kinetics: liquid_oxidation Failed Edge Comparison ❌Original model has 202 species. Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics:
Observables 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:23 nitrogen Passed Core Comparison ✅Original model has 41 species. nitrogen Failed Edge Comparison ❌Original model has 132 species. Non-identical thermo! ❌
thermo: Thermo group additivity estimation: group(O2s-CdN3d) + group(N3d-OCd) + group(Cd-HN3dO) + ring(oxirene) + radical(CdJ-NdO) Non-identical kinetics! ❌
kinetics:
Observables 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:02:24 oxidation Passed Core Comparison ✅Original model has 59 species. oxidation Passed Edge Comparison ✅Original model has 230 species.
Observables 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 ✅Regression test sulfur:Reference: Execution time (DD:HH:MM:SS): 00:00:00:53 sulfur Passed Core Comparison ✅Original model has 27 species. sulfur Failed Edge Comparison ❌Original model has 89 species.
Observables Test Case: SO2 Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! sulfur Passed Observable Testing ✅Regression test superminimal:Reference: Execution time (DD:HH:MM:SS): 00:00:00:33 superminimal Passed Core Comparison ✅Original model has 13 species. superminimal Passed Edge Comparison ✅Original model has 18 species. Regression test RMS_constantVIdealGasReactor_superminimal:Reference: Execution time (DD:HH:MM:SS): 00:00:02:28 RMS_constantVIdealGasReactor_superminimal Passed Core Comparison ✅Original model has 13 species. RMS_constantVIdealGasReactor_superminimal Passed Edge Comparison ✅Original model has 13 species.
Observables Test Case: RMS_constantVIdealGasReactor_superminimal Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! RMS_constantVIdealGasReactor_superminimal Passed Observable Testing ✅Regression test RMS_CSTR_liquid_oxidation:Reference: Execution time (DD:HH:MM:SS): 00:00:06:11 RMS_CSTR_liquid_oxidation Passed Core Comparison ✅Original model has 37 species. RMS_CSTR_liquid_oxidation Failed Edge Comparison ❌Original model has 206 species.
Observables Test Case: RMS_CSTR_liquid_oxidation Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! RMS_CSTR_liquid_oxidation Passed Observable Testing ✅Regression test fragment:Reference: Execution time (DD:HH:MM:SS): 00:00:00:40 fragment Passed Core Comparison ✅Original model has 10 species. fragment Passed Edge Comparison ✅Original model has 33 species.
Observables Test Case: fragment Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! fragment Passed Observable Testing ✅Regression test RMS_constantVIdealGasReactor_fragment:Reference: Execution time (DD:HH:MM:SS): 00:00:03:09 RMS_constantVIdealGasReactor_fragment Passed Core Comparison ✅Original model has 10 species. RMS_constantVIdealGasReactor_fragment Passed Edge Comparison ✅Original model has 27 species.
Observables Test Case: RMS_constantVIdealGasReactor_fragment Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! RMS_constantVIdealGasReactor_fragment Passed Observable Testing ✅beep boop this comment was written by a bot 🤖 |
xiaoruiDong
left a comment
xiaoruiDong
left a comment
There was a problem hiding this comment.
@hwpang Good work finalizing the isodesmic reaction functionality. I have a few comments, majorly focused on improving readability of this piece of codes.
I foresee there will be cases some users ask about relevant questions or a developer need to modify something here for whatever reason, but future developers may not have adequate background knowledge or experience.
|
|
||
| def __repr__(self): | ||
| symbols = ["", "-", "=", "#"] | ||
| return f"{self.atom1}{symbols[self.bond_order]}{self.atom2}" |
There was a problem hiding this comment.
Since no error is raised so I assumed we wouldn't encounter cases like bond_order==1.5 for aromatic molecules? All the bond orders are kekulized?
There was a problem hiding this comment.
We currently are not kekulizing input species for the user. I believe that isodesmic reaction method requires the molecule to be kekulized. @oscarwumit Please correct me if I'm wrong. We currently rely on the user to know how to use the isodesmic method correctly by inputting a kekulized structure.
Alternatively, we could add a pre-processing step in the ErrorCancelingSpecies class to force kekulizing the input molecule. What do you think?
There was a problem hiding this comment.
When you say "kekulizing input species" do you mean changing the benzene bonds to single and double bonds? e.g., Cn1cccc1Nc1ccccn1 to CN1C=CC=C1NC1=NC=CC=C1？
If so, I think you are correct. We should either raise an error or add a pre-processing (probably better) to make sure the correct structure is being used for IDR scheme. Thanks.
| return True | ||
| return False | ||
|
|
||
| if isinstance(other, GenericConstraint): |
There was a problem hiding this comment.
In what situation a bond constraint will be compared to a generic constraint?
There was a problem hiding this comment.
I don't think we compare a bond constraint to a generic constraint within our code, but if a user were to make such constraint, this makes sure to catch that case. @oscarwumit Can you double check on this?
Edit: Please ignore above. I realize that we do compare them at the following code block in _enumerate_constraints
# enumerate for each species
new_constraints_list = []
for i, spcs_constraints in enumerate(full_constraints_list):
new_spcs_constraints = [c for c in spcs_constraints if c != constraint]
matching_constraints = len(spcs_constraints) - len(new_spcs_constraints)
enumerated_constraints[i].append(matching_constraints)
new_constraints_list.append(new_spcs_constraints)
| atom1 = AtomConstraint(label=atom1.symbol) | ||
| atom2 = AtomConstraint(label=atom2.symbol) | ||
|
|
||
| return BondConstraint(atom1=atom1, atom2=atom2, bond_order=int(bond.order)) |
There was a problem hiding this comment.
I saw int() is applied to bond.order, is this due to aromatic bond?
There was a problem hiding this comment.
Thanks for the question. It is not for the aromatic bond. Single and double bonds are saved as float in Bond object,
mol.atoms[0].bonds
{<Atom 'C'>: <Bond "1.0">, <Atom 'C'>: <Bond "2.0">, <Atom 'H'>: <Bond "1.0">}
and float would cause this part of code to fail:
def __repr__(self):
symbols = ["", "-", "=", "#"]
return f"{symbols[self.bond_order]}{self.atom}"
| np.array([1, 3, 6, 3]), | ||
| ) | ||
|
|
||
| spcs_consts.all_reference_species = [self.propene] |
There was a problem hiding this comment.
This is to ensure that reference_constraints in the code below will only return one thing and so make the check more explicit
spcs_consts.all_reference_species = [self.propene]
propene_features = spcs_consts._get_all_constraints(self.propene)
_, reference_constraints = spcs_consts._enumerate_constraints([benzene_features, propene_features])
|
Another general question is, in this implementation, the features (constraints) of reference species will be repeatedly generated for each target molecule in a thermo calculation workflow, right? |
|
After the discussion with Hao-Wei today, we decided to remove the usage of This API is added in SciPy 1.9.0, which only supports Python >=3.8. No need to implement this right away in this PR, but worth to leave a note here for future reference. |
Chiming in to say that I strongly support switching this to scipy - if we merge #2553 first we could upgrade to the latest Python and possibly do this now? Thoughts @hwpang @xiaoruiDong ? |
|
Any updates? We should probably get the fix merged as the paper gets published in case someone wants to use the IDR feature. |
|
I haven't gotten around to fix this due to Chemprop v2.0 release. If we can't get this resolve before the paper is published, we can direct the user to use the original branch, rather than main. |
Maybe addition to pointing the user to the original branch, we can also say it will be included in RMG version > 3.2 |
Regression Testing ResultsWARNING:root:Initial mole fractions do not sum to one; normalizing. Detailed regression test results.Regression test aromatics:Reference: Execution time (DD:HH:MM:SS): 00:00:01:05 aromatics Passed Core Comparison ✅Original model has 15 species. aromatics Passed Edge Comparison ✅Original model has 106 species.
Observables 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:10 liquid_oxidation Failed Core Comparison ❌Original model has 37 species. Non-identical kinetics! ❌
kinetics: liquid_oxidation Failed Edge Comparison ❌Original model has 202 species. Non-identical kinetics! ❌
kinetics:
Observables 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:22 nitrogen Passed Core Comparison ✅Original model has 41 species. nitrogen Failed Edge Comparison ❌Original model has 132 species. Non-identical thermo! ❌
thermo: Thermo group additivity estimation: group(O2s-CdN3d) + group(N3d-OCd) + group(Cd-HN3dO) + ring(oxirene) + radical(CdJ-NdO) Non-identical kinetics! ❌
kinetics:
Observables 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:02:25 oxidation Passed Core Comparison ✅Original model has 59 species. oxidation Passed Edge Comparison ✅Original model has 230 species.
Observables 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 ✅Regression test sulfur:Reference: Execution time (DD:HH:MM:SS): 00:00:00:54 sulfur Passed Core Comparison ✅Original model has 27 species. sulfur Failed Edge Comparison ❌Original model has 89 species.
Observables Test Case: SO2 Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! sulfur Passed Observable Testing ✅Regression test superminimal:Reference: Execution time (DD:HH:MM:SS): 00:00:00:35 superminimal Passed Core Comparison ✅Original model has 13 species. superminimal Passed Edge Comparison ✅Original model has 18 species. Regression test RMS_constantVIdealGasReactor_superminimal:Reference: Execution time (DD:HH:MM:SS): 00:00:02:22 RMS_constantVIdealGasReactor_superminimal Passed Core Comparison ✅Original model has 13 species. RMS_constantVIdealGasReactor_superminimal Passed Edge Comparison ✅Original model has 13 species.
Observables Test Case: RMS_constantVIdealGasReactor_superminimal Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! RMS_constantVIdealGasReactor_superminimal Passed Observable Testing ✅Regression test RMS_CSTR_liquid_oxidation:Reference: Execution time (DD:HH:MM:SS): 00:00:05:56 RMS_CSTR_liquid_oxidation Passed Core Comparison ✅Original model has 37 species. RMS_CSTR_liquid_oxidation Failed Edge Comparison ❌Original model has 206 species. Non-identical kinetics! ❌
kinetics:
Observables Test Case: RMS_CSTR_liquid_oxidation Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! RMS_CSTR_liquid_oxidation Passed Observable Testing ✅Regression test fragment:Reference: Execution time (DD:HH:MM:SS): 00:00:00:40 fragment Passed Core Comparison ✅Original model has 10 species. fragment Passed Edge Comparison ✅Original model has 33 species.
Observables Test Case: fragment Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! fragment Passed Observable Testing ✅Regression test RMS_constantVIdealGasReactor_fragment:Reference: Execution time (DD:HH:MM:SS): 00:00:03:03 RMS_constantVIdealGasReactor_fragment Passed Core Comparison ✅Original model has 10 species. RMS_constantVIdealGasReactor_fragment Passed Edge Comparison ✅Original model has 27 species.
Observables Test Case: RMS_constantVIdealGasReactor_fragment Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! RMS_constantVIdealGasReactor_fragment Passed Observable Testing ✅beep boop this comment was written by a bot 🤖 |
…o isodesmic_2024
|
The scipy.optimize.milp function is only available with scipy >= 1.9, which is available only with python >= 3.8. This is not available with the current environment.yml. @JacksonBurns I think it would be easier to develop this after we are able to bump the python version to >= 3.8. What is the progress on this? |
@oscarwumit Thanks for checking in. I have addressed most of the comments given by @xiaoruiDong. However, I am not able to switch over to use @oscarwumit Let me know what do you think we should do. I have tried to hack my way in an attempt to obtain a rmg env with python >= 3.8 so that I can develop for this PR, but I had no success. |
|
Thanks for your efforts on this. Let's merge after Python upgrading. |
oscarwumit
left a comment
oscarwumit
left a comment
There was a problem hiding this comment.
I think you are correct about the kekulizing part. As you suggested, please either raise an error or add a pre-processing step.
|
|
||
| def __repr__(self): | ||
| symbols = ["", "-", "=", "#"] | ||
| return f"{self.atom1}{symbols[self.bond_order]}{self.atom2}" |
There was a problem hiding this comment.
When you say "kekulizing input species" do you mean changing the benzene bonds to single and double bonds? e.g., Cn1cccc1Nc1ccccn1 to CN1C=CC=C1NC1=NC=CC=C1？
If so, I think you are correct. We should either raise an error or add a pre-processing (probably better) to make sure the correct structure is being used for IDR scheme. Thanks.
|
Development moved to #2695 which adds support for the required version of Python needed for the required SciPy version, as well as removes LPsolve |
4 participants
Motivation or Problem
Add Mark's isodesmic efforts and add element balance constraints.
Description of Changes
This PR adds two parts:
(1) attempting to add @amarkpayne 's isodesmic efforts from this branch: https://github.com/ReactionMechanismGenerator/RMG-Py/tree/isodesmic_improvements. As this branch is 4 years old, it is very difficult to rebase or merge. I manually copied his efforts in the isodesmic.py over.
(2)
When @oscarwumit and I are working on the bond additivity correction paper, @am2145 found that the isodesmic reactions found by the current code generate reactions that are not mass balanced. I implemented a fix by adding element balance constraints.