Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add random user ID for Nominatim and allow arbitrary geocoder #164

Merged
merged 5 commits into from
Oct 16, 2020

Conversation

darribas
Copy link
Collaborator

This addresses #44

Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few suggestions!

contextily/place.py Outdated Show resolved Hide resolved
# Set user ID for Nominatim
rng = np.random.default_rng()
val = rng.integers(1000000)
gp.geocoders.options.default_user_agent = f"contextily_user_{val}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we only set this in the actual geocoder object we create below?
Because now importing contextily will have a side effect on the geopy module

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@darribas are you OK with only passing this "contextily_user_val" to the gp.geocoders.Nominatim() default keyword value?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@darribas are you ok with this? Then I can push a change, merge this, and do a small bug fix release

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds great and a much cleaner way to go about. I'm +1 on this

@jorisvandenbossche
Copy link
Member

It also seems that I made a small mistake in my PR (hidden by those failures), from the travis log:

                # Warp
                if (crs is not None) and (raster.crs != crs):
                    image, bounds, _ = _warper(
                        image, img_transform, raster.crs, crs, resampling
                    )
                image = image.transpose(1, 2, 0)
>               extent = bounds.left, bounds.right, bounds.bottom, bounds.top
E               UnboundLocalError: local variable 'bounds' referenced before assignment
contextily/plotting.py:190: UnboundLocalError

If not warping, we need to get the bounds from raster.bounds

rng = np.random.default_rng()
val = rng.integers(1000000)
gp.geocoders.options.default_user_agent = f"contextily_user_{val}"
_val = np.random.randint(1000000)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds good and more backwards compatible. I tried to go with the numpy recommendation but if it's not a widespread approach yet and fails on earlier versions, let's go with randint

@jorisvandenbossche
Copy link
Member

OK, updated to only use the default contextily user agent for the actual Nominatim geocoder that is created as the default.

@jorisvandenbossche jorisvandenbossche merged commit 27d60da into geopandas:master Oct 16, 2020
jpn-- added a commit to jpn--/contextily that referenced this pull request Jan 8, 2021
…iles

* commit '3c26d5a16a094e8d5ae05f8e5a41f05278816daf':
  DOC: update for latest pydata-sphinx-theme (fix sidebar + use CSS variables) (geopandas#168)
  RLS: v1.0.1
  Fix resetting of extent with local files (geopandas#155) (geopandas#156)
  Add random user ID for Nominatim and allow arbitrary geocoder (geopandas#164)
  Close and reopen memfile dataset before WarpedVRT (geopandas#165)
  Fix use of rasterio MemoryFile (geopandas#163)
  DOC: Use Cape Town consistently in description and variable names (geopandas#154)
  Add matplotlib Framework classifier (geopandas#152)
  Require Python 3.6 as minimum version (geopandas#150)
  Add earthengine to docker for binder
  Example interfacing GEE with contextily (geopandas#147)
  DOC: add notebook about working with local files (geopandas#139)

# Conflicts:
#	tests/test_ctx.py
@darribas darribas mentioned this pull request Feb 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants