Skip to content

Forbid a charged carbene in singlet_carbene_intra_disproportionation#214

Merged
mliu49 merged 1 commit intomasterfrom
chargedCarbene
Oct 13, 2017
Merged

Forbid a charged carbene in singlet_carbene_intra_disproportionation#214
mliu49 merged 1 commit intomasterfrom
chargedCarbene

Conversation

@alongd
Copy link
Member

@alongd alongd commented Sep 11, 2017 •
edited
Loading

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

mention-bot commented Sep 11, 2017

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

@alongd alongd requested a review from zjburas September 11, 2017 16:32
@alongd
Added a zero charge to carbenes in singlet_carbene_intra_disproportio…
056ca9c 
…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?

zjburas
zjburas approved these changes Oct 13, 2017
View reviewed changes
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 !

@alongd alongd mentioned this pull request Nov 3, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zjburas zjburas zjburas approved these changes

Assignees

No one assigned

Labels

Complexity: Low Type: Error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@alongd @mention-bot @zjburas @mliu49