WIP: Cython port of contact map functions #375

kain88-de · 2015-08-02T21:43:07Z

I want to fix #264. The non PBC version is already implemented in Cython and the tests are passing as well. The Cython-implementation is also faster using the 1Q0W protein.

Porting the PBC version I noticed that there are no tests for it. I added a test and the test suite is still passing. But I'm not sure if the test I have added is run. Btw I'm new to nose is there a way to specify a subset of tests that I want to run instead of executing the whole test-suite all the time?

If someone can help me get the test-cases for the PBC-case running I will also port the PBC version.

Here is a short description how I run the test-suite since I couldn't find up-to-date information how to set-up a dev environment or run unit-tests.

For testing I'm using the approach defined in the travis.yml, but I installed all dependencies via pip.
After each change in either the package or testsuite folder I reinstalled them with pip install --upgrade <folder>/

Simple Cython implementation. Instead of the other functions in distances.pyx I have used pure cython for clarity. The loop is converted to pure C except for saving the sparse matrix element.

richardjgowers · 2015-08-03T09:02:41Z

Hey,

This looks like a good start.

To run a subset of tests, from a command line in the directory testsuite/MDAnalysisTests you can do

# run tests from a specific file
nosetests test_analysis.py

# or specify the name of the class to run
nosetests test_analysis.py:TestContactMatrix

More details here:
https://github.com/MDAnalysis/mdanalysis/wiki/UnitTests

This should then let you see the two tests that fail on Travis (scroll to bottom):
https://travis-ci.org/MDAnalysis/mdanalysis/builds/73811170

If you're not sure a test is running, the quickest way to check is to change the reference state (your res) and see if the test then fails.

Let us know if you need any more help

The test are currently only run for the dense arrays.

kain88-de · 2015-08-03T11:00:59Z

Ok I got the new test running and it passes like it should.

The current sparse test is failing though due to an error in the cython code. My next problem is that installing the changes with pip install --upgrade packages/ isn't updating the cython-generated C-module. Is there a special way how I should trigger the cython-compiler?

Sorry for all the questions but this is badly/not documented in the wiki.

richardjgowers · 2015-08-03T11:07:10Z

My workflow is to run setup.py install --user from the package/ directory, I'm not sure if this is the same as your method.

If that doesn't do it, deleting src/numtools/distances.c should force it to regenerate the C module from the pyx you wrote.

Due to a typo I calculated the distance between points on different axes instead of on the same one.

kain88-de · 2015-08-03T11:29:00Z

Yeah got it working now. Installing cython in the virtual-env and removing the c file did it.

I have noticed that the return values between the numpy and sparse matrix are inconsistent. For the numpy array the diagonal of the contact-matrix is true. But for sparse matrices the diagonal is set to false.

Personally I would prefer if if the diagonal elements were always true independent of the chosen return-type.

richardjgowers · 2015-08-03T12:03:57Z

It's not documented either way, so we can go with the diagonal being True (update the docstring and tests) unless anyone objects.

The only thing in the package that uses contact_matrix current is analysis/leaflet, and I think all this does is build a networkx graph from the contacts, I'm not sure how the diagonal would affect this? @orbeckst

To do the PBC versions, it might be a good idea to import minimum_image from calc_distances.h and use that. I think you'll need to change your x y z into a single array for that function.

Simple port. I'm currently not using any of the helper functions defined in calc_distances.h

kain88-de · 2015-08-03T14:02:40Z

Btw is the progressbar feature still required? Without it it should be straight forward to add these functions to calc_distances.h which will make adding openmp support later easier.

richardjgowers · 2015-08-03T15:57:23Z

I wasn't too sure how easy it'd be to fill a np sparse matrix in C though?

orbeckst · 2015-08-03T23:39:01Z

The progressbar feature is not really necessary inside the cython code; feel free to get rid of it, especially if it makes the code cleaner.

