ReactionMechanismGenerator / AutoTST

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

Addressed typos in Updating Methods for the TS Databases #63

Closed nateharms closed 4 years ago

nateharms commented 4 years ago

There were some typos and py2->py3 issues that crept up when I was trying to use previous updating methods to update the TS database with new TS training data. This PR addresses makes these fixes. Two more PRs will exist that will (1) provide an example script that can update the database automatically and (2) provide an updated database with new AutoTST numbers.

codecov[bot] commented 4 years ago

Codecov Report

Merging #63 into master will increase coverage by 1.87%. The diff coverage is 16.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #63      +/-   ##
==========================================
+ Coverage   62.83%   64.71%   +1.87%     
==========================================
  Files          27       27              
  Lines        4601     4806     +205     
==========================================
+ Hits         2891     3110     +219     
+ Misses       1710     1696      -14     
Impacted Files Coverage Δ
autotst/data/update_test.py 93.33% <0.00%> (+38.78%) :arrow_up:
autotst/reaction_test.py 99.58% <0.00%> (+0.08%) :arrow_up:
autotst/data/update.py 10.31% <0.00%> (+3.10%) :arrow_up:
autotst/reaction.py 84.66% <0.00%> (+4.79%) :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 cd1b67f...7798f2e. Read the comment docs.

nateharms commented 4 years ago

@davidfarinajr can you look at this? Also, what are our thoughts on 3 out of 4 checks passing? The only check not passing is the coverage for the patch.

nateharms commented 4 years ago

Okay, so all of the tests pass and overall coverage increased but the coverage for the path is low. Are we okay with merging this in anyway or should I add more testing for the patch?

nateharms commented 4 years ago

@davidfarinajr @rwest any complaints about the patch coverage? Otherwise, can we merge this in?

davidfarinajr commented 4 years ago

No complaints from me

rwest commented 4 years ago

Go ahead