Terachem #1788

alongd · 2019-10-27T15:51:19Z

Motivation or Problem

We'd like to expand the electronic structure software Arkane supports

Description of Changes

Added full support for TeraChem.

I couldn't figure out how to parse the moments of inertia out of a TeraChem log file (I think they might not be reported, although they are certainly calculated and used internally), so some general functionalities for calculating them from xyz were added to common.py. Note that neither RDKit nor OB were used to have the capability to calculate moments of inertia for TSs as well. These functions are used in statmech if no rotational mode exists in conformer.modes. Could be useful in the future for additional ESS. I verified that the results are in agreement with those Gaussian would have calculated for a few species, and added tests.

Sine we have additional future plans to expand ESS support in Arkane (Orca, Psi4), we'll end up with many ESS log parsing and respective tests files under the arkane folder. For this to not get out of hand, a logs subdir was created for all ESS log files and their tests, including the Log parent class.

Testing

Plenty of tests were added.

Reviewer Tips

See that the code makes sense, verify that the tests are comprehensive and run them.

I'm tagging @dranasinghe as the reviewer and @mjohnson541 as the merger.

dranasinghe

I have made some comments to see whether you agree. They are mostly for the terachem Parser. for the moments of inertia, I could n't recreate the Gaussian output for CH4 manually. I will keep looking into that.

dranasinghe · 2019-10-31T18:14:29Z

arkane/logs/terachem.py

+        if len(coords) == 0 or len(numbers) == 0 or len(masses) == 0 \
+            or ((len(coords) != num_of_atoms or len(numbers) != num_of_atoms or len(masses) != num_of_atoms)
+                and num_of_atoms is not None):
+            raise LogError(f'Unable to read atoms from TeraChem geometry output file {self.path}.')


What happened if someone gives geometry optimization output file as output? I think it better to suggest user give frequency calculation, output.geomatry or .xyz or if parse could n't find a Since there is .xyz, ouput.geomatry and frequency

excellent suggestion, added

dranasinghe · 2019-10-31T18:18:49Z

arkane/logs/terachem.py

+                        # Matrix element rows
+                        for j in range(n_rows):  # for j in range(i*6, Nrows):
+                            line = f.readline()
+                            while len(line.split()) != 7:


What will happen if you have an odd number of atoms eg. 11. you will have a 33*33 matrix and the last block will only have 3 columns, which means while loop will exit.

let's add as a test.

good catch! test added

arkane/logs/terachem.py

dranasinghe · 2019-10-31T20:27:33Z

arkane/logs/terachem.py

+                if 'Scan Cycle' in lines[line_index]:
+                    # example: '-=#=-     Scan Cycle     7/36        -=#=-'
+                    v_index += 1
+                if 'FINAL ENERGY:' in lines[line_index]:


you need to grab "Optimized Energy" value

I don't see Optimized Energy in a scan log file. Let's talk about it

arkane/common.py

alongd

Thanks!! Comments were addressed, and fixup commits were added

arkane/common.py

alongd · 2019-11-01T14:26:30Z

arkane/logs/terachem.py

+        if len(coords) == 0 or len(numbers) == 0 or len(masses) == 0 \
+            or ((len(coords) != num_of_atoms or len(numbers) != num_of_atoms or len(masses) != num_of_atoms)
+                and num_of_atoms is not None):
+            raise LogError(f'Unable to read atoms from TeraChem geometry output file {self.path}.')


excellent suggestion, added

arkane/logs/terachem.py

alongd · 2019-11-01T14:39:45Z

arkane/logs/terachem.py

+                if 'Scan Cycle' in lines[line_index]:
+                    # example: '-=#=-     Scan Cycle     7/36        -=#=-'
+                    v_index += 1
+                if 'FINAL ENERGY:' in lines[line_index]:


I don't see Optimized Energy in a scan log file. Let's talk about it

alongd · 2019-11-01T19:23:57Z

arkane/logs/terachem.py

+                        # Matrix element rows
+                        for j in range(n_rows):  # for j in range(i*6, Nrows):
+                            line = f.readline()
+                            while len(line.split()) != 7:


good catch! test added

alongd · 2019-11-02T00:27:50Z

Remaining things to do:

parse E from either freq (first) or opt (last), appropriately
in scans, use opt E
check number of scan points according to the input
check termination
test failed job

alongd · 2019-11-04T02:21:40Z

@dranasinghe, I addressed all the comments we discussed (online and offline), please take a look when you get a chance.

