Skip to content

Enable Automatic Isodesmic Reaction Generation for Arkane#1561

Merged
alongd merged 21 commits into
masterfrom
errorCancelingReactions
Apr 27, 2020
Merged

Enable Automatic Isodesmic Reaction Generation for Arkane#1561
alongd merged 21 commits into
masterfrom
errorCancelingReactions

Conversation

@amarkpayne

Copy link
Copy Markdown
Member

Based heavily on ideas and algorithms provided by

Buerger, P., Akroyd, J., Mosbach, S., & Kraft, M. (2018). A systematic method to estimate and validate enthalpies of
formation using error-cancelling balanced reactions. Combustion and Flame (Vol. 187).
https://doi.org/10.1016/j.combustflame.2017.08.013

Dobek, F. J., Ranasinghe, D. S., Throssell, K., & Petersson, G. A. (2013). Evaluation of the heats of formation of
corannulene and C60 by means of inexpensive theoretical procedures. Journal of Physical Chemistry A, 117(22), 4726–4730.
https://doi.org/10.1021/jp404158v

@codecov

codecov Bot commented Apr 3, 2019

Copy link
Copy Markdown

Codecov Report

Merging #1561 into master will increase coverage by 0.56%.
The diff coverage is 73.62%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1561      +/-   ##
==========================================
+ Coverage   44.49%   45.06%   +0.56%     
==========================================
  Files          84       86       +2     
  Lines       21776    22210     +434     
  Branches     5679     5787     +108     
==========================================
+ Hits         9689    10008     +319     
- Misses      11020    11096      +76     
- Partials     1067     1106      +39     
Impacted Files Coverage Δ
arkane/encorr/reference.py 68.29% <68.29%> (ø)
arkane/encorr/isodesmic.py 80.31% <80.31%> (ø)
arkane/encorr/data.py 100.00% <100.00%> (ø)
arkane/ess/qchem.py 72.72% <100.00%> (ø)
arkane/kinetics.py 12.14% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 707e9d3...97d6868. Read the comment docs.

@davidfarinajr

Copy link
Copy Markdown
Contributor

How did you generate the yaml files? When do you run the reference species through arkane to get the NASA polynomials? Do you have a script to make them?

@rwest

rwest commented Jun 4, 2019

Copy link
Copy Markdown
Member

We've been looking into the YAML thing a bit more.
Seems Arkane already has a way to save and load YAML representations of everything, like NASA polynomials, etc. eg

def save_yaml(self, path):

But the yaml looks different from that generated by the yaml Dumper in

def save_to_yaml(self, path=None):

Should we pick one horse to back? Re-use existing I/O routines?

@amarkpayne

Copy link
Copy Markdown
Member Author

I think it is probably best if the yaml files follow the already established Arkane yaml format as much as possible. I'll take a look at this and update this PR.

@amarkpayne amarkpayne added the After Py3 To be addressed after Python 3 transition label Jul 11, 2019
@amarkpayne amarkpayne removed the After Py3 To be addressed after Python 3 transition label Jul 23, 2019
@amarkpayne
amarkpayne force-pushed the errorCancelingReactions branch from 81c7cee to 8e35d46 Compare July 25, 2019 15:12
@amarkpayne amarkpayne removed the Status: Blocked This PR should not be merged label Jul 25, 2019

@davidfarinajr davidfarinajr 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 great Mark!
My comments are mostly about adding isinstance or assert statements where I think we could use them. For the bigger picture, on this or future PRs:

  • perhaps add model_chemistry to low_level_hf attribute for ErrorCancelingSpecies object and use the model_chemistry as a constraint to only select reference species calculated with the same model chemistry
  • incorporate uncertainty in hf determined from EBRs scheme. Instead of using the median of multiple EBRs to calculate hf, weigh each EBR by its uncertainty and put greater weight on less uncertain reactions (reactions with reference species with higher certainty in low or high level hf, maybe)
  • add a function to test the reference dataset. Delete a reference species from reference dataset and use the deleted species as a target species to calculate hf using isodesmic scheme. This could be a way to gauge the uncertainty for that species.

