search

pele-python / pele

Python energy landscape explorer
Other
95 stars 41 forks source link

Implementing periodic boundaries for rigid bodies #96

Closed sniblett402 closed 9 years ago

sniblett402 commented 10 years ago

Lots of the changes here are entirely cosmetic or else were intended entirely for my use - e.g. most of the new comments and the changes to pymolwrapper.py which were designed to allow visualisation of small rigid-body periodic systems.

The important changes are the ones where I've defined new or overloaded functions and classes. In particular the new _otp_bulk module, the new classes in minpermdist_stochastic.py and periodic_exact_match.py, and the changes to the cpp files/their wrappers. I've also put in a fairly extensive test routine.

js850 commented 10 years ago

Thanks sam. I'll have a look at it tomorrow.

js850 commented 10 years ago

Hi Sam, everything looks pretty good I think. I made a bunch of comments, but most of them were about coding style. We should talk about how to implement MeasureRigidBulk because we actually want to combine functionality from both MeasureRigidBody and MeasurePeriodic. It's not trivial how best to do that.

I've done a bit of work on the current pele master branch. Try to merge the current master into this branch. Hopefully there will not be too many conflicts. Once it's merged and pushed back your fork the changes will show up automatically in this pull request.

In the meantime I'm going to look at better ways of implementing the changes you made to the c++ code.

js850 commented 10 years ago

Hi Sam, I redid how the c++ aatopology handles periodic vs cartesian distance to be more flexible. If you give me write access to your pele fork I can push my changes to your branch and they can be included in this PR (pull request).

js850 commented 10 years ago

I pushed my commits, I explained my thinking in one of the commit messages.

The c++ RigidFragment now accepts another paramter std::shared_ptr distance_function. Both CartesianDistanceWrapper and PeriodicDistanceWrapper inherit from DistanceInterface, so either of those can be used as the distance function.

I could have done it by templating RigidFragment, which would have been faster, but the code ends up simpler using interfaces. The distance function is called over and over again, so the loss of speed may end up being a problem. I think it will be fine, but we should user profiling to test for sure.

js850 commented 9 years ago

I think we can merge after you fix that issue with how self.cpp_topology is called

js850 commented 9 years ago

Hi Sam, It looks like you fixed those last issues. Should I merge this?

sniblett402 commented 9 years ago

If you're happy with it, then yes please!

It passes all the tests I can think to run on it, so hopefully it is now working within the limitations we were expecting.

Thanks,

Sam

On 2014-12-01 12:08, Jacob Stevenson wrote:

Hi Sam, It looks like you fixed those last issues. Should I merge this?

Reply to this email directly or view it on GitHub [1].

*

Links:

[1] https://github.com/pele-python/pele/pull/96#issuecomment-65056017