alongd · 2019-11-04T02:24:03Z

@goldmanm, I implemented in TeraChem the rotor scan checks we discussed, please take a look and let me know what you think. Feel free to replicate for Gaussian and/or make suggestions.

dranasinghe

Thank for addressing the issues. The code looks good. :)

goldmanm · 2019-11-05T19:16:08Z

The part about rotor scans looks good. I had one comment about it, listed below.

I've implemented the gaussian changes, and I'm just waiting on the more substantial bugfix pr #1810 to go through to reduce merge conflicts.

goldmanm · 2019-11-05T19:26:38Z

arkane/logs/terachem.py

        logging.info('   Assuming {0} is the output from a TeraChem PES scan...'.format(os.path.basename(self.path)))

+        v_list.append(v_list[0])  # TeraChem does not repeat the first scan point (unlike Gaussian), append it


Is there a reason why we need to repeat the data at the end? I think adding data should be avoided if possible. The only reason I can think of is how we define 'angles', will have a point at the end.

A workaround might be to not add the extra datapoint and to replace the angle definition on line 310 so that it does not have the 2$\pi$ angle.

angles = np.linspace(0, 2 * math.pi * (len(v_list) -1) / len(v_list), len(v_list), np.float64)

for example, if there are 4 data points (90 degree spacing), the angles output will be 0, 0.5$\pi$, $\pi$, and 1.5$\pi$.

I found a way to repeat the last point in TeraChem instead, I'll change these lines. We do it as a quality assurance method of checking that a 360 scan gives almost the same energy as the initial point (checked in ARC)

codecov · 2019-11-05T19:43:09Z

Codecov Report

Merging #1788 into master will decrease coverage by 1.16%.
The diff coverage is 14.14%.

@@            Coverage Diff             @@
##           master    #1788      +/-   ##
==========================================
- Coverage   43.02%   41.85%   -1.17%     
==========================================
  Files          80       82       +2     
  Lines       21099    21574     +475     
  Branches     5516     5653     +137     
==========================================
- Hits         9077     9030      -47     
- Misses      11004    11539     +535     
+ Partials     1018     1005      -13

Impacted Files	Coverage Δ
arkane/encorr/corr.py	`53.65% <ø> (ø)`	⬆️
arkane/logs/log.py	`92.3% <ø> (ø)`
arkane/logs/molpro.py	`9.16% <100%> (ø)`
arkane/logs/gaussian.py	`81.81% <100%> (ø)`
arkane/logs/orca.py	`20.19% <100%> (ø)`
arkane/logs/terachem.py	`12.5% <12.5%> (ø)`
arkane/logs/qchem.py	`22% <50%> (ø)`
... and 10 more

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 e0a78cf...1132931. Read the comment docs.

alongd · 2019-11-11T01:59:28Z

@mjohnson541, thanks for the comments, I reverted the style changes (rebased and squashed). That commit was not removed, though, it instead applies a consistent style to all of Arkan'es files (now keeping the #### lines). The only exception is common.py where the #### lines are deleted, but added in a subsequent commit. Sorry for that, it just created an annoying conflict otherwise.
I also added a minor commit, correcting some docstring in RMG Molecule, since it's very minor I thought it's OK to just append it here instead of creating a PR for it. No other changes other than that.
I think this PR should be good to go, let me know if you have any additional comments.

alongd · 2019-11-20T18:20:38Z

@mjohnson541, I think all comments were addressed. This branch is now rebased. Please feel free to merge unless there are additional concerns.

Soon we'll add support for many additional ESS, for better organization and readability, these files were relocated.

And restructured this function

And removed existing determine_qm_software tests from GaussianTest

alongd added Type: Feature Arkane labels Oct 27, 2019

alongd requested review from mjohnson541 and dranasinghe October 27, 2019 15:51

alongd self-assigned this Oct 27, 2019

alongd force-pushed the terachem branch from 7717ef5 to f916b76 Compare October 27, 2019 16:25

alongd mentioned this pull request Oct 29, 2019

Add LogError to raise LogError for unconverged gaussian log file #1766

Merged

alongd force-pushed the terachem branch 3 times, most recently from 142c6bb to c388681 Compare October 30, 2019 15:35

dranasinghe reviewed Oct 31, 2019

View reviewed changes

alongd force-pushed the terachem branch from c388681 to 86133b2 Compare November 1, 2019 14:42

alongd commented Nov 1, 2019

View reviewed changes

