search

eljost / pysisyphus

Python suite for optimization of stationary points on ground- and excited states PES and determination of reaction paths.
GNU General Public License v3.0
99 stars 35 forks source link

Add Geodesic interpolation #310

Closed cvsik closed 3 months ago

cvsik commented 3 months ago

This PR adds the geodesic interpolation for reaction pathways (https://doi.org/10.1063/1.5090303) to interpolate.

The only strange thing I noticed is that the pytest session seems to disable threading for scipy, such that the runtime is much slower on pytest than when running the geodesic_interpolate command manually from the command line.... not so important though.

eljost commented 3 months ago

Dear Maximilian,

thanks for the contribution. I'll take a closer look at the PR in the coming days.

eljost commented 3 months ago

Thanks again for the PR. It looks like a promising addition.

At a first glance the method seems quite slow... The alanine-dipetide already takes 42 sec on my notebook w/ 2 threads.

How did you conclude that pytest somehow disables scipy's threading? When I'm not setting OMP_NUM_THREADS=N all my cores (6 physical + 6 logical) are running at 100% utilization.

Regarding my changes in the commit: I added geodesic to the testing package, so the test is skipped when the package is not installed. I also fixed the can_geodesic, which was always False. Finally I enables --geodesic in pysistrj. Further options can't be passed though.

If you are fine with the change I'll merge the PR.

cvsik commented 3 months ago

At a first glance the method seems quite slow...

Yes, that's true. I think their Python implementation isn't the fastest, but it's giving really good interpolation results, so I'm always more than happy to spend some more time on interpolating then 😁

The changes look good! Thanks for taking care of it! 🚀