search

ReactionMechanismGenerator / AutoTST

AutoTST: A framework to perform automated transition state theory calculations
Other
32 stars 16 forks source link

Added connect_the_dots Vibrational Analysis methods to validate TS #78

Closed davidfarinajr closed 4 years ago

davidfarinajr commented 4 years ago

The main contributions of this PR are 2 new methods to the Vibrational Analysis calculator. 1) _get_complexes_from_rxn_label() This method is used in validate_by_connecting_the_dots() to obtain the rmg reactant and product complexes, determined by the smiles in the ts reaction label. This method converts all bonds to single in order to check isomorphism to molecules created from_xyz coords from a log file. 2) validate_by_connecting_the_dots() This method validates a TS by checking the isomorphism between the reaction label reactant and product complexes (obtained through _get_complexes_from_rxn_label()) and the two rmgpy.molecule complexes generated using from_xyz with the coordinates adjusted (plus and minus) by the atom displacements from the negative frequency.

I also added unit test for validate_by_connecting_the_dots(). Other small changes include 1) added log_file to init a Vibrational Analysis calc 2) added parser as Vibrational Analysis calc attribute. 3) added Br to obtain_geometries symbol dict

codecov[bot] commented 4 years ago

Codecov Report

Merging #78 into master will increase coverage by 0.27%. The diff coverage is 81.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #78      +/-   ##
==========================================
+ Coverage   64.64%   64.91%   +0.27%     
==========================================
  Files          27       27              
  Lines        4644     4712      +68     
==========================================
+ Hits         3002     3059      +57     
- Misses       1642     1653      +11
Impacted Files Coverage Δ
autotst/calculator/vibrational_analysis.py 80.76% <78.46%> (-5.54%) :arrow_down:
autotst/calculator/vibrational_analysis_test.py 85.52% <100.00%> (+2.44%) :arrow_up:
autotst/reaction.py 80.50% <0.00%> (+0.84%) :arrow_up:

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 70347df...b72f2b8. Read the comment docs.

nateharms commented 4 years ago

Also, it looks like you're running into this issue in testing. Might be because you tested with a different branch of RMG? The testing suite uses the binary version. 

ERROR: test_validate_by_connecting_the_dots (autotst.calculator.vibrational_analysis_test.VibrationalAnalysisTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/ReactionMechanismGenerator/AutoTST/autotst/calculator/vibrational_analysis_test.py", line 135, in test_validate_by_connecting_the_dots
    self.assertTrue(self.vibrational_analysis2.validate_by_connecting_the_dots())
  File "/home/travis/build/ReactionMechanismGenerator/AutoTST/autotst/calculator/vibrational_analysis.py", line 314, in validate_by_connecting_the_dots
    complex1, complex2 = make_complexes(i)
  File "/home/travis/build/ReactionMechanismGenerator/AutoTST/autotst/calculator/vibrational_analysis.py", line 296, in make_complexes
    raise_atomtype_exception=False,
  File "rmgpy/molecule/molecule.py", line 1706, in rmgpy.molecule.molecule.Molecule.from_xyz
TypeError: from_xyz() got an unexpected keyword argument 'raise_atomtype_exception'
davidfarinajr commented 4 years ago

Yes, I guess the binary version doesn't have raise_atomtype_exception as keyword for from_xyz. Should I remove that keyword?

nateharms commented 4 years ago

Yea, remove it for now and add a comment saying that we should add it in when the binary gets updated. It should be good for now. You may also want to make it into a try and except as well because I'm guessing that argument suppressed errors.

davidfarinajr commented 4 years ago

This PR will likely conflict with https://github.com/ReactionMechanismGenerator/AutoTST/pull/79. Lets merge https://github.com/ReactionMechanismGenerator/AutoTST/pull/79 in, then I'll rebase this one.

nateharms commented 4 years ago

Well, it looks like this could be merged in automatically and all the checks pass. Shall we merge it in? @davidfarinajr

davidfarinajr commented 4 years ago

hmm, interesting. I thought there might be conflicts, but apparently not. Do you want me to rebase it just in case?

nateharms commented 4 years ago

If you're willing to -- sure, go for it and rebase. Better safe than sorry. ¯\(ツ)\/¯

davidfarinajr commented 4 years ago

I rebased it locally and it looks good, no conflicts. You can go ahead and merge it! Thanks