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

ATG R Addition MultipleBond #585

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

kspieks
Copy link
Contributor

@kspieks kspieks commented May 19, 2022

This PR simply refits the rate tree for R_Addition_MultipleBond. I had to change the logic node for YJ to 1 *3 R u[1,2,3,4] px (source) before running ATG.

The zip file below contains the notebook for running ATG. Since there are nearly 3000 training reactions for this family, the notebook takes ~3 hours to run on supercloud. However, 85% of these training reactions are from group additivity on CBS-QB3 calculations. On the bright side, the decision tree can basically reproduce the results from GAV since the uncertainty estimation from the newly trained rate tree is probably the lowest I've seen: median error of k_estimated / k_true was only 1.6 and the mean error was only 2 as shown at the bottom of the notebook. For reference, the median error of retroene was 2.5, ketoenol was 2.6, diels alder was 6.1. Although it's nice that the decision tree fits the GAV results well, these training reactions did not come from TST calculations so it's unclear how accurate the training data is. I think this topic is ultimately outside the scope of this PR but I wanted to document it for future discussion.

tree_fitting_notebook_supercloud.ipynb.zip

@kspieks kspieks requested a review from hwpang May 19, 2022 11:22
@kspieks kspieks self-assigned this May 19, 2022
@kspieks kspieks force-pushed the ATG_R_Addition_MultipleBond branch from aa2eeba to 2cbb37c Compare May 19, 2022 12:02
Copy link
Contributor

@hwpang hwpang left a comment

Choose a reason for hiding this comment

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

This seems fine to me. Could you rebase?

@kspieks kspieks force-pushed the ATG_R_Addition_MultipleBond branch from 2cbb37c to dc2b360 Compare November 8, 2022 22:49
@kspieks
Copy link
Contributor Author

kspieks commented Nov 8, 2022

Thanks for reviewing! Just rebased, so we'll let the tests run

@kspieks kspieks force-pushed the ATG_R_Addition_MultipleBond branch from dc2b360 to 2be3aea Compare March 2, 2023 19:52
@hwpang
Copy link
Contributor

hwpang commented Mar 2, 2023

Did you make any changes (e.g., add new reactions, etc.) to the reaction.py and dictionary.txt? If not I think the consensus is to not commit them, as it makes it difficult to keep track of things

@kspieks kspieks force-pushed the ATG_R_Addition_MultipleBond branch from efdd435 to f400205 Compare March 2, 2023 20:19
@kspieks
Copy link
Contributor Author

kspieks commented Mar 2, 2023

@hwpang no I just refit the rate tree so I believe the changes to reaction.py and dictionary.txt are just automatic formatting changes. I just force pushed a new branch that doesn't commit these files

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.

2 participants