7
\$\begingroup\$

My code takes 143.023 seconds for extracting nodes from a road network of city like Göteborg in Sweden. Please check it out if I can optimize it.

# -*- coding: utf-8 -*- 
import arcpy
from Class import loading_layer
import time

def node_extraction(network_input):
    time_start=time.time()
    nodes_list = []
    list_of_existing_nodes = []
    for feature in network_input:
        coordinates_list = list()

        feature_startX = feature["Shape"].firstPoint.X
        feature_startY = feature["Shape"].firstPoint.Y
        start_list =[]
        start_list.append(feature_startX)
        start_list.append(feature_startY)


        feature_endX = feature["Shape"].lastPoint.X
        feature_endY = feature["Shape"].lastPoint.Y
        end_list =[]
        end_list.append(feature_endX)
        end_list.append(feature_endY)


        if nodes_list.__len__() is not 0:
            if start_list in list_of_existing_nodes:
                point_index=list_of_existing_nodes.index(start_list)
                nodes_list[point_index][1].append(feature["FID"])
            else:
                nodes_list.append((feature["Shape"].firstPoint, [feature["FID"]]))
                list_of_existing_nodes.append(start_list)
            if end_list in list_of_existing_nodes:
                point_index=list_of_existing_nodes.index(end_list)
                nodes_list[point_index][1].append(feature["FID"])
            else:
                nodes_list.append((feature["Shape"].lastPoint, [feature["FID"]]))
                list_of_existing_nodes.append(end_list)
        else:
            nodes_list.append((feature["Shape"].firstPoint, [feature["FID"]]))
            nodes_list.append((feature["Shape"].lastPoint, [feature["FID"]]))
            list_of_existing_nodes.append(start_list)
            list_of_existing_nodes.append(end_list)            
    time_end=time.time()
    print time_end-time_start



layer1 = loading_layer("D:/path/to/your/road/data.shp")
layer1.create_new_dict()
node_extraction(layer1.new_features_dict)

Load_layer class is as follows:

import arcpy
from arcpy.sa import *
from arcpy import env
import re
from _collections import defaultdict

class loading_layer(object):


    def __init__(self, input_data):
        self.features_dict = defaultdict(list)
        self.new_features_dict = []
        self.Address = input_data
        self.file_name = re.search(r'\w+.shp',input_data).group()
        self.coord_sys = arcpy.Describe(self.Address).spatialReference
        self.coord_sys_name = arcpy.Describe(self.Address).spatialReference.name

        self.fieldList = [f.name for f in arcpy.ListFields(self.Address)]
        if "matched_to".upper() not in self.fieldList:
            arcpy.AddField_management(input_data, u"Matched_to", "TEXT")
            self.fieldList.append(u"Matched_to")
            print "field is added"
        print str(self.fieldList)

    def iterate_features(self):
        feature_set = arcpy.SearchCursor(self.Address)
        for feature in feature_set:
            yield feature

    def create_new_dict(self):
        iterator = self.iterate_features()
        for item in iterator:
            feature = dict()
            for f in self.fieldList:
                feature[f] = item.getValue(f)
            self.new_features_dict.append(feature)
\$\endgroup\$
  • \$\begingroup\$ Welcome to CodeReview.SE! Would you be able to provide you info so that we can test your script (sample input, command to run, etc) ? \$\endgroup\$ – SylvainD Jun 30 '14 at 14:53
  • \$\begingroup\$ @Josay: Thanks, please let me know how I can provide you with a sample data. \$\endgroup\$ – msc87 Jun 30 '14 at 15:13
  • \$\begingroup\$ Can we get a self-sufficient sample from D:/path/to/your/road/data.shp for instance ? \$\endgroup\$ – SylvainD Jun 30 '14 at 15:15
  • \$\begingroup\$ yes, I suppose so. \$\endgroup\$ – msc87 Jun 30 '14 at 15:17
4
\$\begingroup\$

Performance:

if start_list in list_of_existing_nodes:
    point_index=list_of_existing_nodes.index(start_list)
    nodes_list[point_index][1].append(feature["FID"])

