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

Fix for the house number cause the erratic behaviour #391

Closed
wants to merge 15 commits into from

Conversation

DavidGorritiGeograma
Copy link

@DavidGorritiGeograma DavidGorritiGeograma commented Mar 14, 2019

e.g. we received a golf course hole in Australia instead of an address in Austria as the following:
http://photon.komoot.de/api/?q=3%20Universitaetsplatz,%20graz,%20austria

With location_bias_scale, it still shows the first two results in Italy:
http://photon.komoot.de/api/?q=3%20Universitaetsplatz,%20graz,%20austria&lat=47.58472242&lon=14.13731478&location_bias_scale=10

However, the result is correct without house number:
http://photon.komoot.de/api/?q=Universitaetsplatz,%20graz,%20austria

Therefore, we increase the prefix matching number, and boost the city and street name to increase the rank of the matching location names.
And modify the bias boost mode to locate the result closer to the bias center.

Thanks for your review and help!
David Gorriti

@@ -50,29 +50,32 @@
private MatchQueryBuilder defaultMatchQueryBuilder;

private MatchQueryBuilder languageMatchQueryBuilder;

private MatchQueryBuilder numricMatchQueryBuilder;

Copy link
Contributor

@simonpoole simonpoole Apr 8, 2019

Choose a reason for hiding this comment

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

This variable isn't used anywhere , and should be removed or used (PS it would be "numeric").

Choose a reason for hiding this comment

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

Thank you very much! I removed the unused the parameter. By the way, could you please help me to review if there is a better solution to solve the issue? Thanks again!
David

Copy link
Contributor

Choose a reason for hiding this comment

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

We are rather thin on ES knowledge here and up to now none of the relevant experts has weighed in yet, but, speaking for myself, the main question is how much this impacts the behaviour outside of the specific case you've been looking at (which it clearly improves). In my preliminary tests the change in behaviour is large so it will take some time to get to a definite conclusion on the PR.

Choose a reason for hiding this comment

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

Thank you so much for your explanation and help!

// @formatter:off
m_queryBuilderForTopLevelFilter = QueryBuilders.boolQuery()
.should(QueryBuilders.boolQuery().mustNot(QueryBuilders.existsQuery("housenumber")))
.should(QueryBuilders.matchQuery("housenumber", query).analyzer("standard"))
.should(QueryBuilders.existsQuery(String.format("name.%s.raw", language)));
Copy link
Contributor

@simonpoole simonpoole Apr 8, 2019

Choose a reason for hiding this comment

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

Removing this would seem to partially break querying for objects with names. I added this back and the behaviour seems to be much better.

Choose a reason for hiding this comment

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

Thank you! I will do more tests as well.

@juanpelegrina
Copy link

Dear @simonpoole,
I would like to know the status of this pull request. Are you planning to accept it? we have been using the code and we have not got any errors.
thanks for your help.
Cheers
Juan

@simonpoole
Copy link
Contributor

Dear @simonpoole,
I would like to know the status of this pull request. Are you planning to accept it? we have been using the code and we have not got any errors.
thanks for your help.
Cheers
Juan

Hi Juan

I'm not a maintainer of this repo and wont be able to help directly. @lonvia any input?

@lonvia
Copy link
Collaborator

lonvia commented Nov 17, 2019

I need to find time to review this. Not sure when this will happen.

@juanpelegrina
Copy link

I need to find time to review this. Not sure when this will happen.

Thanks @lonvia is there any way we can help you? @DavidGorritiGeograma @danielnavarrogeo and I have been involved in the development. We are interested in the improvement, let us know how we can collaborate.

Cheers
Juan

@lonvia
Copy link
Collaborator

lonvia commented Nov 19, 2019

Do you mind rebasing it on the latest master and force-push then new version? That would help.

@DavidGorritiGeograma
Copy link
Author

Do you mind rebasing it on the latest master and force-push then new version? That would help.

Hello @lonvia, we have rebased our code with the latest fix for the test.
Thanks a lot for your help!
David

@@ -0,0 +1 @@
/PhotonQueryBuilderBackup.java
Copy link
Contributor

Choose a reason for hiding this comment

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

Putting java files in gitignore seems wrong

Choose a reason for hiding this comment

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

Hello, I deleted the .gitignore file.
Thank you very much!
David

@juanpelegrina
Copy link

Hi @lonvia,
Could be possible to accept this pull request?
Regards
Juan

@DavidGorritiGeograma
Copy link
Author

Dear @lonvia,

We have been working locally with the solution which we developed in this PR, and we can confirm the results improve. We have been testing our proposal and we would like to share with you the results. Please check on this map the results of our testing

https://www.google.com/maps/d/viewer?mid=1JSQEeEBy0VyQRB2BOjo3RgNnXO1N4vF1&ll=54.45751685160265%2C15.605748222341717&z=4

Thanks for your comment and advice!
David

@lonvia
Copy link
Collaborator

lonvia commented Jan 20, 2021

I have looked into this over Christmas. As far as I can see, the flaky housenumbers come from a different issue. We don't handle mixed-ligual searches very well Pretty much what #496 describes. Housenumbers are always in the original name while other parts tend to come from the language-specific parts. That's why they get hit by this more often.

I've looked through some of the examples from your Google document and as far as I can see the problem with them is that other parts of the address are missing in the database. The second round in your PR is more lenient with that.

I think that your approach with a second round has merit but I'd like to see the mixed-lingual problem solved first. I've experimented with multi-match on this branch. It fixed pretty much all the bug reports except for #227. And I still have to track down what the issue with that is.

@lonvia
Copy link
Collaborator

lonvia commented Feb 17, 2022

Last years changes to the search algorithm have vastly improved the behaviour wrt house numbers. So closing here.

@lonvia lonvia closed this Feb 17, 2022
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.

None yet

6 participants