pelias / labels

Pelias Label generation
https://pelias.io
MIT License
4 stars 9 forks source link

default_region: add region_a to label where available #41

Closed missinglink closed 3 years ago

missinglink commented 3 years ago

Using the default schema it's often difficult to distinguish localities with the same name in the same country, this makes selecting the correct result impossible:

Screenshot 2021-02-04 at 23 11 07

In other cases it would be nice to see some more information about which general area of the country a place is in:

Screenshot 2021-02-04 at 23 15 20

This PR adds the region_a value where available (except for the region layer itself, which would be redundant).

It's been this way for a very long time but I think this is an improvement, ideally I would like to see four things in the label:

{name}, {locality}, {region}, {country}

I looked at using the region label instead of region_a but it can generate some labels which sound repetitive like "Wellington, Wellington City" and "Chiang Mai, Chiang Mai".

orangejulius commented 3 years ago

Yeah, I believe this is the correct thing to do.

I took a look at this yesterday only for Brazil, and I think using the existing getRegionalValue method in all labels would work well.

That method has handling for region placetypes, and preference to use region_a but fallback to region (the name).

Joxit commented 3 years ago

region_a is a good agreement, this should be small and will create less noise than the full region name.

And this will not change the FRA overseas where the region replaces the country behavior so :+1:

missinglink commented 3 years ago

@Joxit I think this change will leave FRA as the only country with only two parts in the schema.

Is France good as-is or should we do something similar there?

Joxit commented 3 years ago

IFAIK, we don't use region abbreviations (I didn't know there were :sweat_smile:) We use the full region name or the postalcode. For example in our demo website, when you are looking for the city Bagneux (7 Bagneux in France :scream:), the label looks like {locality} - {country}, {region} only when there are duplicates.

image

But when you are looking for a city without duplicates, the label looks like {locality} - {country}, for example Paris - France: image

So I think France is good as-is :sweat_smile:

missinglink commented 3 years ago

Hmm... think you're generating your own labels? Both the default labels and default results are quite different from what's shown in your screenshots 🤷‍♂️

Screenshot 2021-02-05 at 13 08 33 Screenshot 2021-02-05 at 13 10 08

In both cases I think the results from Jawg are better, any idea what config you're using for that?

Joxit commented 3 years ago

Good morning 😴

This is a custom integration for our demo, so the job is not done by the API (for a better DX, this should be done by the API..)

When we receive all records, we checks duplicates and we update them.

If we want this in Pelias, we will need a Middleware after the label which will check duplicated labels and rename them 🤔

So this will need a work in both the API and this module where the region part can be optional for exemple?

orangejulius commented 3 years ago

Okay, I ran the acceptance tests with this change to get an idea of what a bunch of labels will look like.

I think it adds a bit too many abbreviations. In particular, if the label already includes a city, and there's also a region of the same name, I don't think we should show the abbreviation. For example: image

We might not be able to easily detect this case without adding a bunch more logic. Looks like @Joxit might be working on exactly that in https://github.com/pelias/labels/commit/bbf316a6c8fe9131d621062a1a067a1123e79be3 :)

Joxit commented 3 years ago

Berlin is a good edge case for my PR... :confused: Berlin is both a city AND a region.... Just like Paris... So when we add the region part, this will add nothing more...

Maybe withOptional is too simple for what we want to do ? We should start with something complicated/more specific on regional part ? To be, or not to be, that is the question :shrug:

orangejulius commented 3 years ago

Yeah, I think there are several aspects to a good solution:

orangejulius commented 3 years ago

Replaced with https://github.com/pelias/labels/pull/43 :)