api icon indicating copy to clipboard operation
api copied to clipboard

autocomplete: add macrocounty to list of potential admin matching fields

Open missinglink opened this issue 4 years ago • 5 comments

The query "2 Macquarie Street, Sydney" does not currently return either of these two results despite Parramatta being a suburb within the greater Metro Sydney Area.

The reason for this is that the Metro Area is rather large and so WOF has designated it as a 'macrocounty'.

We're currently not targeting macrocounty in the list of fields which are queried for the 'admin portion' of an autocomplete query:

Screenshot 2021-08-12 at 11 04 39

With this PR we add macrocounty to the existing list:

Screenshot 2021-08-12 at 10 59 36

The macrocounty placetype is seldom used, it only has 477 places at time of writing, so this likely won't have any adverse effects.

@Joxit I noticed on that link above that there's a bunch in France, would this change be beneficial/detrimental for you?

  • https://pelias.github.io/compare/#/v1/autocomplete?text=2+Macquarie+Street+Sydney&debug=1
  • https://pelias.github.io/compare/#/v1/autocomplete?text=2+Macquarie+Street+Parramatta&debug=1

missinglink avatar Aug 12 '21 09:08 missinglink

wow, so actually, that's apparently a very common street name in Sydney, how confusing 😆 https://pelias.github.io/compare/#/v1/autocomplete?boundary.gid=whosonfirst%3Amacrocounty%3A1376953385&text=2+Macquarie+Street

missinglink avatar Aug 12 '21 09:08 missinglink

Hum, in France, the macrocounty is a subdivision of the region where the name of the macrocounty is most often the name of capital/main city... It's not common to use macrocounty in daily life.

I tried something, a street named Avenue Aristide Briand which crosses several cities in the macrocounty named Antony. :memo: Antony, Bagneux, Montrouge, Bourg-la-Reine, Le Plessis-Robinson are cities that are part of the Antony macrocounty.

Before the PR

Avenue Aristide Briand, Antony

Avenue Aristide Briand, Antony, France

After the PR

Avenue Aristide Briand, Antony

Avenue Aristide Briand, Antony, France
Avenue Aristide Briand (Coté Bagneux), Bagneux, France
Avenue Aristide Briand, Montrouge, France
Avenue Aristide Briand, Bagneux, France
Avenue Aristide Briand, Bourg-la-Reine, France
Avenue Aristide Briand, Le Plessis-Robinson, France

The new result is a bit weird but Antony city is still in first position then...

Same behavior with venues

Credit Agricole, Antony

Crédit Agricole, Antony, France
Crédit Agricole, Montrouge, France
Crédit Agricole, Clamart, France
Crédit Agricole, Châtillon, France
Crédit Agricole, Vanves, France
Crédit Agricole, Le Plessis-Robinson, France
Crédit Agricole, Bourg-la-Reine, France
Crédit Agricole, Sceaux, France
Crédit Agricole - Forum, Montrouge, France
Crédit Agricole immobilier, Montrouge, France

Joxit avatar Aug 12 '21 10:08 Joxit

Hey @Joxit I'm feeling less confident with merging this now, I like the improvements in Australia but it feels like it might come with a regression in France/Germany and any other countries which use macrocounty.

I see you approved the PR, does that mean you think it's still worth merging despite that?

missinglink avatar Aug 13 '21 11:08 missinglink

Hi @missinglink, results for France reminded me this draft pelias/wof-admin-lookup#300

IMO, if France were the only country affected and if it improves the results in Australia, I think it may be worth it. Since we are talking about autocomplete, I think that returning results from nearby cities might not be a bad idea either :thinking: (even if in my examples there are too many results :sweat_smile:)

Joxit avatar Aug 19 '21 21:08 Joxit

🤔 thinking... {buffering}

missinglink avatar Aug 20 '21 08:08 missinglink

This PR stalled due to it increasing noise in France while also fixing issues in Australia.

It seems that the issues in Australia have worsened now since some WOF refactoring and many addresses are no longer retrievable in Melbourne (for instance) because it only appears as a macroregion in the document parents.

I feel that the omission of macroregion from the query list was due to oversight (or it being introduced later) rather than intent, we should be listing all the parent fields in the query.

For that reason I'd like to imminently merge this PR.

missinglink avatar Mar 28 '24 13:03 missinglink