This is not an efficient way to update an existing element. The fact that you want to lookup a node based on it's coordinates means that you are using the wrong data structure. A dictionary is explicitly designed to make key-based lookups fast, making this type of manipulation easy. This way you can have the point object as the key and the value would be the data object that might have a FID value.

What you have now will cause each look to iterate over the entire list to see if the node exists. Then when it does, iterate again to find the index. As the number of nodes increase, the lookup will take longer and longer. Using a dictionary will take a constant amount of time independent of how many nodes are in the collection.


def iterate_features(self):
    feature_set = arcpy.SearchCursor(self.Address)
    for feature in feature_set:
        yield feature

def create_new_dict(self):
    iterator = self.iterate_features()
    for item in iterator:
        # ...

This is not providing any benefit. If SearchCursor creates a list, then the list has already been allocated containing all of the values. This means your generator method is just adding additional iteration overhead when switching back and forth between contexts. If SearchCursor is already a generator (which I suspect is the case based on the name cursor), you are just wrapping it without adding any additional values.


Other Points:

coordinates_list is never used, so remove it. Additionally, this is the only case where you use list() instead of [], you should be consistent and use [].


Don't suffix your variables with _list, making the name plural should be sufficient to convey that the value is a collection.


A list is not a great data structure for when each index has specific meaning. If you just need a simple data store object, namedtuple is great for that.

The data you are pulling values from has X and Y attributes. Is there a reason you don't just use that object directly?

Creation of the start and stop point can be cleaned up a lot, even if you stick with lists.

shape = feature["Shape"]
start = [shape.firstPoint.X, shape.firstPoint.Y]
end = [shape.lastPoint.X, shape.lastPoint.Y]

if nodes_list.__len__() is not 0:

should be

if len(nodes_list) == 0:

The built-in function is much cleaner and doesn't depend in the internal implementation.

is is an identity comparison and == is an equivalence comparison. The fact that an identity comparison works is an implementation detail and is not guaranteed to always be true.

Edit: Since we are dealing with a list, this check is also equivalent to

if not nodes_list:

This is the recommended style by the style guide.


You are performing the same operation for the start and end points. you can create a single function that does the operation instead of repeating the code. When you change to using a dictionary, write the generic function first, then you can call it once for each point.


The naming convention for instance variables is lower_case. Address will make other people think it is a class.

\$\endgroup\$
  • 1
    \$\begingroup\$ About your if len(nodes_list) == 0:, quoting PEP 8 : "For sequences, (strings, lists, tuples), use the fact that empty sequences are false.". In your case, you'd better write if not nodes_list. \$\endgroup\$ – SylvainD Jun 30 '14 at 15:54
  • \$\begingroup\$ I try to answer each part separately: (using the dictionary) I did use it in the beginning. it has a problem or at least I dont know how to fix it. Everytime I add new a node, the new item with a point object as a key and FID as a value is not added to the end of the dictionary so I can't find the correct point if it already exists in the dictionary. \$\endgroup\$ – msc87 Jun 30 '14 at 15:59
  • \$\begingroup\$ @Josay: Added. This is one point where I think python contradicts itself. "Explicit is better than implicit." Here we mean to check for an empty collection, but by using not and truth-iness we can allow other errors through like if the value was None. \$\endgroup\$ – unholysampler Jun 30 '14 at 16:01
  • \$\begingroup\$ when I was using dictionary, for instance, I used: If feature["shape"].firstPoint in Nodes_dict. keys(). But it never found the same point as each point is a different object and only the X and Y values of it are the same. so I started adding each X and Y to a list to be able to compare them. but then the unordered dictionary made me switch to a list. \$\endgroup\$ – msc87 Jun 30 '14 at 16:11
  • \$\begingroup\$ @msc87: When you are working with a dictionary, you generally care about an ordering. There is no concept of "the end of the dictionary". You should always be using the point as the key to find an value of key in d to check if the key is set. If the point object you are getting from the dataset has other attributes contributing to the hash code, then you can use namedtuple to create a Point class, and use instances of that as the key and your data object as the value. \$\endgroup\$ – unholysampler Jun 30 '14 at 16:59

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.