-
Notifications
You must be signed in to change notification settings - Fork 230
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
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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/molproTest.py
Outdated
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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e0
arkane/molproTest.py
Outdated
|
||
self.assertAlmostEqual(E0 / constants.Na / constants.E_h, -78.472682755635, 5) | ||
self.assertAlmostEqual(eo / constants.Na / constants.E_h, -78.472682755635, 5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e0
There was a problem hiding this 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 |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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]) |
There was a problem hiding this comment.
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 E0
s.
def __init__(self, path): | ||
self.path = path | ||
super(MolproLog, self).__init__(path) |
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use line.lower()
.
Also added loadScanEnergies to MolproLog for consistent inheritance
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. |
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.