release 0.3 preparation #314

karussell · 2018-02-19T15:36:19Z

Will try to build a global and nl+de export for the 0.3-beta3 release including the following PRs:

Fix issue 221 (partially) #288 house number fix
avoid distance_sort and use proper distance unit and tuned formula #306 proper location bias
Filter punctuation for search_ngram analyzer #311 Filter punctuation

If this is positive we'll include the specific PR. If not we'll release 0.3.0 without it.

compare geocoder tester (see here)

karussell · 2018-02-19T15:43:30Z

Have created a branch where I'll use the HEAD for the tests https://github.com/komoot/photon/tree/test03

karussell · 2018-02-24T17:36:53Z

This is now testable at http://46.4.67.134:2322/api?q=dresden

karussell · 2018-02-25T11:06:19Z

Somehow many tests fail now in geocoder_tester/world/poi/test_airports.csv (which issue could cause this?)

~~and geocoder_tester/world/france/nordpasdecalais/test_public_buildings_addresses.csv~~ (see this)

lonvia · 2018-02-25T18:55:18Z

@karussell Do we have a version with master somewhere for comparison? photon.komoot.de seems to have the test03 branch already as well.

karussell · 2018-02-25T20:48:49Z

Yeah, photon uses the current data (where due to indexing most of this is fixed) although using an older jar file (beta3). I've therefor triggered an export with latest master again and will later on trigger another export with only #288 merged. (issue #306 can be added later for both and #311 should be handled separately as otherwise we do not know what issue is causing what problems)

karussell · 2018-03-01T15:33:26Z

ok, I have now:

master http://46.4.67.134:2322/api?q=dresden
this branch is at http://46.4.67.134:2314/api?q=dresden

For e.g. q=80&lat=49.166561&lon=6.040720 you see that the installations are really different :)

(Might be very slow as I have not really enough RAM for both on the same server)

lonvia · 2018-03-01T19:06:27Z

Is that now with #306 only? The examples from #311 and #288 still yield the same (non-)results on both instances.

karussell · 2018-03-01T20:01:06Z

Uhm, no. There is something wrong with the indices of the branch

karussell · 2018-03-01T22:34:12Z

I was able to fix this (downloaded the index again), but it is still not working - really really ugly. As it was working before - see here.
Will now do an export just with #288 otherwise this is leading to nowhere here.

karussell · 2018-03-05T09:53:34Z

Have now only merged #288:

http://46.4.67.134:2288/api?q=Malplaquetstra%C3%9Fe%205A,%2013347%20Berlin,%20Germany

The master seems to return also the correct value, but probably I did something wrong there as when you test autocompletion only on GraphHopper Maps then this cannot be found. So I would merge this feature.

But the real issue is that we have to do world wide testing for any pull request that changes indexing to be sure that we have no regression. If we do this without automation then the reviewer of the PR could have a hard time reproducing because the submitter used a different data set or whatever or we would risk merging broken code. But as a world wide export takes so long we cannot easily do an export in parallel to our regular "master-export mechanism", at least not if we have more than 1 PR per month. Also requiring a world wide export from the PR submitter would make things worse.

The ideal scenario would be:

PR submitted and if all is okay the following steps could be triggered (as they are expensive it will be probably manually):
world wide export with this PR is produced
geocoder tests are run automatically: the additional tests are all green and other previously green tests do not fail
if all is fine, PR gets merged. If not, restart from 1.

hbruch · 2018-03-05T20:29:49Z

Perhaps we can improve the situation by distinguishing between index changes and export changes. If only indexing is affected, #293 (with identical id handling added) will improve import times by a factor of 5-10.

Providing a world wide nominatim json dump also would reduce the burden on submitters at least a bit as they would not need to set up a planet nominatim instance to be able to verify their changes themselves before requesting a pull (thinking of me :-)

lonvia · 2018-03-05T21:04:22Z

