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

Forbid a charged carbene in singlet_carbene_intra_disproportionation #214

Merged
merged 1 commit into from
Oct 13, 2017

Conversation

alongd
Copy link
Member

@alongd alongd commented Sep 11, 2017

This solves #213 by adding the simple forbidden group

1 *1 C u0 p1 c-1

to carbene in singlet_carbene_intra_disproportionation.

@zjburas and I were thinking what would be the best approach (adding c0 to all node? leaving the forbidden group?) @nyee would you have any suggestions?

@mention-bot
Copy link

@alongd, thanks for your PR! By analyzing the history of the files in this pull request, we identified @zjburas to be a potential reviewer.

…nation

Forbidding a charged carbene such as in C[C-]=[N+]=O from reacting in
the singlet_carbene_intra_disproportionation family. See #213.
@alongd
Copy link
Member Author

alongd commented Sep 28, 2017

@nyee, thanks for your comment in #213.
I changed this PR to add c0 to all carbenes instead of forbidding c-1.
Ready for review (once Travis is fixed, ReactionMechanismGenerator/RMG-Py#1208)

@alongd
Copy link
Member Author

alongd commented Oct 13, 2017

@zjburas , I re-ran Travis, and this PR is now clean. Could you take a look?

Copy link
Contributor

@zjburas zjburas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, every *1 atom has a c0 attribute now and you removed the forbidden group. My only question is if this change (without the forbidden group) still fixes your original problem?

@alongd
Copy link
Member Author

alongd commented Oct 13, 2017

Yes, it does the trick and solves #213 as well.

@zjburas
Copy link
Contributor

zjburas commented Oct 13, 2017

Great, then I think someone with the authority can merge this.

@alongd
Copy link
Member Author

alongd commented Oct 13, 2017

@nyee, I still can't merge PRs on the database repo... strange.
Could you or anyone else with merging privileges please merge this?

@mliu49
Copy link
Contributor

mliu49 commented Oct 13, 2017

@alongd, I modified the permissions. You should be able to merge now.

@mliu49 mliu49 merged commit 462b48c into master Oct 13, 2017
@mliu49 mliu49 deleted the chargedCarbene branch October 13, 2017 21:03
@alongd
Copy link
Member Author

alongd commented Oct 13, 2017

Thanks @mliu49 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants