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

Tree Generation for Cyclic Ether Formation #487

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

xiaoruiDong
Copy link
Contributor

Generated tree for Cyclic Ether Formation family. This is generated following the guidance of RMG developer hour (Jun 7, 2021).

Note:

  • Previously, this family only supports reactions for 3, 4, 5, 6 member ring products. But, it should have covered reactions for larger ring products as well. The current problem is there is no data for those larger rings and can cause overestimations for them.

productNum = 2

autoGenerated = False
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised at this line 🤔 . Is it a bug, or does in not mean what I think?

Copy link
Contributor

@hwpang hwpang Jun 7, 2021

Choose a reason for hiding this comment

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

I think currently autoGenerated is defaulted to False and needs to be changed to True manually. @mjohnson541 it might be nice to default this to True?

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be set to True here, I forgot to say that in developer hours. Having it be default True would be good once we've converted the majority of the trees, but right now we'd have to go through and manually set it to False for most of the families so I think for the moment it's easier to just add it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or actually much better we should automatically set it when we generate the tree so that when it saves it saves as True!

Copy link
Contributor

Choose a reason for hiding this comment

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

added this to the assorted PR in RMG-Py

@xiaoruiDong xiaoruiDong self-assigned this Jun 8, 2021
@xiaoruiDong xiaoruiDong added this to In progress in Decision Tree Estimator via automation Jun 8, 2021
@xiaoruiDong xiaoruiDong changed the title Automatically Generated Tree for Cyclic formation Tree Generation for Cyclic Ether Formation Jun 14, 2021
Copy link
Contributor

@mjohnson541 mjohnson541 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 to me! Can you just squash the autoGenerated commit?

Replace the logic node to a more generalized node; Removed all nodes previously defined; Add number of reactants and products.
This will be replaced by new rules created by ATG anyway
The new rate rules are generated by the BM tree fitting notebook.
@xiaoruiDong xiaoruiDong changed the base branch from master to main October 4, 2021 03:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Decision Tree Estimator
  
In progress
Development

Successfully merging this pull request may close these issues.

None yet

4 participants