To be strictly comparable I would work with static test data in any case. Json dump is one option. The other option would be to create a database dump of a Nominatim database that only contains what is strictly necessary to create a Photon dump. I'd then set that up on a different machine and just to exports round the clock of all open PRs as well as latest master. It still takes two days before they are available but should be manageable with the amount of PRs we have at the moment. We could also provide the dump publicly for developers, so that their Nominatim setup would just consist of running pg_restore.

karussell · 2018-03-08T09:27:12Z

Oha, now I understand why the state of master seems to be like the new branch and why there were two indices: the two photon instances on the same server joined the cluster :D !

karussell · 2018-03-08T15:50:12Z

New attempt:

master http://46.4.67.134:2322/api?q=dresden
branch with just Fix issue 221 (partially) #288 is at http://138.201.17.75:2288/api?q=dresden

Now we see the difference for e.g. q=Malplaquetstra%C3%9Fe%205A,%2013347%20Berlin,%20Germany

I ran geocoder-tester for both and the difference is listed here: https://gist.github.com/karussell/c847cf4915151fac4c473a932f285e1c
There is some bug where good entries are listed twice geocoders/geocoder-tester#37 (comment)

karussell · 2018-03-10T00:45:28Z

Fixed the utf problem of the test runner and used the latest geocoder-tester that avoid the duplicate entries:
https://gist.github.com/karussell/c847cf4915151fac4c473a932f285e1c/revisions#diff-cac9e2da8488adbb9c149c5b51f8b965

basically all failures & successes for geocoder_tester/world/france/lorraine/test_addresses.csv disappeared and the failure of "eiffel to" and the success of "piazza di villa carpegna 00165 roma"

See the nice analysis of @hbruch #288 (comment)

lonvia · 2018-03-11T19:50:58Z

I would suggest to merge #288 at this point and repeat the process with the next PR (#306).

karussell · 2018-03-13T08:06:01Z

The easiest was to provide a master branch with 306 merged:

http://46.4.67.134:2306/api?q=dresden

but 288 with 306 would be also easy as 306 only affects. query time The result are 8 new tests pass:

https://gist.github.com/karussell/c847cf4915151fac4c473a932f285e1c#file-geotester-master-compared-with-306-log

karussell · 2018-03-13T08:20:10Z

Ok, tested 288 with 306 and there are 17 new tests passing but 2 new failures:

https://gist.github.com/karussell/c847cf4915151fac4c473a932f285e1c#file-geotester-288-compared-with-288-306-log

karussell · 2018-03-13T08:50:48Z

The first test is

/api?lat=52.3879&lon=13.0582&limit=2&q=Friedrichstraße

which now just returns "Berlin" but should return "Werder (Havel)"

The second test is

/api?lat=52.519854&lon=13.438596&limit=2&q=Rosa-Luxemburg-Straße 22

which now returns 1. "Doberlug-Kirchhain Rosa ..." and 2. "Berlin Rosa ..." but should be 1. "Berlin" and 2. "Doberlug-Kirchhain"

hbruch · 2018-03-13T09:23:55Z

The Doberlug-Kirchhain address has a name attribute with value "22". This match will boosts it's ranking more than only the context match of the Berlin match.

To me, this looks more like a data issue than a photon issue.

karussell · 2018-03-13T09:29:01Z

Ah, ok. Makes sense.

Have edited also the comments regarding first test which I initially misunderstood. Now this should pass IMO but it is not even returned for limit=10 only if I use q=Friedrichstraße Havel

karussell · 2018-03-13T09:29:15Z

http://138.201.17.75:2288/api?lat=52.3879&lon=13.0582&limit=2&q=Havel%20Friedrichstra%C3%9Fe&limit=10

It is also not wrong to return berlin and already q=Friedrichstraße W is ok but not even &location_bias_scale=10000 helps

hbruch · 2018-03-13T11:05:08Z

Using a limit of 64, the seventh (and last) result is the intended one. I suppose many matches are discarded due to street deduplication:

http://138.201.17.75:2288/api?lat=52.3879&lon=13.0582&limit=64&q=Friedrichstra%C3%9Fe

