7
\$\begingroup\$

Long time reader of Code Review with my first question. I'm self-taught and suspect this code can be improved. The project is really important to me and the team and I could really use the help.

I'm using this as part of a larger script run through the terminal which takes a GeoDataFrame and adds the GPS coordinates as a Point per the specification, and then outputs a GDF again. (I then export it as a geojson file.) So far it's running through ~3500 rows in about 2 hours - which is fine - but I don't think I'm using many coding best practices. I'd like to make it as robust as possible because we have some datasets to run through that are +15000 rows. Does anyone have any feedback on this script?

import pandas as pd
import geopandas as gpd
from shapely.geometry import Point
import geopy as gpy
from geopy.geocoders import GoogleV3
from geopy.extra.rate_limiter import RateLimiter
from geopy.exc import GeocoderTimedOut
from geopy.location import Location

def addressParsing(gdf_obj, delayseconds):
    """
    This takes a whole GeoDataFrame and adds the Geo Coords from the standard address
    before returning the udpated Geodataframe
    """
    # Returned class obj if None
    site = Location("0", (0.0, 0.0, 0.0))

    def do_geocode(address):
        try:
            return geocode_with_delay(address)
        except GeocoderTimedOut:
            return geocode_with_delay(address)

    print(f"starting parser: {gdf_obj.shape}, estimated time: {round(delayseconds * gdf_obj.shape[0] / 60, 2)} min")

    # Initiate geocoder
    geolocator = GoogleV3(api_key=g_api_key)

    # Create a geopy rate limiter class:
    geocode_with_delay = RateLimiter(
        geolocator.geocode,
        error_wait_seconds=delayseconds + 20,
        min_delay_seconds=delayseconds,
        swallow_exceptions=True,
        return_value_on_exception= site
        )

    # Apply the geocoder with delay using the rate limiter:
    gdf_obj['temp'] = gdf_obj['Address'].apply(do_geocode)

    # Get point coordinates from the GeoPy location object on each row, drop z vector data:
    gdf_obj["coords"] = gdf_obj['temp'].apply(lambda loc: tuple(loc.point)[:2] if loc else tuple(site.point)[:2])

    # Create shapely point objects to geometry column:
    gdf_obj["geometry"] = gdf_obj["coords"].apply(Point)

    # Drop intermediate columns
    gdf_obj = gdf_obj.drop(["temp", "coords"], axis=1)

    print("FINISHED - conversion successful - check shape")
    return gdf_obj
\$\endgroup\$
1
  • 2
    \$\begingroup\$ Long time reader of Code Review with my first question - Welcome, that's exciting :) \$\endgroup\$ Commented Mar 16, 2020 at 15:24

2 Answers 2

Reset to default
6
\$\begingroup\$

Nomenclature

Function names should be snake_case per PEP8, i.e. address_parsing.

Type hinting

For function parameters and return values mainly, type hinting will help define your signatures. For example, delayseconds would probably become delay_seconds: float, and gdf_obj: dict if you don't know a lot about the structure of the dictionary. So:

def address_parsing(gdf_obj: dict, delay_seconds: float) -> dict:

Retries:

    try:
        return geocode_with_delay(address)
    except GeocoderTimedOut:
        return geocode_with_delay(address)

Effectively this will ignore up to one GeocoderTimedOut and retry, but if it occurs twice the exception will fall through. A saner way to represent this is:

TRIES = 2
# ...

for retry in range(TRIES):
  try:
    return geocode_with_delay(address)
  except GeocoderTimedOut as e:
    print('Geocoder timed out')
raise e

Temporary variables

The expression

round(delayseconds * gdf_obj.shape[0] / 60, 2)

is complex enough that it should be assigned to a variable. That said, you're better off using Python's actual time-handling features:

from datetime import timedelta
# ...

est_time = timedelta(seconds=delayseconds * gdf_obj.shape[0])
print(f'Estimated time: {est_time}')  # will do pretty formatting
\$\endgroup\$
2
  • 1
    \$\begingroup\$ gdf_object is probably some subclass of a pandas dataframe \$\endgroup\$ Commented Mar 16, 2020 at 16:11
  • \$\begingroup\$ Man I love SE. That is just so helpful as a self-directed learner. \$\endgroup\$ Commented Mar 16, 2020 at 23:10
3
\$\begingroup\$

Nesting function definitions can often be useful, but it's not what you should normally do. Really, everything should be moved outside of the function unless there's a reason (including readability) for it to be inside.

I would also suggest that delayseconds could be an external setting. Or give it a default value. Also, it seems likely that delayseconds is a big part of why the script takes so long!

I'll assume you want delayseconds as a parameter, so I'll use functools to help keep it abstract.

do_geocode could be more descriptively named.

site needs a better name. Also it looks like you're building a whole object and only using part of it, but I may have misunderstood.

Rather than having temporary columns, package your computation up into a single applyable step.

from functools import partial

import pandas as pd
import geopandas as gpd
from shapely.geometry import Point
import geopy as gpy
from geopy.geocoders import GoogleV3
from geopy.extra.rate_limiter import RateLimiter
from geopy.exc import GeocoderTimedOut
from geopy.location import Location


default_coordinates = (0.0, 0.0)

# Initiate geocoder
geolocator = GoogleV3(api_key=g_api_key)

# Create a geopy rate limiter class:
def geocode_with_delay(delayseconds):
    return RateLimiter(
        geolocator.geocode,
        error_wait_seconds=delayseconds + 20,
        min_delay_seconds=delayseconds,
        swallow_exceptions=True,
        return_value_on_exception= site
    )

def point_of_location(loc):
    if loc:
        return Point(tuple(loc.point)[:2])
    else:
        return Point(default_coordinates)

def try_geocode(address, geo_coder):
    try:
        return point_of_location(geo_coder(address))
    except GeocoderTimedOut:
        return point_of_location(geo_coder(address)) # Is there a reason not to just have a default Point()?

def addressParsing(gdf_obj, delayseconds = 0):
    """
    This takes a whole GeoDataFrame and adds the Geo Coords from the standard address
    before returning the udpated Geodataframe
    """

    print(f"starting parser: {gdf_obj.shape}, estimated time: {round(delayseconds * gdf_obj.shape[0] / 60, 2)} min")

    rate_limited = geocode_with_delay(delayseconds)
    geocode_action = partial(try_geocode, geo_coder=rate_limited)

    # Apply the geocoder with delay using the rate limiter:
    gdf_obj['geometry'] = gdf_obj['Address'].apply(geocode_action)

    print("FINISHED - conversion successful - check shape")
    return gdf_obj

I haven't tested this even to see if it runs.

\$\endgroup\$
1
  • 2
    \$\begingroup\$ I haven't attempted to incorporate @Reinderien's advice, which is all good advice and I think compatible with the changes I suggested. \$\endgroup\$ Commented Mar 16, 2020 at 16:14

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.