-
-
Notifications
You must be signed in to change notification settings - Fork 44
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
Adding unit attribute to document stream to include addresses with th… #320
base: master
Are you sure you want to change the base?
Conversation
…/openaddresses into adding-address-unit * 'adding-address-unit' of https://github.com/sweco-semhul/openaddresses: fix(package): update pelias-wof-admin-lookup to version 4.3.2 disable package-lock in .npmrc fix(package): update pelias-wof-admin-lookup to version 4.3.1 Upgrade to pelias-wof-admin-lookup 4.3.0 Revert "chore(package): update dependencies" chore(package): update dependencies fix(package): update pelias-wof-admin-lookup to version 4.2.0 fix(package): update pelias-dbclient to version 2.3.0
@orangejulius updated with changed order for street and unit attribute |
It's been a long time and I'd love to see this merged. The reason it's not an easy merge is that it makes a change to the name field which effects This has a (potentially) big impact on the system because:
The potential issue with autocomplete is due to how the prefixngrams are calculated for the I suspect that if a user asks for This is only a suspicion and it'd need to be tested, I just wanted to write down my concerns with merging this and what needs to be tested before we do. This behaviour would also be compounded by the similarity scoring which would favour tokens with a lower Inverse-Document-Frequency. I drafted another PR yesterday #402 which added additional address fields not currently present in the model, that PR doesn't change |
Following up on a super old PR that I'd also love to see merged The last comment that was holding this PR up was around this
I've got something very similar setup locally and I've tested out this scenario.
Note that the data I'm testing with has unit numbers rather than unit letters, I'm not sure if this would make a material difference. Let me know if I can do anything else on this. |
@rowanwins this is similar to #402 in some ways and different in others. A few observations about this PR:
I suspect that second point was enough to scare us off merging it and would need to be thoroughly tested before we could consider hitting merge. |
…is attribute, see e.g. openaddresses/openaddresses#3511
To resolve pelias/pelias#618 more details found in comments to that issue