Arkane bugfixes #1810
Arkane bugfixes #1810
Conversation
goldmanm
force-pushed
the
arkane_bugfixes
branch
from
b0970ff to
a458abd
Compare
goldmanm
force-pushed
the
arkane_bugfixes
branch
from
a458abd to
9f559c6
Compare
arkane/gaussian.py
Outdated
| with open(self.path, 'r') as f: | ||
| line = f.readline() | ||
| while line != '': | ||
|
|
||
| if 'SCF Done:' in line: | ||
| e_elect = float(line.split()[4]) * constants.E_h * constants.Na | ||
| elect_energy_source = 'SCF' | ||
| elif 'MP2 =' in line: |
There was a problem hiding this comment.
We assume these energies come after SCF Done, and that each occurs once?
Can you add unit tests?
If you'd like, add a comment giving an example of the line you're parsing for each case, it makes code review / maintenance / debug much easier.
There was a problem hiding this comment.
I'm going to drop this commit because I am not sure I even have the files or knowledge anymore to unittest it. I've been using molpro for all my calculations, so it also doesn't effect my work at all. I think I might have written it to help someone else with some calculation, but this was too long ago and I can't really remember.
There was a problem hiding this comment.
before you drop it, maybe @dranasinghe can take a look?
There was a problem hiding this comment.
It'd be useful to fix this bug (if it is still a bug), I just completely forgot why I made the change and what exactly it fixes.
I dropped from this PR, though we can add it back or make a new PR. Here's access to the commit: ab0705c
goldmanm
force-pushed
the
arkane_bugfixes
branch
2 times, most recently
from
99f43f2 to
ee36a92
Compare
Codecov Report
@@ Coverage Diff @@
## master #1810 +/- ##
==========================================
+ Coverage 42.99% 43.02% +0.02%
==========================================
Files 80 80
Lines 21090 21099 +9
Branches 5513 5517 +4
==========================================
+ Hits 9067 9077 +10
+ Misses 11008 10990 -18
- Partials 1015 1032 +17
Continue to review full report at Codecov.
|
goldmanm
force-pushed
the
arkane_bugfixes
branch
2 times, most recently
from
787c7ab to
ca6ebcb
Compare
arkane/gaussian.py
Outdated
| @@ -466,6 +473,20 @@ def load_scan_frozen_atoms(self): | |||
| """ | |||
| return self._load_scan_specs('F') | |||
|
|
|||
| def load_scan_angle(self): | |||
There was a problem hiding this comment.
make these functions private with a leading _, or add an equivalent function with a "not implemented error" in all other log classes (probably the first option?)
There was a problem hiding this comment.
good catch. Added the _ to these two functions.
arkane/gaussian.py
Outdated
| output = self._load_scan_specs('S', get_after_letter_spec=True) | ||
| return float(output[0][1]) | ||
|
|
||
| def load_number_scans(self): |
There was a problem hiding this comment.
I think we can assume a 360 scan (we do it anyway, right?) and only keep one of these methods, e.g., derive the angle diff from 360 / number of scans.
There was a problem hiding this comment.
After this PR, I am going to make another PR that no longer assumes a 360 degree scan. I think there are cases where scans may be shorter (like a methyl scan with 120 degrees) giving similar accuracy with 1/3rd the computation time. Ideally we should allow users this flexibility. The code is already written to implement the more robust scanning, I just wanted to make changing the hindered rotor treatment a separate PR (since this one is long enough and the addition would require different testing).
This commit adjusts the trimolecular reactants/products Configuration's partition function used in pressure dependent calculations to agree with the gibbs free energy of individual reactants. The previous pdep partition functions were lower by a consistant factor of 66.7%. This indicates the mass was off by a factor of cuberoot(1.5)=1.144, which has been added to the partition function for trimolecular reactants/products.
This commit adds a unittest to ensure that DensityOfStates does not return NaN, as this causes errors downstream.
If a network has molecules with high energy, then normalizing by exp(-Elist/RT) could lead to python to round to zero, creating NaN in the density of states. This commit checks for this numerical error and throws an error if it occurs, so that the user knows the source of the mistake. This error is most likely to occur if atom corrections are not used or incorrect. This commit also normalizes the product density of states, so that all are consistant, unless there are no density of states at any grain, which in that case no normalization occurs to prevent divide by zero errors.
This commit adds units to the description to help people using these methods.
This commit adds a debug statement to output collision frequency values. Unlike some of the notorious debug statements, this only outputs one line, so should not cause significant issues with readability.
Previously the conversion incorrectly converted from cm^-1 to J/molec. Added avogadro number to make it J/mol.
Previously supporting_info.csv switched the placement of optical isomers and symmetry, which was due to incorrect placement of the output of Log.get_symmetry_properties. The output order was changed and the resulting supporting_info.csv is now properly output.
Previously, Arkane threw an error when tunneling=None because it would try tunneling.lower(), which only works for strings. Rearranging the order of if statements fixes this bug.
The Network docstring was missing variables, not describing what type of objects are contained in lists, and had incorrect units. This commit fixes these issues.
This commit checks to make sure energy is a Log object before trying to access energy.path, since energy can also be a float in hartrees or a tuple with various units
goldmanm
force-pushed
the
arkane_bugfixes
branch
from
ca6ebcb to
818ce9c
Compare
Previously, the string keyword ' freq ' would not discover if freq is the last keyword. This commit fixes this by also requiring removing the space but also requiring 'Geom' keyword.
This commit allows GaussianLog objects to parse the angle and the number of scans from a quantum job output. This is used to improve how GaussianLog obtains energies and their corresponding angles.
This commit adds a test for get_all_species to ensure it outputs the correct values.
goldmanm
force-pushed
the
arkane_bugfixes
branch
from
818ce9c to
57cf5ef
Compare
Motivation or Problem
When working with arkane, a number of bugs arose, which either caused arkane to crash or gave inaccurate values. These problems are distinct from #1808 because they exist in both python 2 and python 3.
Description of Changes
Each commit details the problem and changes.
Testing
Unittests and ran complicated arkane pdep networks.
Reviewer Tips
read each commit's message and make sure the changes make sense. running an RMG pdep model might also be useful.