But it seems that the formular does not spread sufficiently and I'd guess that elasticsearch's BM25 similarity ranking with field-length norm ranks Berlin Friedrichstraße much higher than Werder (Havel).

karussell · 2018-03-13T14:22:56Z

Ok, so we need to fix this. I would still like to merge #306 (and open a new issue for these details). Would this be okay for you and @lonvia? Then we are able to start feeding with the remaining issue #311 for the 0.3 release.

lonvia · 2018-03-13T20:32:21Z

The Friedrichstrasse ranking comes from the importance. The street in Berlin is important enough to have its own Wikipedia entry. I wouldn't call it an error if it appears first in the list when anywhere near Berlin.

I've played around with the 306 test instance and it looks good. Pending my nitpicking comments over in the PR merging sounds fine.

karussell · 2018-03-17T14:15:16Z

#311 is now ready for testing: http://138.201.17.75:2311/api?q=%20rue%20du%20mont%20d%27or%20Saint-Didier-au-Mont-d%27Or

And when comparing to the old state where I merged 306 into 288 (which should be ~master) this results into 19 new failures and 81 new passings

Hmmh, maybe I made an error somewhere as the house numbers should not be make a difference (?) Will trigger a clean master export to make this sure.

hbruch · 2018-03-18T15:10:40Z

Hm. Comparing http://138.201.17.75:2311 and http://photon.komoot.de for all failures, I don't see any differences(?)

The 8 failures for cities in Poland have their root cause in #95: photon returns admin level 6, 7, 8 boundaries, and currently admin level 8 (city/town) is apparently returned second for the failing tests. Quick fix would increase limit in geocoder_tester, but I think solving #95 would be the better fix.

For 10 failures, there seems to be no housenumber information in OSM, resulting again (as already for #288) in ways farter away than 100m.

Only for 5 RUE LEO LAGRANGE 59265 AUBIGNY AU BAC the correct housenumber is returned second. These partial postal code matches are annoying and merrit their own issue.

To me, all failures are unrelated to 311.

karussell · 2018-03-18T15:17:58Z

I don't see any differences(?)

The problem is that the data is updated there automatically. I need to avoid to link these test exports to "photon-db-latest". As a new export will be triggered we can wait and see the differences in a few days.

To me, all failures are unrelated to 311

Thanks for your analysis!

karussell · 2018-03-20T23:41:03Z

The new master including a fresh export is now running at http://46.4.67.134:2322/api?q=dresden and it does not work for cases mentioned in #311 (probably official photon demo will automatically update soonish too to it)

Will test tomorrow how #311 compares to it.

Update: result of comparison is here

karussell · 2018-03-21T12:54:50Z

So this looks even better than the comparison before, except these failures were added:

geocoder_tester/world/france/nordpasdecalais/test_public_buildings_addresses.csv::335, rue Fernand-Bar 62400 Béthune (this is strange as I cannot find a difference)
geocoder_tester/world/germany/test_train_stations.csv::Bahnhof Dresden-Neustadt (now 2nd after Leipziger Bahnhof?)
geocoder_tester/world/poland/test_cities.csv::Poznań (okay as 2nd after admin boundary)

I don't understand the Dresden problem although the Leipziger Bahnhof is really close but it doesn't have any "Neustadt" name in its properties. Maybe we need to weight the station according to the number of platforms? E.g. Dresden-Neustadt has 4 and also historic should get lower importance as it means "inactive". It looks like this node for Dresden-Neustadt does not appear under the first 10 results.

karussell · 2018-03-21T13:15:43Z

Oh, but in general this POI search is bad (for the master instance too!). E.g. if you search for "Bahnhof Dresden" the main station is not returned under the first 10 results because its official name is "Dresden Hbf" and just the ref name is "Dresden Hauptbahnhof" (so still different to "Dresden Bahnhof")! So probably we merge #311 and improve this via #318 or related.

lonvia · 2018-03-25T20:22:37Z

The 'Leipziger Bahnhof' is in the Neustadt quarter, so there is no surprise that it shows up in the results. I just don't quite understand why #311 should change the order here.

Still, I agree that #311 is good to merge.

karussell · 2018-03-25T20:46:25Z

I just don't quite understand why #311 should change the order here.

Oh, yes. That is the right question. Maybe the random ordering wie get when feeding like @hbruch already speculated in a different issue? I think we should abandon all iterations over HashSets and use LinkedHashSets instead (potentially possible with maps too)

Still, I agree that #311 is good to merge.

Will merge and then produce a new export again

karussell · 2018-03-29T13:14:20Z

New data for the latest branch is here: http://46.4.67.134:2322/api?q=berlin

karussell · 2018-04-07T13:14:41Z

And objections to release this state as 0.3?

lonvia · 2018-04-07T16:40:36Z

Fine with me.

hbruch · 2018-04-07T20:39:39Z

Fine with me as well.

karussell · 2018-04-14T16:18:14Z

Released now at https://github.com/komoot/photon/releases/tag/0.3.0

karussell added this to the 0.3.0 milestone Feb 19, 2018

karussell mentioned this issue Feb 24, 2018

Fix issue 221 (partially) #288

Merged

karussell mentioned this issue Mar 8, 2018

Add option like skip-known-failures geocoders/geocoder-tester#37

Open

karussell closed this as completed Apr 14, 2018

release 0.3 preparation #314

release 0.3 preparation #314

Comments

karussell commented Feb 19, 2018 • edited Loading

karussell commented Feb 19, 2018

karussell commented Feb 24, 2018

karussell commented Feb 25, 2018 • edited Loading

lonvia commented Feb 25, 2018

karussell commented Feb 25, 2018

karussell commented Mar 1, 2018

lonvia commented Mar 1, 2018

karussell commented Mar 1, 2018

karussell commented Mar 1, 2018 • edited Loading

karussell commented Mar 5, 2018

hbruch commented Mar 5, 2018

lonvia commented Mar 5, 2018

karussell commented Mar 8, 2018 • edited Loading

karussell commented Mar 8, 2018 • edited Loading

karussell commented Mar 10, 2018 • edited Loading

lonvia commented Mar 11, 2018

karussell commented Mar 13, 2018 • edited Loading

karussell commented Mar 13, 2018

karussell commented Mar 13, 2018 • edited Loading

hbruch commented Mar 13, 2018

karussell commented Mar 13, 2018 • edited Loading

karussell commented Mar 13, 2018 • edited Loading

hbruch commented Mar 13, 2018

karussell commented Mar 13, 2018 • edited Loading

lonvia commented Mar 13, 2018

karussell commented Mar 17, 2018 • edited Loading

hbruch commented Mar 18, 2018

karussell commented Mar 18, 2018

karussell commented Mar 20, 2018 • edited Loading

karussell commented Mar 21, 2018 • edited Loading

karussell commented Mar 21, 2018 • edited Loading

lonvia commented Mar 25, 2018

karussell commented Mar 25, 2018

karussell commented Mar 29, 2018 • edited Loading

karussell commented Apr 7, 2018

lonvia commented Apr 7, 2018

hbruch commented Apr 7, 2018

karussell commented Apr 14, 2018

karussell commented Feb 19, 2018 •

edited

Loading

karussell commented Feb 25, 2018 •

edited

Loading

karussell commented Mar 1, 2018 •

edited

Loading

karussell commented Mar 8, 2018 •

edited

Loading

karussell commented Mar 8, 2018 •

edited

Loading

karussell commented Mar 10, 2018 •

edited

Loading

karussell commented Mar 13, 2018 •

edited

Loading

karussell commented Mar 13, 2018 •

edited

Loading

karussell commented Mar 13, 2018 •

edited

Loading

karussell commented Mar 13, 2018 •

edited

Loading

karussell commented Mar 13, 2018 •

edited

Loading

karussell commented Mar 17, 2018 •

edited

Loading

karussell commented Mar 20, 2018 •

edited

Loading

karussell commented Mar 21, 2018 •

edited

Loading

karussell commented Mar 21, 2018 •

edited

Loading

karussell commented Mar 29, 2018 •

edited

Loading