added orca parser to arkane #1749
Conversation
alongd
left a comment
alongd
left a comment
There was a problem hiding this comment.
Thanks for adding Orca into Arkane!!
I added a bunch of comments, most of them are minor. Let me know what you think.
arkane/orca.py
Outdated
|
|
||
| class OrcaLog(Log): | ||
| """ | ||
| Represent an output file from QChem. The attribute `path` refers to the |
arkane/orca.py
Outdated
| def load_force_constant_matrix(self): | ||
| # Orca print the hessian to .hess file. you need to provide .hess instead of .log | ||
|
|
||
| raise LogError('Could not find a successfully load Orca hessian:Parser is not inpemented') |
There was a problem hiding this comment.
Could you replace this with a more specific NotImplementedError error?
Something like: NotImplementedError('The load_force_constant_matrix method is not implemented for Orca Logs')
arkane/orca.py
Outdated
| CAUTION: The rotational entropy is not quite correctly treated here | ||
| because it includes a symmetry number that is not yet correctly | ||
| implemented in ORCA. Orca does not provide Rotational temperatures. Only E(rot) and E(trans) energies | ||
| Run gaussian to get E0 |
There was a problem hiding this comment.
Could you change the last line to `Consider using another supported software (such as Gaussian or QChem) to calculate the frequencies and derive E0.
|
Thank you for the comments I have addressed all the issues. |
|
@dranasinghe, the tests did not pass, giving the following trace: Let's talk and see how to resolve it. |
Codecov Report
@@ Coverage Diff @@
## master #1749 +/- ##
==========================================
+ Coverage 42.74% 42.92% +0.18%
==========================================
Files 81 82 +1
Lines 21078 21182 +104
Branches 5491 5519 +28
==========================================
+ Hits 9010 9093 +83
+ Misses 11067 11061 -6
- Partials 1001 1028 +27
Continue to review full report at Codecov.
|
alongd
left a comment
alongd
left a comment
There was a problem hiding this comment.
I saw several previous comments that were not addressed, please take a look, add a short explanation if you think the current state of the code is better that the suggestion.
arkane/orca.py
Outdated
| atom, coord, number, mass = [], [], [], [] | ||
|
|
||
| with open(self.path) as f: | ||
| log = f.read().splitlines() |
There was a problem hiding this comment.
Did you agree with the above comment? I didn't see a respective fix
arkane/orca.py
Outdated
| for i in reversed(range(len(log))): | ||
| line = log[i] | ||
| if 'CARTESIAN COORDINATES (ANGSTROEM)' in line: | ||
| for line in log[(i + 3):]: |
There was a problem hiding this comment.
I think the above comment was not addressed
arkane/orca.py
Outdated
| Run gaussian to get E0 | ||
| """ | ||
|
|
||
| raise LogError('Could not find a successfully load Orca scan:Parser is not inpemented') |
There was a problem hiding this comment.
I think the above comment was not addressed
arkane/orca.py
Outdated
|
|
||
| def load_energy(self, zpe_scale_factor=1.): | ||
| """ | ||
| Load the energy in J/ml from a Orca log file. Only the last energy |
There was a problem hiding this comment.
I think the above comment was not addressed
arkane/orca.py
Outdated
| zpe = float(line.split()[-4]) * constants.E_h * constants.Na | ||
| if zpe is not None: | ||
| return zpe | ||
| else: |
There was a problem hiding this comment.
I think the above comment was not addressed
arkane/orca.py
Outdated
|
|
||
| def load_scan_energies(self): | ||
|
|
||
| raise LogError('Could not find a successfully load Orca scan:Parser is not inpemented') |
There was a problem hiding this comment.
I think the above comment was not addressed
arkane/orca.py
Outdated
| if 'T1 diagnostic ' in line: | ||
| items = line.split() | ||
| return float(items[-1]) | ||
| raise ValueError('Unable to find T1 diagnostic in energy file: {}'.format(self.path)) |
There was a problem hiding this comment.
I think the above comment was not addressed
alongd
left a comment
alongd
left a comment
There was a problem hiding this comment.
Thanks! I added some comments, there were few comments from the original review that were not addressed
arkane/orca.py
Outdated
| atom, coord, number, mass = [], [], [], [] | ||
|
|
||
| with open(self.path) as f: | ||
| log = f.read().splitlines() |
There was a problem hiding this comment.
I didn't see any response to this comment. Could you take a look?
arkane/orca.py
Outdated
| if 'CARTESIAN COORDINATES (ANGSTROEM)' in line: | ||
| for line in log[(i + 3):]: | ||
| if not line.strip(): | ||
| for coordinates in log[(i + 2):]: |
There was a problem hiding this comment.
I think this should remain line - otherwise it'll be confusing for someone else to read this code, they would expect coordinates to contain actual coordinates (list or tuple), and not a string with additional info
arkane/orca.py
Outdated
| def load_energy(self, zpe_scale_factor=1.): | ||
| """ | ||
| Load the energy in J/ml from a Orca log file. Only the last energy | ||
| Load the energy in J/ml from an Orca log file. Only the last energy |
There was a problem hiding this comment.
Could you remove the extra space after Orca?
arkane/orca.py
Outdated
| zpe = float(line.split()[-4]) * constants.E_h * constants.Na | ||
| if zpe is not None: | ||
| return zpe | ||
| else: |
There was a problem hiding this comment.
I would change this part to:
if zpe is None:
raise LogError('Unable to find zero-point energy in Orca output file.')
return zpe
|
Thank you for the comments. I addressed all the comments. I left the comments on the original thread. |
|
@dranasinghe, take a look at the Codacy report for this branch, it suggests to add some spaces in |
alongd
left a comment
alongd
left a comment
There was a problem hiding this comment.
There's one comment which I think wasn't addressed. Feel free to squash all fixups
arkane/orca.py
Outdated
| atom, coord, number, mass = [], [], [], [] | ||
|
|
||
| with open(self.path) as f: | ||
| log = f.read().splitlines() |
There was a problem hiding this comment.
It still shows f.read().splitlines(), not f.readlines(). Am I missing something?
alongd
left a comment
alongd
left a comment
There was a problem hiding this comment.
Looking good!
I added minor comments. Let's also squash all fixup comments. I think the final commits should be something like:
- Added Orca to Arkane
- Added Orca to statmech
- Added tests
arkane/orca.py
Outdated
| atom, coord, number, mass = [], [], [], [] | ||
|
|
||
| with open(self.path) as f: | ||
| log = f.read().splitlines() |
There was a problem hiding this comment.
Do you mean when you replaced it with lines = f.readlines()? Strange, we use this format frequently. What problem did you have? Would you like us to look at it together?
alongd
left a comment
alongd
left a comment
There was a problem hiding this comment.
I added just minor comments.
BTW, why don't we parse frequencies from Orca? I saw that one of the test files is opt-freq, but only used to parse the energy.
arkane/orca.py
Outdated
| raise LogError('The load_force_constant_matrix method is not implemented for Orca Logs') | ||
|
|
||
| def load_geometry(self): | ||
|
|
dranasinghe
left a comment
dranasinghe
left a comment
There was a problem hiding this comment.
Thank you for the comments. Once @alongd code for the translation and rotational modes get implemented I will include frequencies and rotor scans.
|
@dranasinghe, I think we're all set. Squash and rebase? |
minor: address minor issues raised by RMG developers
alongd
left a comment
alongd
left a comment
There was a problem hiding this comment.
Thanks for this awesome contribution!!
3 participants
add OrcaLog and orca test to Arkane. currently limited to parsing energy, geometry, number of atoms, t1 diagnostic. Freq, spin multiplicity, scan, conformers are not implemented