Only calculate the distances for the upper triangle contact matrix. The other entries are already determined through the symmetry. This also sets the contacts along the diagonal automatically to true.This should save about half of the operations.

This makes the behavior consistent regardless of the chosen return type.

The refactored tests don't depend on any external test-data anymore. This has two nice effects. Giving the coordinates explicitely makes it easy to see where a contact is and the tests now run over a 100 times faster. Plus everything looks a lot more cleaned up.

I reordered some variable assigment and calculate the squares directely instead of using `pow`. This can lead to performance improvements on some platforms.

kain88-de · 2015-08-04T07:39:20Z

I added some changes

removed progress bar
only calculate upper triangle matrix and use symmetry
refactored test to be nicer to read and easier to understand.
set diagonal elements to True also for sparse matrices

richardjgowers · 2015-08-04T08:01:30Z

Ok this is looking good, nice catch with the reflection.

There's actually a minor bug in the code you've translated, (was present before so not your fault), I've left a line comment, if you can fix that up then we're good to go.

Other minor things to do:

add yourself to AUTHORS
add a line to CHANGELOG detailing the fix to the diagonal return type

richardjgowers · 2015-08-04T08:06:11Z

package/src/numtools/distances.pyx

+            y = xyz[i, 1] - xyz[j, 1]
+            z = xyz[i, 2] - xyz[j, 2]
+
+            if abs(x) > box_half[0]:


This will only add/subtract a single box_view from each dimension, but it needs to do it multiple times where necessary.

So if box_half = 10 then x = 1 = 11 == 21 = 31, and x = 1 = -9 = -19

You can either import the minimum_image function from calc_distances.h which does this, or inline that into here.

Good catch. I always assume that particles are in one of the neighboring images and then wonder that happened when this is not the case. I will first add this to the unit-tests and then fix it.

Two points with PBC can be further apart then one image. The test now checks that as well. The numpy pbc function already passes this. The sparse version still assumes that a point is maximally in a neighboring image.

Now also points that are several images apart are handled correctly. All tests are passing.

kain88-de · 2015-08-04T09:36:48Z

PBC are handled correctly with minium-image now.

kain88-de · 2015-08-04T09:55:30Z

Btw should I regenerate the documentation or is this done automatically with a git-hook when this PR is merged?

orbeckst · 2015-08-04T17:13:30Z

Docs are not generated automatically (yet). There is a page in the wiki outlining the procedure but in the end someone with write access has to merge the develop branch with docs into the gh-pages branch.

Oliver Beckstein
email: orbeckst@gmail.com
skype: orbeckst

Am Aug 4, 2015 um 2:55 schrieb kain88-de notifications@github.com:

Btw should I regenerate the documentation or is this done automatically with a git-hook when this PR is merged?

—
Reply to this email directly or view it on GitHub.

kain88-de · 2015-08-04T17:50:02Z

Docs are updated now. Sphinx has rebuild almost all the doc pages but I have only included the one affected by this PR.

orbeckst · 2015-08-04T18:04:39Z

I had a quick look and it seems fine but I let @richardjgowers do the final approval and merge.

Thanks @kain88-de !

WIP: Cython port of contact map functions

Port contact_matrix_no_pbc to cython

841be3d

Simple Cython implementation. Instead of the other functions in distances.pyx I have used pure cython for clarity. The loop is converted to pure C except for saving the sparse matrix element.

Add test for contac-matrix with pbc

af65ed5

The test are currently only run for the dense arrays.

kain88-de force-pushed the cython-contact-map branch from 1886597 to af65ed5 Compare August 3, 2015 10:45

Use correct axis in contact_matrix_no_pbc

949ccea

Due to a typo I calculated the distance between points on different axes instead of on the same one.

kain88-de added 2 commits August 3, 2015 15:15

Add sparse PBC test for contact-matrix

9fac3a1

Port contact_matrix_pbc to cython

2b92835

