Check for adsorbates in resonance structure generation #2717
Conversation
bjkreitz
changed the title
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:06 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:11 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:27 nitrogen Passed Core Comparison ✅Original model has 41 species. nitrogen Passed Edge Comparison ✅Original model has 133 species.
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:55 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:36 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:26 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:02 RMS_CSTR_liquid_oxidation Failed 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: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:08 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 ✅Regression test minimal_surface:Reference: Execution time (DD:HH:MM:SS): 00:00:00:45 minimal_surface Passed Core Comparison ✅Original model has 11 species. minimal_surface Passed Edge Comparison ✅Original model has 38 species.
Observables Test Case: minimal_surface Comparison
✅ All Observables varied by less than 0.500 on average between old model and new model in all conditions! minimal_surface Passed Observable Testing ✅beep boop this comment was written by a bot 🤖 |
|
Is this preferable to what Matt noted in #2716 (comment) ? namely that
Or indeed should we do both? |
|
Matts's solution wouldn't prevent RMG from finding the charged species in the first place, but only that it reacts further. I think that we want both solutions. This PR addresses the general problem of why it found the charged species. RMG uses the resonance templates that were designed for gas-phase species and applies them to adsorbates. With our current understanding of nitrogen chemistry, I don't think that we should apply the gas-phase resonance templates, as this creates a whole range of problems we need to fix. It also saves computational time (not sure how much) by preventing the unintended execution of the resonance generation code for adsorbates. |
|
I am creating a set of reaction recipes for the nitrogen chemistry project that handle charge separation. They either create charge separation or remove it where necessary. An example would be *NO2 dissociation to *N and *NO. My thought is that we should keep the charge separation to be limited to being across just one bond and not on opposite sides of an adsorbate. For systems where it is expected that no charge separated species are created, Matts solution works just fine, but there are issues when using my new recipes with resonance. Turning off resonance seems to fix these issues. I think in some cases we may want charge for the surface families to be "cx" like in the case of the association reaction: If we set charge to 0 for the R group in the surface dissociation reaction family, this reaction would not be found. I would bet in many cases the recipes would just not work for charged R groups because the recipes do not specify the movement of lone pairs, so in many cases setting the charge to 0 would be redundant, but in some cases we may want to have charge be "cx" |
|
I might be misunderstanding the broader issue, but I don't think the issues discussed here don't exist in gas phase theoretically. I think these were solved in gas phase by modifying existing templates, adding ORs, and creating new families. Which would probably be more ideal. It should also be possible to tweak the reasonance functions to make them not applicable to certain structures involving surface sites as needed. Although, I don't think that is as trivial as modifying the families. Far away and resonance-wise isolated from the site I think we would expect adsorbates to have the same resonance behavior as gas-phase species so it seems like ignoring them rather than tweaking them might not be ideal. |
|
This pull request adjusts the resonance code so that it is not applied to surface species, and it is actually rather trivial. Certainly, this is much easier than modifying all the families. I don't see the point in generating all possible resonance structures for adsorbates that contain charge separation based on the gas-phase template if we don't apply the reaction templates to convert them. We should also modify the reaction templates so as not to do anything unintended for charged species. However, fixing the resonance code already takes a huge step towards preventing unintended species with charge separation. I also agree that the resonance behavior for large adsorbates is probably similar to that of gas-phase species. However, there is currently no proof that this is actually the case. We can use RMG reliably only for small adsorbates at the moment (around 3 heavy atoms). For these structures, I don't think that the resonance behavior is similar to that of gas-phase species, and I would prevent it. We might have to revisit this idea when we develop mechanisms for very large N-containing molecules, though. |
|
I don't have a grasp of the exact issues you're running into so really all I'm trying to say is that this looks a lot like splitting a system/ or turning off a system we're going to eventually want to merge back together/turn on. Historically, we do this all the time because sometimes adding a new feature/system is too much work to integrate all at once. However, we almost consistently end up paying for it after people have left and/or have less incentives to do the work. Sometimes we look back and it was unavoidable, other times we feel differently. If you've determined this is the most sensible way to get adsorbate resonance working this is fine with me, but I would be surprised if we don't eventually want to merge the resonance systems together, but how far away we are from that I don't know. |
|
Could you please explain the original issue in #2716? |
| @@ -130,6 +130,7 @@ def analyze_molecule(mol, save_order=False): | |||
| 'is_aryl_radical': False, | |||
| 'hasNitrogenVal5': False, | |||
| 'hasLonePairs': False, | |||
| 'is_adsorbate':mol.contains_surface_site(), | |||
There was a problem hiding this comment.
minor: please add a space after :
|
I am trying to generate a set of reaction families to handle reactions where charge separation is created or removed. For the Reaction:
I made a generic template to handle this:
With the Recipe: recipe(actions=[ And the top level node: entry( entry( But now RMG will find a species like:
Then apply resonance to get:
It will then say that this structure fits the description of my family, try to apply the recipe, and then get an error because my recipe only works currently when the charge separation is on adjacent atoms. I have tried setting the charge in my template to c+1 and c-1 instead of cx respectively, but then the recipe is only found in the forward direction. |
|
HI, sorry for being slow on this: |
Motivation or Problem
@kirkbadger18 observed an issue when generating mechanisms with adsorbates that contain N atoms. RMG seems to generate species that have formal charges, although the reaction templates technically don't allow this reaction. This is described in issue #2716. The reason RMG found this issue is because of the resonance structure generation. Currently, there is no check if a species is an adsorbate or a gas-phase species, when applying the resonance algorithm. In this case, it produced a species with charge separation and then produced a whole bunch of weird reactions from that before crashing.
In my opinion, we should not apply the resonance structure generation templates for gas-phase species to adsorbates because we don't really know if they exist (yet). But I'm open to discussion if something thinks that the gas-phase recipes should be applied to adsorbates.
I have another PR open #2511 that adds resonance generation specifically for adsorbates.
Description of Changes
I added a check in
resonance.pyto only apply the resonance structure generation if a species is an adsorbate.Testing
I confirmed that the issue is resolved. RMG passes the tests locally and I can also generate mechanisms without any errors.
Reviewer Tips
This should be fairly quick to review. It shouldn't affect the gas-phase chemistry and doesn't have any implications for catalysis mechanisms. It would be great, if someone could test this for a gas-phase reaction mechanism that involves N where resonance is important and confirm that this didn't change anything.
@alongd It would be great if you could take a look at this .