logsdail / carmm

Scripts for creation, manipulation and analysis of geometric and electronic structure of molecular models
GNU General Public License v3.0
5 stars 18 forks source link

Geodesic Interpolated Starting Path #158

Closed GabrielBram closed 2 months ago

GabrielBram commented 6 months ago

Implements the initial path construction specified in: https://pubs.aip.org/aip/jcp/article/150/16/164103/198363/Geodesic-interpolation-for-reaction-pathways.

Code by and large lifted from the repository associated with the paper: https://github.com/virtualzx-nad/geodesic-interpolate/tree/master. However, this implementation honours FixAtoms constraints and periodic boundary conditions in ASE.

Essentially gives a more robust starting guess for the initial path than linear interpolation. Although the full geodesic interpolation algorithm is not implemented, the path should give a better starting guess for IDPP (IDPP uses the linear interpolation as a starting guess and suffers from nasty clashes if two atoms pass through one another).

Test case shows a 120 degree rotation of a CH3 group in ethane, which would otherwise fail spectacularly with the default IDPP interpolation without a better starting guess.

Best use for now would look like:

from carmm.build.neb.geodesic import GeodesicInterpolator
from ase.mep import idpp_interpolate, NEB

Geodesic = GeodesicInterpolator(initial, final, 6, None)
Geodesic.init_path()

idpp_interpolate(Geodesic.images, mic=True)
mep = NEB(Geodesic.images)
...
codecov[bot] commented 6 months ago

Codecov Report

Attention: Patch coverage is 96.55172% with 6 lines in your changes missing coverage. Please review.

Project coverage is 88.84%. Comparing base (001ce56) to head (e8452a0). Report is 12 commits behind head on main.

Files Patch % Lines
carmm/build/neb/geodesic.py 95.83% 4 Missing :warning:
carmm/build/neb/geodesic_utils.py 96.66% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #158 +/- ## ========================================== + Coverage 88.44% 88.84% +0.40% ========================================== Files 82 85 +3 Lines 3340 3514 +174 ========================================== + Hits 2954 3122 +168 - Misses 386 392 +6 ``` | [Flag](https://app.codecov.io/gh/logsdail/carmm/pull/158/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andrew+Logsdail) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/logsdail/carmm/pull/158/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andrew+Logsdail) | `88.84% <96.55%> (+0.40%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andrew+Logsdail#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

GabrielBram commented 6 months ago

(A warning: This is all desperately uncommented)

logsdail commented 2 months ago

@GabrielBram Is this still needed? Or does #172 supersede this request, and it should be closed/merged?

GabrielBram commented 2 months ago

@GabrielBram Is this still needed? Or does #172 supersede this request, and it should be closed/merged?

My mistake - I thought this was already merged. This branch is superseded by the more recently PR. Will close.

GabrielBram commented 2 months ago

Will clean the current branch once the newer one is merged.