Simple port. I'm currently not using any of the helper functions defined in calc_distances.h

kain88-de force-pushed the cython-contact-map branch from 8f88666 to 2b92835 Compare August 3, 2015 13:26

kain88-de added 5 commits August 4, 2015 07:26

Remove progressbar for sparse contact-matrix

cf7a0d4

Diagonal elements in contact-matrix are always true

b106b6a

This makes the behavior consistent regardless of the chosen return type.

Clean up contact-matrix cython code.

ac0acb0

I reordered some variable assigment and calculate the squares directely instead of using `pow`. This can lead to performance improvements on some platforms.

kain88-de force-pushed the cython-contact-map branch from b95dadb to ac0acb0 Compare August 4, 2015 07:36

richardjgowers added this to the 0.11 milestone Aug 4, 2015

richardjgowers reviewed Aug 4, 2015
View reviewed changes

kain88-de added 3 commits August 4, 2015 11:16

Add test for general pbc conditions in contact-matrix

ebfd790

Two points with PBC can be further apart then one image. The test now checks that as well. The numpy pbc function already passes this. The sparse version still assumes that a point is maximally in a neighboring image.

Handle PBC correctly for contact-matrix sparse

484e0a6

Now also points that are several images apart are handled correctly. All tests are passing.

Add author/changelog information

f74f0b8

Update docstring of contact_matrix

3d2c416

kain88-de added 2 commits August 4, 2015 19:48

Update CHANGELOG

4c34d7e

update doc file for analysis.distances

14efd69

orbeckst assigned richardjgowers Aug 4, 2015

orbeckst added the Component-Analysis label Aug 4, 2015

richardjgowers added a commit that referenced this pull request Aug 5, 2015

Merge pull request #375 from kain88-de/cython-contact-map

d043dd1

WIP: Cython port of contact map functions

richardjgowers merged commit d043dd1 into MDAnalysis:develop Aug 5, 2015

kain88-de deleted the cython-contact-map branch September 12, 2015 17:51

richardjgowers mentioned this pull request Nov 30, 2021

contact_matrix_pbc only handles orthgonal boxes #3471

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Cython port of contact map functions #375

WIP: Cython port of contact map functions #375

kain88-de commented Aug 2, 2015

richardjgowers commented Aug 3, 2015

kain88-de commented Aug 3, 2015

richardjgowers commented Aug 3, 2015

kain88-de commented Aug 3, 2015

richardjgowers commented Aug 3, 2015

kain88-de commented Aug 3, 2015

richardjgowers commented Aug 3, 2015

orbeckst commented Aug 3, 2015

kain88-de commented Aug 4, 2015

richardjgowers commented Aug 4, 2015

richardjgowers Aug 4, 2015

kain88-de Aug 4, 2015

kain88-de commented Aug 4, 2015

kain88-de commented Aug 4, 2015

orbeckst commented Aug 4, 2015

kain88-de commented Aug 4, 2015

orbeckst commented Aug 4, 2015

WIP: Cython port of contact map functions #375

WIP: Cython port of contact map functions #375

Conversation

kain88-de commented Aug 2, 2015

richardjgowers commented Aug 3, 2015

kain88-de commented Aug 3, 2015

richardjgowers commented Aug 3, 2015

kain88-de commented Aug 3, 2015

richardjgowers commented Aug 3, 2015

kain88-de commented Aug 3, 2015

richardjgowers commented Aug 3, 2015

orbeckst commented Aug 3, 2015

kain88-de commented Aug 4, 2015

richardjgowers commented Aug 4, 2015

richardjgowers Aug 4, 2015

Choose a reason for hiding this comment

kain88-de Aug 4, 2015

Choose a reason for hiding this comment

kain88-de commented Aug 4, 2015

kain88-de commented Aug 4, 2015

orbeckst commented Aug 4, 2015

kain88-de commented Aug 4, 2015

orbeckst commented Aug 4, 2015