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

Parse CCSD(T) energies (no F12) in Molpro #1592

Merged
merged 6 commits into from
May 6, 2019
Merged

Parse CCSD(T) energies (no F12) in Molpro #1592

merged 6 commits into from
May 6, 2019

Conversation

alongd
Copy link
Member

@alongd alongd commented May 2, 2019

I found out that some jobs don't have the Electronic Energy at 0 [K]: -768.275662 [H] line we're currently looking for when parsing energies from a non-F12 CCSD calculation in Molpro. I made a test out of this example, and modified the parser accordingly.

@alongd alongd added the Arkane label May 2, 2019
@alongd alongd requested a review from cgrambow May 2, 2019 18:17
@alongd alongd self-assigned this May 2, 2019
@codecov
Copy link

codecov bot commented May 2, 2019

Codecov Report

Merging #1592 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1592      +/-   ##
==========================================
- Coverage    41.6%   41.58%   -0.02%     
==========================================
  Files         176      176              
  Lines       29147    29147              
  Branches     5995     5995              
==========================================
- Hits        12127    12122       -5     
- Misses      16183    16187       +4     
- Partials      837      838       +1
Impacted Files Coverage Δ
rmgpy/data/kinetics/family.py 52.74% <0%> (-0.24%) ⬇️

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 8c2f86a...20ff313. Read the comment docs.

Copy link

@cgrambow cgrambow left a comment

Choose a reason for hiding this comment

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

Making the PEP-8 changes as well is fine, but could you also make a separate commit for PEP-8 changes in molpro.py so that your actual code change in your first commit is clear?

# Determine whether the sp method is f12,
# if so whether we should parse f12a or f12b according to the basis set.
# Otherwise, check whether the sp method is MRCI.
f12, f12a, f12b, mrci = False, False, False, False
Copy link

Choose a reason for hiding this comment

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

Why do you need the f12 variable? Could you just check that both f12a and f12b are false?

Copy link
Member Author

Choose a reason for hiding this comment

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

We determine f12a and f12b from the basis set, so one of them could be True, but the method won't necessarily be F12

Copy link

Choose a reason for hiding this comment

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

I see, but we could also just set f12a to be true if we encounter both f12 and vtz/vdz in the file and likewise for f12b.

Copy link
Member Author

Choose a reason for hiding this comment

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

They don't have to be on the same line if one uses an F12 method but not an F12 basis set (for which we have model chemistries)

logging.debug('Found MRCI+Davidson energy in molpro log file {0}, using this value'.format(
self.path))
break
if E0 is None and mrci:
elif not f12:
if 'Electronic Energy at 0' in line:
Copy link

Choose a reason for hiding this comment

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

Just curious, do you know when (if ever) this gets used?

Copy link
Member Author

@alongd alongd May 2, 2019

Choose a reason for hiding this comment

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

Not really. This is actually the original line we were parsing so far for non-F12 sp jobs. The only thing "different" about the test I added is that it is QZ (basis=aug-cc-pvqz). Maybe that changes something in the output format.

log=MolproLog(os.path.join(os.path.dirname(__file__),'data','ethylene_f12_qz.out'))
E0=log.loadEnergy()
log = MolproLog(os.path.join(os.path.dirname(__file__), 'data', 'ethylene_f12_qz.out'))
eo = log.loadEnergy()
Copy link

Choose a reason for hiding this comment

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

e0


self.assertAlmostEqual(E0 / constants.Na / constants.E_h, -78.472682755635, 5)
self.assertAlmostEqual(eo / constants.Na / constants.E_h, -78.472682755635, 5)
Copy link

Choose a reason for hiding this comment

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

e0

Copy link
Member Author

@alongd alongd left a comment

Choose a reason for hiding this comment

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

Thanks! force pushing fixes

# Determine whether the sp method is f12,
# if so whether we should parse f12a or f12b according to the basis set.
# Otherwise, check whether the sp method is MRCI.
f12, f12a, f12b, mrci = False, False, False, False
Copy link
Member Author

Choose a reason for hiding this comment

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

We determine f12a and f12b from the basis set, so one of them could be True, but the method won't necessarily be F12

logging.debug('Found MRCI+Davidson energy in molpro log file {0}, using this value'.format(
self.path))
break
if E0 is None and mrci:
elif not f12:
if 'Electronic Energy at 0' in line:
Copy link
Member Author

@alongd alongd May 2, 2019

Choose a reason for hiding this comment

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

Not really. This is actually the original line we were parsing so far for non-F12 sp jobs. The only thing "different" about the test I added is that it is QZ (basis=aug-cc-pvqz). Maybe that changes something in the output format.

arkane/molpro.py Outdated
@@ -304,7 +305,7 @@ def loadEnergy(self, frequencyScaleFactor=1.):
for line in lines:
if f12a:
if 'CCSD(T)-F12a' in line and 'energy' in line:
E0 = float(line.split()[-1])
e0 = float(line.split()[-1])
break
if 'Electronic Energy at 0' in line:
E0 = float(line.split()[-2])
Copy link

Choose a reason for hiding this comment

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

You forgot to change some of the E0s.

def __init__(self, path):
self.path = path
super(MolproLog, self).__init__(path)
Copy link

Choose a reason for hiding this comment

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

This isn't a PEP-8-related change. Add to commit where you do the same for Gaussian and Q-Chem.

@@ -391,3 +392,9 @@ def loadNegativeFrequency(self):
raise Exception('Unable to find imaginary frequency in Molpro output file {0}'.format(self.path))
negativefrequency = -float(frequency)
return negativefrequency

def loadScanEnergies(self):
Copy link

Choose a reason for hiding this comment

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

Also not a PEP-8-related change.

# Determine whether the sp method is f12,
# if so whether we should parse f12a or f12b according to the basis set.
# Otherwise, check whether the sp method is MRCI.
f12, f12a, f12b, mrci = False, False, False, False
Copy link

Choose a reason for hiding this comment

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

I see, but we could also just set f12a to be true if we encounter both f12 and vtz/vdz in the file and likewise for f12b.

arkane/molpro.py Outdated
for line in lines:
if 'basis' in line.lower():
if 'vtz' in line.lower() or 'vdz' in line.lower():
f12a = True # MRCI could also have a vdz/vtz basis, so don't break yet
elif any(high_basis in line.lower() for high_basis in ['vqz', 'v5z', 'v6z', 'v7z', 'v8z']):
f12b = True # MRCI could also have a v(4+)z basis, so don't break yet
elif 'ccsd' in line and 'f12' in line:
Copy link

Choose a reason for hiding this comment

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

Use line.lower().

@alongd
Copy link
Member Author

alongd commented May 6, 2019

I didn't use fixup commits since I had to re-commit stuff to better organize the PR. I think I addressed everything (though I left the f12 flag, let me know if you think of a better way), and force pushed.

@cgrambow cgrambow merged commit fe57caa into master May 6, 2019
@cgrambow cgrambow deleted the no_f12 branch May 6, 2019 15:35
@mliu49 mliu49 mentioned this pull request May 15, 2019
5 tasks
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.

2 participants