0
\$\begingroup\$

While working with someone else's code I've isolated a time-killer in the (very large) script. Basically this program runs through a list of thousands and thousands of objects to find matches to an input (or many inputs) object. It works well when the number is below 50 but any input higher than that and it begins to take a looooong time. Is this just an issue with the database I'm running it against (30,000 things)? Or is this code written inefficiently?

for i in range(BestX):
       Tin=model[BestFitsModel[0][i]][2]
       Tau2=np.append(Tau2,model[BestFitsModel[0][i]][6])
       Dir=model[BestFitsModel[0][i]][0]
       Dir=Dir.replace('inp','out',1) ##open up correct .out file
       T=open(Dir,"rb")
       hold=''
       Tau=[]
       for line in T:
        if not line.strip():
         continue
        else:
         hold=line.split()
         Tau.append(hold)
       Tauline=0
       Taunumber=0
       print 'Loop'
       for j in range(5,len(Tau)-50):
        if 'RESULTS:' in Tau[j-5][0]:
         for k in range(j,j+50):
          if (Tau[k][1] == model[BestFitsModel[0][i]][6]):
           Tauline=i #line number of the tau
           Taunumber=k-(j) #Tau 1, 2, 3, etc.

   Dir=Dir.replace('out','stb',1)#.stb file
   Dir2=np.append(Dir2,Dir)
   F=open(Dir, "rb")
   hold='' 
   Spectrum=[]
   for line in F:
    hold=line.split()
    Spectrum.append(hold)

BestX is a list of 30,000 objects (stars).

I'm not looking for an answer to this problem, I'm looking for code-critique...there has to be a way to write this so that it performs faster, right?

Thanks.

EDIT: Reading through the Python Performance Tips I've noticed one little snag (so far). Starting with the line Tau=[] and continuing for the next six lines. I can shorten it up by

Tau=[line.split() for line in T]

I'm gonna keep looking. Hopefully I'm on the right track.

\$\endgroup\$
  • \$\begingroup\$ Can you clarify a bit further what this is supposed to do and what 'number' you are referring to exactly? The number of input objects? Are the stars in BestX the input objects? Also, with >50 objects, does it work slowly but correctly, or does it not work at all? \$\endgroup\$ – Stuart Jul 12 '13 at 21:34
  • \$\begingroup\$ Yeah, sorry for being 'nebulous' (drumroll). The BestX file is the file I'm matching my input stars to. If I put in a star it might have 5 matches in that file (I called it a database), or it could have 1000+, when it has a high number of matches it begins to get bogged down dramatically. What this does after it finds the matches -moving down the script- is computes some stellar values of some characteristic or another to give me an averaged value over all the matches for whatever property I'm searching for. I ran a lot of checks and isolated it to this exact region of the code. Need more? \$\endgroup\$ – Matt Jul 12 '13 at 21:40
  • \$\begingroup\$ @Stuart (more) these files I'm searching through are easily less than a gigabyte. That's why I think it's the code structure. \$\endgroup\$ – Matt Jul 12 '13 at 21:48
  • \$\begingroup\$ as a first improvement try to give the variables more descriptive names. This should make it easier to understand what's going on. \$\endgroup\$ – Stuart Jul 12 '13 at 22:04
  • \$\begingroup\$ Your edit improving the code actually changes what it does. What you need is Tau = [line.split() for line in T if line.strip()], i.e. only include the line if it is contains something other than spaces. \$\endgroup\$ – Stuart Jul 12 '13 at 22:07
2
\$\begingroup\$

Some guesswork here but I think what you want is actually something like this:

def match_something(file, match):
    """ Finds a line in file starting with 'RESULTS:' then checks the following 50
    lines for match; if it fails returns None, otherwise it returns
    how many lines down from the 'RESULTS:' heading the match is found
    """
    in_results_section = False
    results_counter = 0
    for line in file:
        if line.strip():
            if results_header_found:
                if line.split()[1] == match:
                    return results_counter  # found a match
                else:
                    results_counter += 1
                    if results_counter > 50:
                        return None  
# I'm assuming in the above that there can only be one results section in each file

            elif line.startswith('RESULTS:'):
                results_header_found = True

matched_results = []        
for i in range(BestX):
    thing = model[BestFitsModel[0][i]]

    # make variables with descriptive names 
    directory, input_thing, match = thing[0], thing[2], thing[6]
    list_of_matches = np.append(list_of_matches, match)
    with open(directory.replace('inp', 'out', 1), 'rb') as file:
        m = match_something(file, match)
        if m is not None:
            matched_results.apppend((i, m))
    stb_directory = directory.replace('inp', 'stb', 1), 'rb')
    np.append(directory_list, stb_directory)
    with open(stb_directory, 'rb') as file:
        spectrum = [line.split() for line in file]    

This uses a function match_something to go through the file. As soon as a result is found, the function exits (return i, results_counter) and then the main loop adds this result to a list of matched results. If no result is found within 50 lines of the line starting 'RESULTS:' then the function returns None, and the loop moves to the next line of BestFitsModel[0]. (If, on the other hand, there can be more than one results section within each file, then you need to alter this accordingly; it should carry on checking the rest of the file rather than returning, which will be slower.) I hope it should be fairly clear what is happening in this code; it should avoid looping through anything unnecessarily (assuming I've guessed correctly what it's supposed to do)

In the original code, it seems that the files are not closed (T.close()), which could cause problems later on in the programme. Using the with notation avoids the need for this in the above code.

\$\endgroup\$
1
\$\begingroup\$

You are opening the file(never closing it) and then using a for loop. Try using the with statement and list comprehension to open, close and get your list. This goes for both Tau and Spectrum.

np and Dir are declared before the loop starts so you can declare np_appends =np.append and dir_replace = Dir.replace. For the reason see the part "Avoiding the dots..." in Python performance Tips.

You can try to use xrange instead of range if you are on Python 2. Using generators might be better instead of creating the whole list. You'll have to time this.

This isn't much but might be helpful.

\$\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.