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

Gaussian energies #1815

Merged
merged 4 commits into from
Nov 13, 2019
Merged

Gaussian energies #1815

merged 4 commits into from
Nov 13, 2019

Conversation

dranasinghe
Copy link
Contributor

Motivation or Problem

Arkane parser for Gaussian does not parse MP2, double hybrid DFT, CCSD, CCSD(T) energies. Previous to this change, HF energy was appended as e0 or energies should be given manually in the Arkane input file.

Description of Changes

CBS-QB3 calculation contains CCSD(T), MP2, and CCSD energies, therefore, new if statements were added before CBS-QB3. The elif statements were arranged in the order of appearance in the output file

Testing

unit test added

Reviewer Tips

run a Gaussian calculation and check e0

goldmanm and others added 3 commits November 7, 2019 14:04
Previously the gaussian log file would read the SCF energy
if coupled cluster or similar methods were being used. This would
increase the error in calculation.

This commit allows for the correct energy to be obtained from MP2,
CCSD, and CCSD(T) methods. It also adds a debug message which
specifies what type of energy is being taken from the log file.
@alongd alongd requested review from alongd and goldmanm November 8, 2019 01:13
@alongd alongd assigned dranasinghe and unassigned goldmanm and alongd Nov 8, 2019
@alongd alongd added the Arkane label Nov 8, 2019
@goldmanm
Copy link
Contributor

goldmanm commented Nov 8, 2019

@dranasinghe, I would merge the first two commits, since they relate to the same effect.

The tests are showing an error when reading examples/arkane/species/C2H5/ethyl_cbsqb3.log

I don't know the CBS always appears in E2(CBS)= . If it does you could include the entire string. If it does not you can check for both ' E2(' and ')= ' (with the spaces) to ensure it doesn't pick up the other dataline.

@codecov
Copy link

codecov bot commented Nov 9, 2019

Codecov Report

Merging #1815 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1815   +/-   ##
=======================================
  Coverage   42.99%   42.99%           
=======================================
  Files          80       80           
  Lines       21090    21090           
  Branches     5513     5513           
=======================================
  Hits         9067     9067           
+ Misses      11008    10995   -13     
- Partials     1015     1028   +13
Impacted Files Coverage Δ
arkane/kinetics.py 12.14% <0%> (ø) ⬆️
rmgpy/data/statmech.py 42.2% <0%> (ø) ⬆️
rmgpy/rmg/pdep.py 12.21% <0%> (ø) ⬆️
rmgpy/data/kinetics/database.py 50.61% <0%> (ø) ⬆️
rmgpy/data/kinetics/family.py 48.35% <0%> (ø) ⬆️
rmgpy/statmech/ndTorsions.py 59.78% <0%> (ø) ⬆️
rmgpy/yml.py 15.71% <0%> (ø) ⬆️
arkane/sensitivity.py 10% <0%> (ø) ⬆️
rmgpy/rmg/input.py 34% <0%> (ø) ⬆️

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 a38eaf3...e914aa4. Read the comment docs.

@@ -274,7 +274,7 @@ def load_energy(self, zpe_scale_factor=1.):
if 'SCF Done:' in line:
e_elect = float(line.split()[4]) * constants.E_h * constants.Na
elect_energy_source = 'SCF'
elif ' E2(' in line:
elif ' E2(' in line and ' E(' in line:
Copy link
Contributor

Choose a reason for hiding this comment

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

I am probably missing something. What line in ethyl_cbsqb3.log should this be parsing?

When I type grep ' E2(' ethyl_cbsqb3.log | grep ' E(', I don't find any matching lines.

The unit tests are passing, though I don't understand why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that how it should be. In my previous commit, I only put " E2(" and it found a match at the end of the summary gaussian print. That's why the unit test failed. " E2(" and "E(" does not appear together in the cbs-qb3 calculation and it will correctly parse the " cbs-qb3 energy" , but it will appear in the double-hybrid calculations.

@dranasinghe
Copy link
Contributor Author

Dear @goldmanm Mark, i made the changes you suggested. Further, i added the file previously gave the error as a test.

@goldmanm goldmanm merged commit f561368 into master Nov 13, 2019
@mliu49 mliu49 mentioned this pull request Dec 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants