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

feat(localNames): prefer local names in lookup #318

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JeremyBYU
Copy link

👋 I did some awesome work for the Pelias project and would love for everyone to have a look at it and provide feedback.


Here's the reason for this change 🚀

This allows the PIP-Service to return administrative names in the local language. It seems this feature was started and pushed to the master branch in this commit in 2017. This 2017 commit involves adding a local boolean parameter called localizedAdminNames, which if set to true extracts the local name from the WOF dataset. However, I noticed some issues:

  • The feature is permanently disabled by hard coding a false value here. I do not know why this was done. I am guessing because the feature was incomplete and not tested?
  • The implementation also has a small bug where the boolean data type of localizedAdminNames is converted to the string data type when passed into a forked child worker process here. This leads to the correct if-branch never being taken here.

Here's what actually got changed 👏

  • We updated schema.js to include the parameter imports.adminLookup.localizedAdminNames in pelias.json. Default is set to false to be backward compatible.
  • This configuration parameter is now read in index.js and passed downstream.
  • We also fixed the bool -> string type change localizedAdminNames experiences across process boundaries in src/pip/worker.js

Here's how others can test the changes 👀

I tested this by updating PIP-Service to point to my fork. I then manually rebuilt the docker image. I then used the following pelias.json file:

{
  "logger": {
    "level": "info",
    "timestamp": false
  },
  "imports": {
    "adminLookup": {
      "enabled": true,
      "localizedAdminNames": true
    },
    "whosonfirst": {
      "datapath": "/data/whosonfirst",
      "countryCode": ["FR"],
      "importPlace": ["136253037", "85633147"]
    }
  }
}

and the following docker-compose.yml:

version: '3'
networks:
  default:
    driver: bridge
services:
  whosonfirst: # data download and prepartion
    image: pelias/whosonfirst:master
    container_name: pelias_whosonfirst
    user: "${DOCKER_USER}"
    volumes:
      - "./pelias.json:/code/pelias.json"
      - "${DATA_DIR}:/data"
  pip: # run-time container
    image: pelias/pip-service:master
    container_name: pelias_pip-service
    user: "${DOCKER_USER}"
    restart: always
    environment: [ "PORT=4200" ]
    ports: [ "127.0.0.1:4200:4200" ]
    volumes:
      - "./pelias.json:/code/pelias.json"
      - "${DATA_DIR}:/data"

When you start up pip it will take some time (~3-5 min) to build the in-memory spatial index. Afterward you can query the service as so: curl http://localhost:4200/2.320957/48.871326. You should see a response as so:

...
  "macroregion": [
    {
      "id": 404227465,
      "name": "Île-De-France", # without this feature it would be Ile-of-France
      "abbr": "IF",
      "centroid": { "lat": 48.709278, "lon": 2.503396 },
      "bounding_box": "1.446743,48.120537,3.558455,49.241062"
    }
  ]
...

We also did these tests on about 50K different GPS points in France and got a 90% exact match with our existing french-named locality lookup using a Nominatim instance. A sampling of the reaming 10% seem to be minor neighborhood differences (still in French).


Possible Concerns

We did not test the world. There could be edge cases where this breaks? I also have concerns about why the feature was not enabled in the first place. Maybe I missed something?

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.

1 participant