opengeos / open-buildings

Tools for working with open building datasets
https://opengeos.github.io/open-buildings
Other
119 stars 17 forks source link

Nicer error reporting when geocoder fails #55

Open cholmes opened 7 months ago

cholmes commented 7 months ago

When I request a location with --location and it doesn't have results I get a barfed stack trace:

Traceback (most recent call last):
  File "/opt/homebrew/Caskroom/miniforge/base/envs/qgis/bin/ob", line 8, in <module>
    sys.exit(main())
             ^^^^^^
  File "/opt/homebrew/Caskroom/miniforge/base/envs/qgis/lib/python3.11/site-packages/click/core.py", line 1157, in __call__
    return self.main(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/Caskroom/miniforge/base/envs/qgis/lib/python3.11/site-packages/click/core.py", line 1078, in main
    rv = self.invoke(ctx)
         ^^^^^^^^^^^^^^^^
  File "/opt/homebrew/Caskroom/miniforge/base/envs/qgis/lib/python3.11/site-packages/click/core.py", line 1688, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/Caskroom/miniforge/base/envs/qgis/lib/python3.11/site-packages/click/core.py", line 1434, in invoke
    return ctx.invoke(self.callback, **ctx.params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/Caskroom/miniforge/base/envs/qgis/lib/python3.11/site-packages/click/core.py", line 783, in invoke
    return __callback(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/Caskroom/miniforge/base/envs/qgis/lib/python3.11/site-packages/open_buildings/cli.py", line 84, in get_buildings
    geojson_data = geocode(location)
                   ^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/Caskroom/miniforge/base/envs/qgis/lib/python3.11/site-packages/open_buildings/cli.py", line 40, in geocode
    location = osmnx.geocode_to_gdf(data)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/Caskroom/miniforge/base/envs/qgis/lib/python3.11/site-packages/osmnx/geocoder.py", line 137, in geocode_to_gdf
    gdf = pd.concat([gdf, _geocode_query_to_gdf(q, wr, by_osmid)])
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/Caskroom/miniforge/base/envs/qgis/lib/python3.11/site-packages/osmnx/geocoder.py", line 187, in _geocode_query_to_gdf
    raise InsufficientResponseError(msg)
osmnx._errors.InsufficientResponseError: Nominatim geocoder returned 0 results for query 'adsfsad'

It'd be better to catch the error and just inform users that their location string didn't work - they can get the geojson on their own or try a more common string.

mtravis commented 7 months ago

I tried a simple fix for this

def geocode(data: str):
    try:
        location = ox.geocode_to_gdf(data)
        geom = location.geometry[0]
        geojson = json.loads(json.dumps({"type": "Feature", "geometry": mapping(geom)})) # turn geom tuple into list by (de-)serialising
        return geojson
    except InsufficientResponseError as e: 
         print(f"An error occurred: {e}")

but getting issues with the geojson_to_quadkey function in download_buildings

  File "/usr/local/lib/python3.10/dist-packages/open_buildings-0.10.0-py3.10.egg/open_buildings/download_buildings.py", line 23, in geojson_to_quadkey
    geom = shape(data["geometry"])
TypeError: 'NoneType' object is not subscriptable

@felix-schott can you try reproducing please?

felix-schott commented 7 months ago

Hi @mtravis - not on my laptop at the moment, so can't reproduce but I'm guessing it might be because the programme is not exiting and because the geocoding is unsuccessful, the programme continues and expects a geometry instead of None where it raises the error. try-except is the right approach to suppress the long error message, but we need to exit the programme immediately afterwards. A similar thing happens here - maybe copy that approach? The empty return terminates the programme. (On Linux, it would be better to use sys.exit(1) instead of an empty return but I'm not sure how that behaves on other platforms, so just use the empty return.)

felix-schott commented 7 months ago

I'd also print a more meaningful error message: "No results found for {location} - please try a different search term." Otherwise users don't know what's happening.

felix-schott commented 7 months ago

error

Actually, sorry, you will have to use sys.exit or raise an exception since return will only exit the helper function, not the whole programme. It might be cleaner to return None from the geocoding function if no result could be found, and then use the empty return approach in the calling function if geocoding_result is None. Not sure if that's clear, if I have time, I can give you a code snippet

mtravis commented 7 months ago

@felix-schott

I've got it working with sys.exit but will try implementing the return None and updating the calling function later.

My error message is sensible looking once it calls the internal message from the event.

mtravis commented 7 months ago

@felix-schott I'm struggling a bit with to get the script to exit fully without sys.exit(1). I agree it would be cleaner not to use this method but can't get download_buildings to work.