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

don't boost street if parse is ambiguous #1479

Closed
wants to merge 2 commits into from
Closed

Conversation

blackmad
Copy link
Contributor

@blackmad blackmad commented Jul 31, 2020

This is a redo of #1430 that's on the pelias repo rather than on mine, and fixed for place->venue rename

It fixes mccarren park and wrigley field as well as mccarren park brooklyn and wrigley field chicago

this was the original approach suggested by @missinglink for solving "wrigley field" - I don't see a great way to solve it in pelias/parser at the moment, though I'm open to suggestions.

doesn't change anything in a 1k query set.

@orangejulius
Copy link
Member

Cool, this makes a lot of sense. I'll test it out next week!

@blackmad
Copy link
Contributor Author

blackmad commented Aug 4, 2020

something else this should probably do is that if the parse is ambiguous, consume the whole query

it would fix queries like
Kinetic Arts Center
"parsed_text": {
"subject": "Arts Center",
"street": "Arts Center"
}

and

KT Auto Repair
"parsed_text": {
"subject": "Auto Repair",
"street": "Auto Repair"
}

@orangejulius
Copy link
Member

orangejulius commented Aug 10, 2020

I just gave this PR a full run through all our tests, and it looks good. There are some improvements to POI related tests, such as the following:
Screenshot_2020-08-10_16-08-02
It looks similar, but not quite as good as #1469 in that regard (#1469 is a more aggressive change with more downsides, so...tradeoffs)

There were very minimal changes to address queries that are not worth worrying about. Here's an example of the magnitude of the changes
Screenshot_2020-08-10_16-11-19

As far as I'm concerned from a behavior perspective this PR is good to merge.

@missinglink do you have any thoughts on the code? It does add even more logic to our already complex sanitizer/_text_pelias_parser.js file, but that's probably unavoidable.

return (
streetOnlySolution &&
venueOnlySolution &&
streetOnlySolution.pair[0].span.body === venueOnlySolution.pair[0].span.body &&
Copy link
Member

Choose a reason for hiding this comment

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

I think you can put this before const restOfSolutionIsTheSame =.... ? something like

if (!streetOnlySolution || !venueOnlySolution || streetOnlySolution.pair[0].span.body !== venueOnlySolution.pair[0].span.body) {
  return false
}

@orangejulius
Copy link
Member

We just merged #1308, so I'd be curious to see if/when this PR provides additional improvements on top of this. @blackmad if you get a chance to rebase and check it out, let me know, otherwise I'll add it to my queue for testing.

@blackmad
Copy link
Contributor Author

I think this is basically a no-op on top of 1308, I'm happy to just close it

@blackmad blackmad closed this Aug 12, 2020
@orangejulius
Copy link
Member

@blackmad Its a no-op for autocomplete, but not necessarily search, so it might still be useful.

@blackmad
Copy link
Contributor Author

okay true you're right. This is still important for "wrigley field" in search

@blackmad blackmad reopened this Aug 13, 2020
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.

3 participants