search

pele-python / mcpele

Monte Carlo and parallel tempering routines built on the pele foundation
Other
20 stars 5 forks source link

Radial distribution function #28

Closed kjs73 closed 10 years ago

kjs73 commented 10 years ago

Add radial distribution function action. C++ version works for any boxdimension. Cython version works for 2D and 3D. Depends on pele::distance and on mcpele::histogram. Integer template parameters are handled like for the inverse power-law potential of pele.

js850 commented 10 years ago

Looks pretty good I think, though it could do with a bit more documentation. It's not totally clear what some of the functions do. e.g. what's the difference between get_hist_r and get_hist_gr? Actually I see now, but it took a few minutes.

Also, will this create a cumulative pair correlation function or one for each time it's called?

This is order N^2, so it could be a bottleneck in the calculation. You might want to pass an option that it is called only rarely.

kjs73 commented 10 years ago

Thank you for the feedback!

Some documentation has been added now. It also contains explanation of get_hist_r and get_hist_gr. This is not very elegant, but I was not sure how to deal e.g. with std::pair in cython.

As it is implemented now, it will accumulate all passed configuration snapshots in the same histogram. This is also contained in the documentation now.

A parameter record_every has been added, such that it is possible to call the measurement only a few times. In addition, one could also consider a fixed, N independent number of pairs instead of all pairs to get rid of the N^2 dependence, but probably this is not needed here.

smcantab commented 10 years ago

you can wrap the std::pair in cython just as we did for the shared pointers. Then you can wrap the functions first and second or even the assignment operator []. that way you can use pairs in cython just as you would do in c++ and you can have pele arrays of pairs that you can iterate over.

Stefano

On Wed, Aug 13, 2014 at 8:44 PM, kjs73 notifications@github.com wrote:

Thank you for the feedback!

Some documentation has been added now. It also contains explanation of get_hist_r and get_hist_gr. This is not very elegant, but I was not sure how to deal e.g. with std::pair in cython.

As it is implemented now, it will accumulate all passed configuration snapshots in the same histogram. This is also contained in the documentation now.

A parameter record_every has been added, such that it is possible to call the measurement only a few times. In addition, one could also consider a fixed, N independent number of pairs instead of all pairs to get rid of the N^2 dependence, but probably this is not needed here.

— Reply to this email directly or view it on GitHub https://github.com/pele-python/mcpele/pull/28#issuecomment-52092066.

js850 commented 10 years ago

I think its fine the way it is without needing pairs.

kjs73 commented 10 years ago

I agree that the solution with pairs that I thought of initially is not needed and would probably be more complicated. Also it would be less flexible if we wanted to return more information in later versions.

On 13 August 2014 21:01, Jacob Stevenson notifications@github.com wrote:

I think its fine the way it is without needing pairs.

— Reply to this email directly or view it on GitHub https://github.com/pele-python/mcpele/pull/28#issuecomment-52102504.

smcantab commented 10 years ago

Ok, it was more of a FYI than an actual suggestion :)

On Wed, Aug 13, 2014 at 10:36 PM, kjs73 notifications@github.com wrote:

I agree that the solution with pairs that I thought of initially is not needed and would probably be more complicated. Also it would be less flexible if we wanted to return more information in later versions.

On 13 August 2014 21:01, Jacob Stevenson notifications@github.com wrote:

I think its fine the way it is without needing pairs.

— Reply to this email directly or view it on GitHub https://github.com/pele-python/mcpele/pull/28#issuecomment-52102504.

— Reply to this email directly or view it on GitHub https://github.com/pele-python/mcpele/pull/28#issuecomment-52106949.

kjs73 commented 10 years ago

Thank you! Now I looked again at the shared_prt werapping and I think I got the general point.