Skip to content

Forbid a charged carbene in singlet_carbene_intra_disproportionation#214

Merged
mliu49 merged 1 commit into
masterfrom
chargedCarbene
Oct 13, 2017
Merged

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

Conversation

@alongd

@alongd alongd commented Sep 11, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

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

mention-bot commented Sep 11, 2017

Copy link
Copy Markdown

@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

alongd commented Sep 28, 2017

Copy link
Copy Markdown
Member Author

@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

alongd commented Oct 13, 2017

Copy link
Copy Markdown
Member Author

@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

@zjburas zjburas left a comment

Copy link
Copy Markdown
Contributor

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

alongd commented Oct 13, 2017

Copy link
Copy Markdown
Member Author

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

@zjburas

zjburas commented Oct 13, 2017

Copy link
Copy Markdown
Contributor

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

@alongd

alongd commented Oct 13, 2017

Copy link
Copy Markdown
Member Author

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

@mliu49

mliu49 commented Oct 13, 2017

Copy link
Copy Markdown
Contributor

@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

alongd commented Oct 13, 2017

Copy link
Copy Markdown
Member Author

Thanks @mliu49 !

@alongd alongd mentioned this pull request Nov 3, 2017
Closed
JacksonBurns pushed a commit that referenced this pull request Mar 23, 2026
@mjohnson541
Merge pull request #214 from ReactionMechanismGenerator/save_sol
92a8b09 
Add function to save the solution
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