pele-python / pele

Python energy landscape explorer
Other
95 stars 41 forks source link

Invstill poly #150

Closed smcantab closed 8 years ago

smcantab commented 8 years ago

in this PR I make a structural change to the cython pairwise potentials. Now instead of deriving from pele.BasePotential directly, they derived from pele.SimplePairwisePotential, another cython interface that implements the function evaluatePairPotential: a function that allows to plot the interatomic potential as a function of distance. This makes it very convenient to check a new pairwise potential and/or getting some curves for comparison with python (potentially one day we could even plot all of the potentials somewhere automatically for reference). The only additional requirement for this to work is to initialize another pointer that I call self.spp_ptr that is a casted version of self.thisptr to the cppSimplePairwisePotentialInterface type. See _pele.pxd and _pele.pyx, the changes are really minimal.

The other change I made was to make the Stillinger inverse pairwise potential work in the polydisperse case.

kjs73 commented 8 years ago

I think it is nice to have the plotting function for pair potentials for testing etc. Do we know why the python tests are failing, the segmentation fault seems unrelated with your changes?

smcantab commented 8 years ago

a segfault? I had identified the errors in a minimizer wrapper test and a takestep rotations step. The first one I think has to do with a change in scipy bfgs implementation (I can't explain it otherwise), and the latter has to do with an AssertEqual used with doubles instead of AssertAlmostEqual

On Thu, Mar 3, 2016 at 11:50 AM, Julian Schrenk notifications@github.com wrote:

I think it is nice to have the plotting function for pair potentials for testing etc. Do we know why the python tests are failing, the segmentation fault seems unrelated with your changes?

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

kjs73 commented 8 years ago

It says something like

test_cpp_potential (pele.angleaxis.tests.test_atom_indices.TestAtomIndices) ... /home/travis/build.sh: line 45:  9035 Segmentation fault      (core dumped) nosetests -v --with-coverage pele

Maybe that is a result of what you are saying?

smcantab commented 8 years ago

ok that's the first time I see that, then different PRs are getting different errors. We need to fix it in older PRs, start merging and then see how it evolves

On Thu, Mar 3, 2016 at 11:56 AM, Julian Schrenk notifications@github.com wrote:

It says something like

test_cpp_potential (pele.angleaxis.tests.test_atom_indices.TestAtomIndices) ... /home/travis/build.sh: line 45: 9035 Segmentation fault (core dumped) nosetests -v --with-coverage pele

Maybe that is a result of what you are saying?

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

smcantab commented 8 years ago

I think we might be picking up a bug in a new/older version of scipy, if that is the case it would be a pain. The best thing to do would be to comment out the test for the time being. Basically the computation is supposed to terminate when max(abs(grad)) < tol but apparently after we terminate and check max(abs(grad)) > tol, although only slightly. It almost looks like scipy bfgs is returning the gradient before last

On Thu, Mar 3, 2016 at 11:57 AM, Stefano Martiniani < stefano.martiniani@gmail.com> wrote:

ok that's the first time I see that, then different PRs are getting different errors. We need to fix it in older PRs, start merging and then see how it evolves

On Thu, Mar 3, 2016 at 11:56 AM, Julian Schrenk notifications@github.com wrote:

It says something like

test_cpp_potential (pele.angleaxis.tests.test_atom_indices.TestAtomIndices) ... /home/travis/build.sh: line 45: 9035 Segmentation fault (core dumped) nosetests -v --with-coverage pele

Maybe that is a result of what you are saying?

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

kjs73 commented 8 years ago

C++ part looks good to me. I think the requirements for powers are different for InversePower and Stillinger. Here I think it is better not to have meta powers for flexibility. The computation of large integer powers should be reasonably fast here. We could still change the meta powers to use the algorithm used here, I have played around with that: https://github.com/kjs73/pele/commit/116473bcb4485b50f43d03f45862c696c14d3fa3

js850 commented 8 years ago

OK, seems alright now.

kjs73 commented 8 years ago

Now the tests should work fine.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.09%) to 89.308% when pulling cb6c3246f92b8480faedd845298bbdb9f256f05f on smcantab:invstill_poly into d16aa71fffc9308e8666ace4dfb565a233f5520f on pele-python:master.