ReactionMechanismGenerator / AutoTST

AutoTST: A framework to perform automated transition state theory calculations

Other

32 stars 16 forks source link

try is_linear() #76

Closed davidfarinajr closed 4 years ago

davidfarinajr commented 4 years ago

conformer.rmg_molecule.is_linear() raises IndexError for autotst TS's that have atoms with no bonds such as an H_Abstraction by an atom (H, F, Cl, or Br). This commit adds a try and except block to keep it from crashing.

davidfarinajr commented 4 years ago

This is where we are getting the IndexError in the molecule.is_linear() method. We are not checking if len(bonds) == 0

       for atom in self.vertices:
            bonds = list(atom.edges.values())
            if len(bonds) == 1:
                continue  # ok, next atom
            if len(bonds) > 2:
                break  # fail!
            if bonds[0].is_single() and bonds[1].is_triple():
                continue  # ok, next atom
            if bonds[1].is_single() and bonds[0].is_triple():
                continue  # ok, next atom
            break  # fail if we haven't continued
        else:
            # didn't fail
            return True

Perhaps we should open a PR with rmgpy as well. Although this method is not typically used for rmg molecules with nonbonded atoms

codecov[bot] commented 4 years ago

Codecov Report

Merging #76 into master will decrease coverage by 0.02%. The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master      #76      +/-   ##
==========================================
- Coverage   64.66%   64.64%   -0.03%     
==========================================
  Files          27       27              
  Lines        4641     4644       +3     
==========================================
+ Hits         3001     3002       +1     
- Misses       1640     1642       +2

Impacted Files	Coverage Δ
autotst/species.py	`72.32% <66.66%> (-0.19%)`	:arrow_down:

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 fb37081...ae7b89c. Read the comment docs.

nateharms commented 4 years ago

Looking at the PR itself, it looks good and makes sense. However, there's a weird issue with the decrease in coverage. Once this is addressed and the tests pass, I'm happy to merge this in.

davidfarinajr commented 4 years ago

coverage looks fine now. I messed up my first commit and had to force push. Mixed up continue and pass 🙃