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

Portland synonyms #291

Closed
wants to merge 4 commits into from
Closed

Portland synonyms #291

wants to merge 4 commits into from

Conversation

missinglink
Copy link
Member

@missinglink missinglink commented May 16, 2018

@orangejulius we need to decide what to do with this code, I would suggest we merge it, but I'm not sure what the effect would be, in particular the following changes make me nervous:

  • removing space from the peliasStreetTokenizer
  • changing the analyzer token filters
  • changing the synonyms files, making some 'real synonyms' instead of replacements and updating the lists.

despite the changes, we should try to get this published, it was tested on a smaller extract and had a positive impact.

how would you like to handle merging, testing and deploying this?

[minor] looks like RUN npm test was removed from the dockerfile and the format was changed

@orangejulius
Copy link
Member

I agree, we should definitely merge the code changes (not the synonyms which are Portland specific).

I think with https://github.com/pelias/schema/tree/disable_fielddata, which I have tested using https://github.com/orangejulius/ci-test and appears to perform identically for Portland, we should be able to now run fairly large builds on our local machines. Want to plan to merge these code changes after comparing the results on a North America or Europe build?

@orangejulius orangejulius changed the title synonyms Portland synonyms Jun 8, 2018
@orangejulius
Copy link
Member

Thanks to pelias/docker#23 I can now confirm that this PR and #310 perform identically when used in the pelias/docker Portland Metro project. We should look towards merging that PR and closing this one when we can.

@orangejulius
Copy link
Member

All generally relevant code for this PR has now been merged in #310, so this PR is no longer needed.

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