Closed bjkreitz closed 1 week ago
WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. ⚠️ One or more regression tests failed. Please download the failed results and run the tests locally or check the log to see why.
beep boop this comment was written by a bot :robot:
Is this preferable to what Matt noted in https://github.com/ReactionMechanismGenerator/RMG-Py/issues/2716#issuecomment-2364502345 ? namely that
Surface_Monodentate_to_Bidentate template allows the participating atoms to have any charge, which makes this a reaction. If you require the participating atoms to be uncharged in the template the charge separated resonance structure will not react with that template.
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: NO2 + C --> *CNO2
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. Also, suppose the part of the molecule that exhibits resonance is far away from the surface. In that case, it does not participate in any reactions because bond breaking is typically limited to atoms close to the surface. Therefore, I think that adjusting the resonance code, as suggested in this PR, does not have any negative impacts on the catalysis projects.
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? If the adsorbed reactant has a valid resonance structure with a double bond between C and N (and formal charge separation), then why does it violate the reaction template?
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=[ ['BREAK_BOND', '1', 1, '2'], ['FORM_BOND', '2', 1, '4'], ['CHANGE_BOND', '2', 1, '4'], ['LOSE_PAIR','2','1'], ['GAIN_PAIR', '1', '1'], ])
And the top level node:
entry( index = 1, label = "Combined", group = """ 1 1 N u0 p0 cx {2,S} {3,[S,D]} {4,[S,D]} 2 2 R!H u0 px cx {1,S} 3 *3 Xo u0 p0 c0 {1,[S,D]} 4 R!H u0 px c0 {1,[S,D]} """, kinetics = None, )
entry( index = 2, label="VacantSite", group = """ 1 *4 Xv u0 p0 c0 """, kinetics = None, )
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: If the top level node only permits charge sep. on adjacent atoms, how come RMG applied the recipe to the structure in your last image? Maybe the issue we need to solve is wrong identification by RMG rather than forbidding resonance pathways?
WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. ⚠️ One or more regression tests failed. Please download the failed results and run the tests locally or check the log to see why.
beep boop this comment was written by a bot :robot:
I have come around and changed my mind on this PR. I think it is beneficial to apply the gas-phase resonance templates to explore all possible species for adsorbates containing nitrogen. We have seen that some adsorbates actually form stable bidentates, which can only be described by using Lewis structures with charge separation, as Kirk has shown above. Therefore, I think that the issue that Kirk described in #2716 should be fixed in a different way, probably by changing the reaction templates. Thanks for pushing back on this PR! I'll close it since the current resonance checks are sufficient.
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.py
to 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 .