RMeli / irc

Transfrormation between Cartesian coordinates and redundant internal coordinates
MIT License
22 stars 8 forks source link

Bugfix/linear angle gradient #37

Closed peterbygrave closed 5 years ago

peterbygrave commented 5 years ago

This PR corrects the linear angle and its gradient

The angle was being calculated defining the orthogonal axis as the additional position rather than using it as a displacement from the central atom. Previous tests worked because its not a "bad" position when molecule is close to the origin.

The second fix is that the angle gradient was wrong. Angle bends has st vector for the terminal atoms, and the central atom is the negative of the sum of these two st vector.

codecov[bot] commented 5 years ago

Codecov Report

Merging #37 into master will increase coverage by 0.02%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #37      +/-   ##
==========================================
+ Coverage   98.26%   98.28%   +0.02%     
==========================================
  Files          20       20              
  Lines        2012     2036      +24     
==========================================
+ Hits         1977     2001      +24     
  Misses         35       35
Impacted Files Coverage Δ
include/libirc/connectivity.h 98.35% <100%> (ø) :arrow_up:
include/libirc/wilson.h 95.2% <100%> (ø) :arrow_up:
src/test/connectivity_test.cpp 100% <100%> (ø) :arrow_up:
src/test/wilson_test.cpp 100% <100%> (ø) :arrow_up:
include/libirc/irc.h 80.55% <0%> (ø) :arrow_up:
include/libirc/atom.h 100% <0%> (ø) :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 22726d8...cfacd6e. Read the comment docs.

RMeli commented 5 years ago

I updated the documentation for the linear_angle_gradient function. @peterbygrave do you have some references I can add?

RMeli commented 5 years ago

Merged master after the dihedral bugfix #35 and corrected the failing tests accordingly (already identified by @peterbygrave).

RMeli commented 5 years ago

I'll merge now since it's a critical bug. Documentation will be added later.