5
\$\begingroup\$

I have a program that reads in two xyz-files (molecular structures) and compares them by an intramolecular distance measure (dRMSD, Fig. 22). A friend told me that my program structure is bad, and as he is a programmer while I am not, I guess he is right. But as he has no time to help me much, I thought maybe some one of you could provide me with helpful answers.

What my program does right now:

  1. read in molecule 1 (p) and molecule 2 (q)
  2. compare them by dRMSD $$\sqrt{\frac{1}{N(N+1)}\sum_{i<j}\left(p_{ij}-q_{ij}\right)^2}$$ with \$N\$ as the total number of atoms and \$p_{ij}\$ and \$q_{ij}\$ as the euclidean distance between atom i and atom j in each molecule
  3. delete the file of molecule 1 if dRMSD is below a certain threshold

#!/usr/bin/env python

import sys
import math
import os
import numpy as np
from scipy import linalg

def ecd(vec1, vec2):
    "euclidean distance"
    a = np.array(vec1)
    b = np.array(vec2)
    return np.linalg.norm(a-b)

def drmsd(p, q):
    "intra-molecular distance measure"
    a = np.array(p)
    b = np.array(q)
    k = len(p)*(len(p)+1.0)
    total = 0.0
    for i in range(len(p)):
        for j in range(i+1,len(p)):
            total += (ecd(a[i],a[j])-ecd(b[i],b[j]))**2
    return np.sqrt(total/k)

nconf = int(sys.argv[1])

for i in range(1,nconf+1):
    for j in range(i+1,nconf+1):
        fname1 = "h" + str(i) + ".xyz"
        xyz = open(fname1)
        n_atoms = int(xyz.readline())
        coord1 = np.zeros(shape=(n_atoms,3))
        title = xyz.readline()

        cnt = 0
        for line in xyz:
            atom,x,y,z = line.split()
            coord1[cnt] = [float(x), float(y), float(z)]
            cnt += 1
        xyz.close()

        fname2 = "h" + str(j) + ".xyz"
        xyz = open(fname2)
        n_atoms = int(xyz.readline())
        coord2 = np.zeros(shape=(n_atoms,3))
        title = xyz.readline()

        cnt = 0
        for line in xyz:
            atom,x,y,z = line.split()
            coord2[cnt] = [float(x), float(y), float(z)]
            cnt += 1
        xyz.close()

        dist = drmsd(coord1,coord2)
        if dist < 0.0001:
            print i, j, dist, " ... ", fname1, "deleted"
            #os.remove(fname1)
            break
\$\endgroup\$
5
\$\begingroup\$

I probably get what your friend meant when they said that your code wasn't particularly well-structured: your program has a main double for loop and ought to be broken into more small functions with names that could be understood at first glance (at least for somebody fluent in the application domain). Putting that aside, there are many small things that could be improved:

  • Iteration over numpy arrays is slow. While numpy arrays can help to write much faster code than with regular Python lists, iterating over them is slower than iterating over a regular Python list. The trick to numpy arrays is to use functions designed for them when possible, time, convert them to Python lists when you really need to iterate over them, time again, read the documentation to find the most useful functions from numpy and scipy, then time again and in the end you will know which solution is the fastest. Agreed, that's not the nicest thing in the world to do.

    I didn't time your code, but I guess that it would be more performant if you used regular Python lists while you read files, still use Python lists in drmsd since you only iterate over the arrays, but use numpy arrays in ecd since you have to use np.linalg.norm.

  • Instead of manually calling close on file instances, use with instead:

    with open('some_file') as f:
        # do things with f
    # Here, f is automagically closed
    

    The file will be closed when you leave the scope of the with. That ensures that your file is always closed, even if an exception is thrown while you are doing things with it. It prevents resource leaks without having to use cumbersome try/catch/finally clauses.

  • Instead of putting your main code directly in the file, try to do this:

    if __name__ == '__main__':
        # Here your main code
    

    It will ensure that your main is only called if the file containing it is the one launched by the Python interpreter. That, you can reuse its functions from other modules without risking to launch the main code when you don't want to. This feature is generally used to provide example code in modules.

  • You seem to be using Python 2.7. If this is the case, then you can turn your range into xrange. While range produces a full list, xrange only produces a lightweight generator object that will produce a return a new integer at every iteration. It is much more memory efficient than using range for the purpose of iterating.

\$\endgroup\$
2
\$\begingroup\$

Below are some changes I would make to your code.

Starting off with drmsd I moved euclidean distance inside of this function to reduce function call overhead. Also it was unnecessary to cast p and q to np.array.

The other major change is that I created a function (get_coord) to load your files into a coordinate vector. Instead of repeating a block of code twice, just make a function out of it.

Edit:

As another has mentioned it is best to replicate the if __name__ = '__main__': pattern. The benefit being that you can either execute the entire script or through the use of modules call your functions separately.

Hope this helps.

def drmsd(p, q):
    len_p = len(p)
    len_q = len(q)
    k = len_p * (len_p + 1.0)
    total = 0.0
    for i in xrange(len_p):
        for j in xrange(i + 1, len_p):
            pij = np.linalg.norm(p[i] - p[j])
            qij = np.linalg.norm(q[i] - q[j])
            diff = pij - qij
            total += diff * diff
    return np.sqrt(total / k)

def get_coord(file_id):
    with open("h" + str(file_id) + ".xyz") as xyz:
        n_atoms = int(xyz.readline())
        coord = np.zeros(shape=(n_atoms,3))
        title = xyz.readline()

        cnt = 0
        for line in xyz:
            atom, x, y, z = line.split()
            coord[cnt] = [float(x), float(y), float(z)]
            cnt += 1

        return coord

def intra_molecular_distance_measure(nconf):
    for i in range(1,nconf+1):
        for j in range(i+1,nconf+1):
            coord1 = get_coord(i)
            coord2 = get_coord(j)
            dist = drmsd(coord1,coord2)
            # the rest of your code

if __name__ == '__main__':
    nconf = int(sys.argv[1])
    intra_molecular_distance_measure(nconf)
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.