2
\$\begingroup\$

I've written a script to look for coordinates in KMZ files. I've tried to speed execution up using the ProcessPoolExecutor. I'm quite new to Python, so any recommendations are more than welcome.

#!/usr/bin/env python

from zipfile import ZipFile
from lxml import html
import os
import concurrent.futures

def process_file(filename):
    results=[]
    try:
        saved_file=False;
        kmz = ZipFile(filename, 'r')
        for kml_name in kmz.namelist():
            if 'doc.kml' in kml_name:
                continue
            kml = kmz.open(kml_name, 'r').read()
            doc = html.fromstring(kml)
            for pm in doc.cssselect('Document Placemark'):
                tmp = pm.cssselect('track')
                if len(tmp):
                    # Track Placemark
                    tmp = tmp[0]  # always one element by definition
                    for desc in tmp.iterdescendants():
                        content = desc.text_content()
                        if desc.tag == 'coord':
                            lon = float(content.split()[0])
                            lat = float(content.split()[1])
                            search_lon = -47
                            search_lat = 47
                            if (abs(lat - search_lat) <= 1 and abs(lon - search_lon) <= 1):
                                if not saved_file:
                                    results.append('\nFile: ' + filename + '\n')
                                    saved_file=True
                                results.append(content + '\n')
    except:
        pass
    return results

def main():
    # Search all files
    kmz_files = []
    for root, subdirs, files in os.walk('raw/L1B_Catalogue'):
        for file in files:
            if '.kmz' in file:
                filename = os.path.join(root,file)
                kmz_files.append(filename)

    # Parallel execution
    f=open('kmz_search_output.txt', 'wt')
    count = 0
    with concurrent.futures.ProcessPoolExecutor(max_workers=8) as executor:
        for results in executor.map(process_file, kmz_files, chunksize=10):
            print("{} / {}".format(count,len(kmz_files)))
            count = count + 1
            for line in results:
                f.write(line);
            pass
    f.close()
    print('Done')

if __name__ == '__main__':
    main()
\$\endgroup\$
2
\$\begingroup\$

1. Review

  1. There are no docstrings. What does process_file do? What does it return?

  2. The name process_file is vague. It's better to use specific names, for example kmz_coordinates.

  3. The saved_file logic should be unnecessary, because the caller already knows which filename they passed to process_file. So you could write something like:

    results = executor.map(kmz_coordinates, kmz_files, chunksize=10)
    for filename, coords in zip(kmz_files, results):
        print("{} / {}".format(count,len(kmz_files)))
        count = count + 1
        f.write(f"File {filename}:\n")
        for line in coords:
            f.write(line);
    

    using zip to match up the filenames with the results, and remove the saved_file logic from kmz_coordinates.

  4. Similarly, the count logic is unnecessary, because you could use enumerate to generate the counts:

    results = executor.map(kmz_coordinates, kmz_files, chunksize=10)
    for count, (filename, coords) in enumerate(zip(kmz_files, results)):
        print("{} / {}".format(count, len(kmz_files)))
        f.write(f"File {filename}:\n")
        f.writelines(coords)
    
  5. The use of try: ... except: pass to suppress exceptions is a bad idea. There are two reasons for this. First, a bare except: catches all exceptions, including KeyboardInterrupt, which might make it difficult to stop the program by typing control-C. Second, the exceptions might be due to bugs in your program, but by suppressing them you make the bugs difficult to spot.

    If you really do have a good reason for suppressing exceptions, then you should localize the try: ... except as tightly as you can around the line of code that might raise the exception, and you should pick the appropriate exception classs. For example, if the problem you are worrying about is that lxml.html.fromstring will raise lxml.etree.ParserError, write something like:

    try:
        doc = html.fromstring(kml)
    except lxml.etree.ParserError:
        continue # skip this file
    
  6. When opening a file, it is a good idea to use with. For example, instead of:

    f=open('kmz_search_output.txt', 'wt')
    # ... code using f ...
    f.close()
    

    write:

    with open('kmz_search_output.txt', 'w') as f:
        # ... code using f ...
    

    This saves a line of code (no need to close the file explicitly), and ensures that the file gets closed in a timely manner even if an exception occurs inside the ... code using f ....

  7. The content of a KML file XML, not HTML, so shouldn't you be using lxml.etree instead of lxml.html?

  8. The code tries to parse every file in the KMZ as if it is a KML file:

    for kml_name in kmz.namelist():
        if 'doc.kml' in kml_name:
            continue
        kml = kmz.open(kml_name, 'r').read()
        doc = html.fromstring(kml)
    

    But a KMZ can include other kinds of data, for example images and audio, so I would expect some check on the filename, for example to see if has the .kml extension:

    for kml_name in kmz.namelist():
        if not kml_name.endswith('.kml') or 'doc.kml' in kml_name:
            continue
    

    If you did this then maybe you could omit the try: ... except: completely.

  9. The name tmp is vague. Better to be as specific as possible, for example track.

  10. This section of code loops over the first track in each placemark:

    for pm in doc.cssselect('Document Placemark'):
        tmp = pm.cssselect('track')
        if len(tmp):
            # Track Placemark
            tmp = tmp[0]  # always one element by definition
    

    But if there is only one track in each placemark, then it looks to me as though this can be as a single loop:

    for track in doc.cssselect('Document Placemark track'):
    
  11. Similarly, the code then loops through the descendents of the track looking for coord elements. So you could combine that into the same loop:

    for coord in doc.cssselect('Document Placemark track coord'):
    
  12. Instead of splitting the content of the coord element twice:

    lon = float(content.split()[0])
    lat = float(content.split()[1])
    

    Split it just once and use map and tuple assignment:

    lon, lat = map(float, content.split())
    
  13. The search coordinates are always the same, so they should be assigned outside the loop. It would make sense for them to be keyword arguments to the kml_track_coordinates, then you could use the function to search for coordinates of your choice.

  14. Similarly, the tolerance of 1 degree is arbitrary and should also be a keyword argument to the function.

