Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Atom label swapping investigation #1182

Closed
3 tasks done
mliu49 opened this issue Aug 28, 2017 · 1 comment
Closed
3 tasks done

Atom label swapping investigation #1182

mliu49 opened this issue Aug 28, 2017 · 1 comment

Comments

@mliu49
Copy link
Contributor

mliu49 commented Aug 28, 2017

I'm creating an issue for documentation and discussion. Basically, I mentioned in #1142 that it seems that the atom label swapping we do when generating reactions in families that are their own reverse does not seem to serve any purpose. At that time I thought that I had tested eg1 with and without the atom relabeling with no difference in the results. However, in #1168, @amarkpayne discovered that removing the relabeling caused an error in eg1.

After investigating, it seems like the atom label swapping does have an effect, but it isn't particularly intuitive. In __generateProductStructures, after applyRecipe has been called, we check the product structures against the forbidden groups. Relabeling the product structures means that the forbidden groups only need to be labeled to match the reactant labeling.

In the current case, removing atom label swapping leads to an error in eg1 with finding the reverse reaction for CC + [CH2]C=C[CH2] <=> C[CH2] + [CH2]C=CC in H_Abstraction. This is because [CH2]C=CC is forbidden as a reactant. With atom label swapping, the reaction would not have been generated because RMG would realize that the product is forbidden. Without atom label swapping, the reaction is initially created, but runs into problems with finding the reverse reaction.

Interestingly, this seems to be related to #412. In that issue, reverse reactions could not be found because the product species would be forbidden when used as a reactant. In the original post, I cited issues I was having with intra_H_migration (atoms were being swapped incorrectly) and intra_substitutionS_isomerization (atom swapping was not implemented). However, I made a later comment about an H_abstraction example, which should actually be caught by the label swapping.

Connie made a hack fix for the original issue by checking whether or not the failure to find a reverse reaction was due to forbidden groups. If so, then the forward reaction would be discarded. However, that fix is now broken due to the changes to reaction generation and multiple transition states. Otherwise, the eg1 error would have been caught.

Action items:

  • Reopen Aromatics updates #1142
  • Add atom label swapping for all families that are their own reverse
  • Update Connie's fix for catching forbidden reverse reactions
@mliu49
Copy link
Contributor Author

mliu49 commented Oct 27, 2017

Fixed by #1142 and #1203.

@mliu49 mliu49 closed this as completed Oct 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant