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鈥檒l 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

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