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

Changes to Globally Forbidden Structures to account for N chemistry #120

Closed
wants to merge 3 commits into from

Conversation

alongd
Copy link
Member

@alongd alongd commented Aug 9, 2016

This simple PR should allow RMG to work with nitrogen species. Several groups were deleted from forbiddenStructures.py, see RMGPy issue #514 (ReactionMechanismGenerator/RMG-Py#514).

In addition, I added the Ethylamine kinetic library (prepared by Beat & Shamel) with additional important nitrogen reactions.

@mention-bot
Copy link

@alongd, thanks for your PR! By analyzing the annotation information on this pull request, we identified @nyee, @jwallen and @bbuesser to be potential reviewers

@nyee
Copy link
Contributor

nyee commented Aug 12, 2016

The Ethylamine library is fine, but I'm a little bit worried about letting lose the globally forbidden structures. As discussed, we should try to set up some regressive tests to make sure this doesn't completely destroy previous mechanisms we have made.

@alongd
Copy link
Member Author

alongd commented Aug 12, 2016

I agree, let me know if I can help with this.
Plus, wouldn't it be awesome to have a comment next to each globally forbidden group mentioning why it is forbidden and providing example\s? I added this request as a comment in forbiddenStructures.py

@alongd
Copy link
Member Author

alongd commented Feb 9, 2017

I'm closing this PR, the commits here will be part of the nitrogen branch PR

@alongd alongd closed this Feb 9, 2017
@alongd alongd deleted the N branch February 9, 2017 16:52
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

Successfully merging this pull request may close these issues.

3 participants