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

Boost responses by country code #1650

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

Joxit
Copy link
Member

@Joxit Joxit commented May 4, 2023

👋 I did some awesome work for the Pelias project and would love for everyone to have a look at it and provide feedback.


Here's the reason for this change 🚀

closes pelias/pelias#944


Here's what actually got changed 👏

I added the query paramater focus.country and focus.gid available only with autocomplete API. This will apply a defined boost set to 1.5 and not customizable. I did some tests and seems to be correct. I also tried 1.25 and 2


Here's how others can test the changes 👀

Some examples with focus.country

/v1/autocomplete?focus.country=fra&size=3&text=13 allée France addresses are in first position

// from
0) Alleestraße 13, Potsdam, BB, Allemagne
1) Alleestraße 13, Putbus, MV, Allemagne
2) Al Molo 13, Capriate San Gervasio, BG, Italie
//to 
0) 13 Allée Louis 13, Maubeuge, France
1) 13 Allee De 13 Vents, Le Breuil-sur-Couze, France
2) 13 Allee Marguerite De Flandre(1-13), Prémesques, France

/v1/autocomplete?focus.country=usa&size=5&lang=en&text=paris USA documents before Jamaica, but Paris in France still in first position

// from
0) Métropole du Grand Paris, France
1) Paris, France
2) Saint Andrew, Jamaica
3) Saint Catherine, Jamaica
4) East Baton Rouge Parish, LA, USA
//to
0) Métropole du Grand Paris, France
1) Paris, France
2) East Baton Rouge Parish, LA, USA
3) Jefferson Parish, LA, USA
4) Orleans Parish, LA, USA

/v1/autocomplete?focus.country=gb&size=5&lang=en&text=melbourne same as Paris, Melbourne is still in first position, but now we see cities from UK

// from
0) Melbourne, VIC, Australia
1) Melbourne, FL, USA
2) Melbourne City Centre, Melbourne, VIC, Australia
3) West Melbourne, FL, USA
4) Port Melbourne, VIC, Australia
// to
0) Melbourne, VIC, Australia
1) Melbourne, England, United Kingdom
2) Melbourne, England, United Kingdom
3) Melbourne, FL, USA
4) Melbourne City Centre, Melbourne, VIC, Australia

Some examples with focus.gid

/v1/autocomplete?focus.gid=whosonfirst:localadmin:1377689997&size=5&text=Starbucks Focus on Aachen in Germany

// from
0) Starbucks, West Drayton, England, United Kingdom
1) Starbucks, Jenjarom, SGR, Malaysia
2) Starbucks, Aachen, NW, Germany
3) Starbucks, Shah Alam, SGR, Malaysia
4) Starbucks, Seattle, WA, USA
//to
0) Starbucks, Aachen, NW, Germany
1) Starbucks, West Drayton, England, United Kingdom
2) Starbucks, Jenjarom, SGR, Malaysia
3) Starbucks, Shah Alam, SGR, Malaysia
4) Starbucks, Seattle, WA, USA

/v1/autocomplete?focus.gid=whosonfirst:region:85688651&size=5&lang=en&text=melbourne just like focus.country this will change the order of result but not replace very high ranking (focus on Florida)

// from
0) Melbourne, VIC, Australia
1) Melbourne, FL, USA
2) Melbourne City Centre, Melbourne, VIC, Australia
3) West Melbourne, FL, USA
4) Port Melbourne, VIC, Australia
//to
0) Melbourne, VIC, Australia
1) Melbourne, FL, USA
2) West Melbourne, FL, USA
3) Melbourne Beach, FL, USA
4) Melbourne City Centre, Melbourne, VIC, Australia

I had to change a test in test/unit/helper/diffPlaces.js, IDK why 🤷

@Joxit
Copy link
Member Author

Joxit commented May 5, 2023

I've just added the support of focus.gid and updated the PR description with new examples.

I thik this is good to go now 😄

@Joxit Joxit marked this pull request as ready for review May 5, 2023 11:52
@Joxit Joxit requested a review from missinglink May 5, 2023 11:52
@Joxit Joxit changed the title Joxit/feat/focus country Boost responses by country code May 21, 2023
@Joxit
Copy link
Member Author

Joxit commented Jun 23, 2023

Can I have feedback on this PR ? Is the API OK for you ?

@missinglink
Copy link
Member

Hi @Joxit, sorry I'm super busy this week (Berlin Buzzwords) and next week I'm off to Kosovo (FOSS4G).

I'll try and have a look over it while I'm away next week.

FYI I think the test issue with the Eszett is caused by tyxla/remove-accents#12

@Joxit
Copy link
Member Author

Joxit commented Jun 23, 2023

Okay, thanks for your response 😄

@missinglink
Copy link
Member

Heya, I just got back from Kosovo and super exhausted, just wanted to send you a quick note that this is on my todo list.

@Joxit Joxit force-pushed the joxit/feat/focus-country branch from 765f0d9 to 82e5655 Compare July 3, 2023 14:59
@Joxit
Copy link
Member Author

Joxit commented Jul 3, 2023

Hi Peter, thank you for your update! I've sync the branch with master according to #1655

Rest well 😉

Copy link
Member

@missinglink missinglink left a comment

Choose a reason for hiding this comment

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

Sorry this took so long to review 🙇

Code looks great, nice and clean and easy to read 👍
One minor question about the ES query.

I'm happy to merge this.
Can we please add user-facing documentation for this API.

Comment on lines 10 to 15
return {
'function_score': {
'query': peliasQuery.view.leaf.multi_match(view_name)(vs),
'score_mode': 'multiply',
},
};
Copy link
Member

@missinglink missinglink Jul 17, 2023

Choose a reason for hiding this comment

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

question about this...

what's the purpose of wrapping the 'leaf' query in a 'function_score' here?
doesn't the 'function_score' query require an Array named 'functions', with at least one function?
the 'score_mode' property then effects how scoring is applied to the multiple 'functions'

Copy link
Member

Choose a reason for hiding this comment

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

I guess what I'm asking is, what's the difference between this and:

return peliasQuery.view.leaf.multi_match(view_name)(vs)

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 tried different score_mode and wanted to be consistent with other functions, but you're right, since I'm using the defaults value, it's semantically the same, so I can return the leaf instead of the function_score 👍

@missinglink
Copy link
Member

missinglink commented Jul 17, 2023

These API params are available for /v1/autocomplete but not /v1/search, we've been moving toward having parity between the two APIs so I think it might be nice to have this also available for search?

I noticed that there are no new tests in test/unit/fixtures, could we add a couple fixtures please? they make understanding how the queries get generated easier and also help prevent someone else coming along and breaking this feature in the future 🙏

@Joxit
Copy link
Member Author

Joxit commented Jul 25, 2023

Hi Pete, thank you for your comments. I removed the function_score from the query and added tests + fixtures 🚀

For the /search endpoint and the two step search (first one with placeholder and second with ES) I don't know which behavior the API should have when placeholder is used.

Currently, the result for /v1/search?size=5&lang=en&text=paris is

Paris, France
Paris, TX, USA
Paris, ON, Canada
Paris, TN, USA
Swainsboro, GA, USA

If we add a focus.country=USA and say "every results in the USA goes first" (since we do not score results here) Paris, France will completely disappear because placeholder send 15 results in USA... 😕
This is a bit tricky IMO and I don't know the correct way to do it for now...
However when search uses ES, this should be ok 😄

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.

Boost responses by country code
2 participants