2. Revised code

This is untested, so probably contains some mistakes.

def kmz_coordinates(filename, search_lon=-47, search_lat=47, tolerance=1):
    """Return list of track coordinates found in a KMZ file that are
    within tolerance degrees of (search_lon, search_lat).

    """
    coords = []
    with ZipFile(filename, 'r') as kmz:
        for kml_name in kmz.namelist():
            if not kml_name.endswith('.kml') or 'doc.kml' in kml_name:
                continue
            with kmz.open(kml_name, 'r') as kml:
                doc = html.fromstring(kml.read())
            for coord in doc.cssselect('Document Placemark track coord'):
                content = coord.text_content()
                lon, lat = map(float, content.split())
                if (abs(lat - search_lat) <= tolerance
                    and abs(lon - search_lon)) <= tolerance):
                    coords.append(content + '\n')
    return coords

def kmz_search_directory(directory, output_filename='kmz_search_output.txt'):
    """Search directory for KMZ files, find matching coordinates within
    the files, and write the results to output_filename.

    """
    kmz_files = []
    for root, _, files in os.walk(directory):
        for filename in files:
            if filename.endswith('.kmz'):
                kmz_files.append(os.path.join(root, filename))

    with open(output_filename, 'w') as f:
        with concurrent.futures.ProcessPoolExecutor(max_workers=8) as executor:
            results = executor.map(kmz_coordinates, kmz_files, chunksize=10)
            for count, (filename, coords) in enumerate(zip(kmz_files, results)):
                print("{} / {}".format(count, len(kmz_files)))
                f.writelines(coords)

    print('Done')
\$\endgroup\$
  • \$\begingroup\$ Thank you so much! Really good tips and information. Point 5 explains why it was so difficult for me to stop the program hehe \$\endgroup\$ – Blasco Sep 13 '18 at 9:34
  • \$\begingroup\$ One question about the 'with' statement. Do I need to have the rest of the code indented inside the with? In your example code the with statement is followed by a for statement with the same indent level instead of inside it, is this fine? or a mistake? \$\endgroup\$ – Blasco Sep 13 '18 at 9:35
  • \$\begingroup\$ The body of a with statement must be indented. But in my revised code you'll see that after the line doc = html.fromstring(kml.read()) we are done with the file kml and don't need it to be open any more. So it's fine to finish the with statement here, allowing kml to be closed. \$\endgroup\$ – Gareth Rees Sep 13 '18 at 9:40
  • \$\begingroup\$ Ahhh, because you only need to read it. Now I get it. Great! \$\endgroup\$ – Blasco Sep 13 '18 at 9:41

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.