Getting structure information correctly for fragment #2570
Conversation
|
@hwpang change you change the base branch to the |
|
@JacksonBurns Just did! |
There was a problem hiding this comment.
This looks good! Should fix some of the bugs we were having. A few small comments, but nothing major.
One overall comment that we should think about - I think that we might want to 'flip' the logic of Fragment. Right now we store the cutting-label containing 'molecule' inside the same attribute that the parent class Molecule stores its actual molecule in, which is what is requiring us to override so many of the methods. Would it make more sense to store the representative molecule in the molecule attribute, and then store the sliced molecule in a new attribute? We would then just need to change the Fragment-specific methods (like sliceitup_etc) to operate on this new attribute, but I think that we could inherit all of the other methods from Molecule without any changes.
hwpang
force-pushed
the
afm_refactor_hwpang
branch
from
4fa1ce8
to
ab9a6db
Compare
|
I realize that there are unit tests that test the molecular weight of a fragment is calculated based on the fragment size. This makes me wonder whether using the fragment structure or the representative molecule structure is correct. I will revert my changes regarding the molecular weight as I am not sure how it might change the downstream application. For example, is the mass balance of a reaction checked somewhere? If that's checked, then using the fragment structure is best. Update: I discussed this with @lily90502 and she said Cantera checks the mass balances of reactions. Based on this, let's keep the |
hwpang
force-pushed
the
afm_refactor_hwpang
branch
from
ab9a6db
to
de6cbaa
Compare
I think this will also involve changing many codes used by Since the reacting part of code is much more core function of RMG and much more involved to change, I think it makes more sense to keep it as it is right now, as functions like calculating molecular weight are easy to work around. |
Codecov Report
@@ Coverage Diff @@
## afm_refactor #2570 +/- ##
================================================
- Coverage 55.44% 55.44% -0.01%
================================================
Files 124 124
Lines 36826 36818 -8
================================================
- Hits 20419 20414 -5
+ Misses 16407 16404 -3
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Regression Testing Results
Detailed regression test results.Regression test aromatics:Reference: Execution time (DD:HH:MM:SS): 00:00:01:09 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:13 liquid_oxidation Passed Core Comparison ✅Original model has 37 species. 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:25 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(Cyclopropene) + 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:33 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
The following observables did not match: ❌ Observable species O=S=O varied by more than 0.100 on average between old model SO2(15) and new model SO2(15) in condition 1.
sulfur Failed Observable Testing ❌Regression test superminimal:Reference: Execution time (DD:HH:MM:SS): 00:00:00:34 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:36 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:43 RMS_CSTR_liquid_oxidation Failed 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 ✅beep boop this comment was written by a bot 🤖 |
JacksonBurns
merged commit ba00728
into
ReactionMechanismGenerator:afm_refactor
Motivation or Problem
This PR contains changes made to #2413. See this comment for details. I put it into a separate PR rather than push commits directly PR as suggested by @JacksonBurns.
Description of Changes
I use a representative molecule for a fragment to calculate the correct molecular weight, number of atoms, and number of bonds. The number of atoms for the representative molecule is used to estimate radius for a fragment, which is used to estimate the stokes diffusivity. I also pass the kLA and kH into the RMS Species for fragments.