-
-
Notifications
You must be signed in to change notification settings - Fork 73
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 "Railway Station alias" to improve searches #470
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests are failing, I think you have an extra opening curly brace there?
I think this might need a bit more consideration for cases such as:
I think we can probably still merge this, the nice thing about aliases is that they are used for search but not for display, so we are free to put whatever we want if it improves matching. The label generation might still be an issue here, as the resulting geojson record would still not have 'station' in the label, which might be confusing from a UX perspective. Regarding the language, we can probably deal with this in elasticsearch with synonyms, we could, for instance add a synonym "railway station, bahnhof" |
Whoops, typo fixed. Yep I guessed that aliases were good to use for this purpose. I'm not sure how many cases there are like this where adding additional aliases would improve searchability substantially, railway stations was a single example I've noticed myself. If there are many others it might be worth refactoring it to handle it in a more generic way. Adding a indexOf probably can't hurt either to ensure it's not already in the name. As for the label, it may be better to expose the categories instead rather than changing the label so it can be annotated at the application level. E.g. show a icon to indicate a train station instead of including it in the name. That also means we don't need to change the label for other languages as well. |
Actually another use for synonyms would be to allow "railway station" and "train station" to both be used when searching. Are there currently any synonyms specifically for venue searching? |
Hi @NickStallman the tests are still failing, you can run the same tests locally that are getting run on CI with I had a quick look over the failures and see that https://www.openstreetmap.org/node/25751159 is producing the alias |
Sorry for some reason the unit tests are throwing other unrelated errors for me. When I have a sec I'll track that down and sort that out. :) |
I've had a chance to sit down and do this properly so I've redone it entirely. I've added test cases to cover the new code and all tests are now passing perfectly. The idea is then to add synonyms to handle different languages and terminology, E.g "Train Station" @missinglink let me know what you think, this is my first substantial contribution so let me know if the style or anything else isn't quite in line with what the project would like. :) |
This is looking really good, I opened a small PR against your fork with some minor formatting changes. Other than that, I think we should merge this. |
minor code formatting
Awesome. I think I will first add support for multiple conditions as that appears to be required for ferries and some other places where this will be useful. |
All good, I will hold off merging until I hear more. |
Add Ferry normalisation Add Car Park normalisation
@missinglink I've rewritten this so it can handle multiple conditions which is a requirement for things like public_transport=stop_position ferry=yes Let me know what you think. I'll file another PR to the API to update the synonym list so things like "train station" will work as you'd expect. I'll also be using this code on a planet build to test out real world results. |
… alias-station-names
I think this is ready for merging. I've gotten rid of the expected output conflict and I've made a full planet build with the desired outcome. Searching "railway, " where the train station's name is just "" is now working correctly. Next steps: |
Many railway stations just have the station name without any mention of "railway" or "station" in it.
E.g. https://www.openstreetmap.org/node/1886265358
In that case the station name is the same as a locality so it's virtually impossible to find the station in search results.
This patch will add "Railway Station" as an alias to allow for easier searching.
It also adds a small popularity boost to the station since they are common landmarks.
TODO: Likely this needs to be expanded for bus stations, light rail, ferry, etc...