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

Venue popularity additions #531

Merged
merged 1 commit into from
Jun 1, 2020
Merged

Conversation

orangejulius
Copy link
Member

@orangejulius orangejulius commented May 18, 2020

This PR builds on #493 with a couple additions I think might help our existing set of test cases. I'm sure we could think of more to add!

Closes pelias/pelias#171
Closes #493

@orangejulius orangejulius marked this pull request as draft May 18, 2020 20:34
@orangejulius orangejulius marked this pull request as ready for review May 22, 2020 17:48
@orangejulius
Copy link
Member Author

I've done some testing on this PR, and while we will have a lot more work to do to really take advantage of these changes, I believe it's more than good enough to merge. It makes a big difference in a good number of cases.

Some observations:

Improved: San Francisco Zoo

/v1/autocomplete?focus.point.lat=37.743618&focus.point.lon=-122.426117&text=zoo
Screenshot_2020-05-22_11-03-31

This query has given us trouble for a long time. Even with proximity boosts, there's not much to prefer a nearby Zoo over places with a technically better text match.

Improved: Statue of Liberty

This one is truly a classic, and returns much better results now. Very frustratingly, for search, the ferry terminal on Liberty Island is the first result. This is because only the ferry terminal is part of the Liberty Island neighbourhood, so it gets a boost enough to outrank the high-popularity statue. We might be able to address this by changing the boost given to neighbourhood matches, or maybe even just removing that neighbourhood from admin lookup.

/v1/search?boundary.country=USA&text=statue of liberty
Screenshot_2020-05-22_11-07-43

The difference for autocomplete is quite good, however:
Screenshot_2020-05-22_11-07-21

Surprise Improvement: Structured requests for venues

It looks like some long-failing structured geocoding requests are now passing because the venue being returned has just enough boosting to be ranked higher than an address for the same location:

/v1/search/structured?venue=police&address=1090 N Charlotte Street&locality=Lancaster&region=PA

Screenshot_2020-05-22_11-50-12

I'm actually a little unclear on how we want the venue parameter on structured search to behave, but it feels right that this query now returns a venue.

Less than expected (if any) improvement: POI test cases

While there are some improvements here, it looks like most POI result improvements will require scoring fixes (like pelias/pelias#862) to solve.
Screenshot_2020-05-22_12-10-38

Testing data

Here are some complete acceptance test logs used for comparison:
dev-2020-05-07-10:17:40-2020-05-05-build-master-baseline-acceptance-tests.txt
dev-2020-05-07-10:17:40-2020-05-05-build-master-baseline-autocomplete-acceptance-tests.txt
dev-2020-05-20-14:50:19-2020-05-18-venue-popularity-additions-acceptance-tests.txt
dev-2020-05-20-14:50:19-2020-05-18-venue-popularity-additions-autocomplete-acceptance-tests.txt

Conclusion

While there are places where we'd prefer better results, this PR doesn't seem to break anything, and includes quite a few solid improvements! It creates a solid foundation for adding popularity to venue records (meaning I consider it to solve a 5 year old issue: pelias/pelias#171). I'm sure we'll be tweaking this sort of thing forever, but I think we should merge this PR to start us out!

Comment on lines +167 to +170
// only map venues
if( doc.getLayer() !== 'venue' ){
return next(null, doc);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking we could remove this. Famous addresses such as for the Sydney Opera House may not always appear first

Copy link
Member Author

Choose a reason for hiding this comment

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

On second thought let's definitely either solve this separately, or at least enable popularity for addresses in a separate PR.

Copy link
Member

@Joxit Joxit left a comment

Choose a reason for hiding this comment

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

It looks very promising 🚀

@orangejulius orangejulius merged commit 3303aa1 into master Jun 1, 2020
@orangejulius orangejulius deleted the venue_popularity_additions branch June 1, 2020 20:32
orangejulius added a commit to pelias/acceptance-tests that referenced this pull request Jun 3, 2020
These are now passing thanks to OSM venue popularity:
pelias/openstreetmap#531
orangejulius added a commit to pelias/acceptance-tests that referenced this pull request Jun 25, 2020
Appears to have been fixed by OSM venue popularity

pelias/openstreetmap#531
pelias/pelias#183
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.

Add popularity scores to landmarks
3 participants