Comment thread arkane/isodesmic.py Outdated
Comment thread arkane/isodesmic.py Outdated
Comment thread arkane/isodesmic.py Outdated
Comment thread arkane/isodesmic.py Outdated
Comment thread arkane/isodesmic.py Outdated
Comment thread arkane/isodesmic.py Outdated
Comment thread arkane/isodesmic.py Outdated
Comment thread arkane/isodesmic.py Outdated
Comment thread arkane/isodesmic.py Outdated
Comment thread arkane/isodesmic.py
@amarkpayne
amarkpayne force-pushed the errorCancelingReactions branch 4 times, most recently from 3c85534 to bc317b0 Compare August 2, 2019 16:12

@amarkpayne amarkpayne left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@davidfarinajr I have addressed your comments, and pushed the last of the commits for this PR. I added some of the assert/type checking statements, but not all of them. I think they are are bigger deal in the reference species section where we want to enforce a standard. For the remaining cases at the very least I added type hints in the docstrings that IDEs like PyCharm can pick up on. Do you mind doing one last review now that everything is in?

After this I will need a Merger review. @alongd , @mliu49 , @mjohnson541 , or @goldmanm do any of you have time to take a look at this after David's review? I know we have a lot of PRs right now, so if not I understand.

Comment thread arkane/isodesmic.py Outdated
Comment thread arkane/isodesmic.py Outdated
Comment thread arkane/isodesmic.py Outdated
Comment thread arkane/isodesmic.py Outdated
Comment thread arkane/isodesmic.py Outdated
Comment thread arkane/isodesmic.py Outdated
Comment thread arkane/isodesmic.py
@amarkpayne
amarkpayne force-pushed the errorCancelingReactions branch from bc317b0 to aade36f Compare August 2, 2019 17:18
@amarkpayne
amarkpayne marked this pull request as ready for review August 2, 2019 17:47
@amarkpayne amarkpayne added the Status: Ready for Review PR is complete and ready to be reviewed label Aug 2, 2019
@amarkpayne
amarkpayne force-pushed the errorCancelingReactions branch 2 times, most recently from 45ec98b to 702bff6 Compare August 5, 2019 20:00
@amarkpayne
amarkpayne requested a review from mjohnson541 August 6, 2019 10:50

@davidfarinajr davidfarinajr 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.

It's looking good! A few comments...

  1. I like the new ReferenceDatabase class. The extract_mode_chemistry method is useful for getting a reference subset with desired model chemistry from the reference set. However, I think we need a naming convention for model chemistry and also a way to see all the model chemistries in your reference set from which you can select.
  2. Since ReferenceSpecies is very similar to ArkaneSpecies, I think it would make sense to add a method to initialize a ReferenceSpecies object from an ArkaneSpecies object.
  3. For isodesmic scheme, we need to get Species Constraints. For Species Constraints, we need a list of ErrorCancelingSpecies from a reference set. You can get this list from the extract_model_chemistry method from the ReferenceDatabase class. However, I think we should add a method to convert ReferenceSpecies to ErrorCancelingSpecies.

Comment thread arkane/reference.py Outdated
Comment thread arkane/isodesmic.py Outdated
Comment thread arkane/reference.py Outdated
@amarkpayne

Copy link
Copy Markdown
Member Author

@alongd I have removed the temporary commit from this PR. Let me know if there are any other comments or changes you would like me to make.

@alongd

alongd commented Apr 25, 2020

Copy link
Copy Markdown
Member

The code looks great, thanks!!
Could you please add Arkane thermo calc example/s?

@amarkpayne
amarkpayne force-pushed the errorCancelingReactions branch from 1ddfa85 to 30f124b Compare April 25, 2020 22:56
@amarkpayne

Copy link
Copy Markdown
Member Author

I had added an example before, but we were encountering memory issues. I created a new example, and tried to make the QM output files as small as possible (though I don't think they contribute much anyways). It ran fine on my local machine, so let's hope that Travis handles it okay.

Also add additional apin-orbit corrections.
@amarkpayne
amarkpayne force-pushed the errorCancelingReactions branch from 30f124b to 97d6868 Compare April 27, 2020 05:17

@alongd alongd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for this valuable addition to Arkane!! All looks good.

@alongd
alongd merged commit 6277770 into master Apr 27, 2020
@alongd
alongd deleted the errorCancelingReactions branch April 27, 2020 13:55
@amarkpayne

Copy link
Copy Markdown
Member Author

Thanks for all of the hard work you put in to reviewing and help me fine tune this PR @alongd, @davidfarinajr and @cgrambow !! I really appreciate it

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.

5 participants