alongd force-pushed the terachem branch 2 times, most recently from 958b630 to 059fb6f Compare November 1, 2019 20:31

alongd force-pushed the terachem branch 6 times, most recently from ff781c0 to 3c2f5dd Compare November 3, 2019 18:31

ReactionMechanismGenerator deleted a comment from codecov bot Nov 4, 2019

dranasinghe reviewed Nov 5, 2019

View reviewed changes

goldmanm reviewed Nov 5, 2019

View reviewed changes

alongd force-pushed the terachem branch from 3c2f5dd to 6b93403 Compare November 5, 2019 19:43

alongd force-pushed the terachem branch from e257c15 to 7bc036d Compare November 11, 2019 01:53

alongd force-pushed the terachem branch 5 times, most recently from 2d3f042 to 0995767 Compare November 19, 2019 18:52

alongd added 13 commits November 20, 2019 20:32

Relocated all ESS log classes to a designated subfolder

696e3c8

Soon we'll add support for many additional ESS, for better organization and readability, these files were relocated.

Minor: Replaced f.read().splitlines() with f.readlines() in QChem log

868f999

Minor style modifications throughout Arkane

397e7ea

Added TeraChemLog

f420efb

Tests: Added tests for TeraChemLog

a3c2d21

Added TeraChemLog to Arkane statmech

2bc0768

Added TeraChem to determine_qm_software()

9f057b7

And restructured this function

Tests: Added utilTests

0cf2c01

And removed existing determine_qm_software tests from GaussianTest

Added functionalities for calculating moments of inertia from xyz

a6d8766

Tests: Added TestMomentOfInertia to Arkane common test

6d5a434

Add translation and rotation modes if missing from species.conformer

4f7d62f

Minor: Improved lower conformer in scan warning

e6a84d9

Minor: Docstring corrections to Molecule.get_deterministic_sssr

1132931

alongd force-pushed the terachem branch from 0995767 to 1132931 Compare November 21, 2019 01:33

alongd added the For Next Release label Nov 21, 2019

mjohnson541 approved these changes Nov 22, 2019

View reviewed changes

mjohnson541 merged commit bfffc5b into master Nov 22, 2019

mjohnson541 deleted the terachem branch November 22, 2019 04:23

alongd removed the For Next Release label Nov 22, 2019

alongd mentioned this pull request Nov 22, 2019

Updated Arkane logs imports ReactionMechanismGenerator/ARC#255

Merged

mliu49 mentioned this pull request Nov 26, 2019

RMG v3.0.0 Release Planning #1830

Closed

mliu49 mentioned this pull request Dec 16, 2019

RMG-Py v3.0.0 Release #1852

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Terachem #1788

Terachem #1788

alongd commented Oct 27, 2019

dranasinghe left a comment

dranasinghe Oct 31, 2019

alongd Nov 1, 2019

dranasinghe Oct 31, 2019

dranasinghe Oct 31, 2019

alongd Nov 1, 2019

dranasinghe Oct 31, 2019

alongd Nov 1, 2019

alongd left a comment

alongd Nov 1, 2019

alongd Nov 1, 2019

alongd Nov 1, 2019

alongd commented Nov 2, 2019 •

edited

Loading

alongd commented Nov 4, 2019

alongd commented Nov 4, 2019

dranasinghe left a comment

goldmanm commented Nov 5, 2019

goldmanm Nov 5, 2019

goldmanm Nov 5, 2019

alongd Nov 5, 2019

codecov bot commented Nov 5, 2019 •

edited

Loading

alongd commented Nov 11, 2019

alongd commented Nov 20, 2019

		logging.info(' Assuming {0} is the output from a TeraChem PES scan...'.format(os.path.basename(self.path)))

		v_list.append(v_list[0]) # TeraChem does not repeat the first scan point (unlike Gaussian), append it

Terachem #1788

Terachem #1788

Conversation

alongd commented Oct 27, 2019

Motivation or Problem

Description of Changes

Testing

Reviewer Tips

dranasinghe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alongd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alongd commented Nov 2, 2019 • edited Loading

alongd commented Nov 4, 2019

alongd commented Nov 4, 2019

dranasinghe left a comment

Choose a reason for hiding this comment

goldmanm commented Nov 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 5, 2019 • edited Loading

Codecov Report

alongd commented Nov 11, 2019

alongd commented Nov 20, 2019

alongd commented Nov 2, 2019 •

edited

Loading

codecov bot commented Nov 5, 2019 •

edited

Loading