Split Nearby into a fast query for coordinates + a details query for each pin #4560
Comments
|
Hi @nicolas-raoul , this is a great idea, thanks! We will try to start implementing it. :) One concern that we have is that this would result in a very large number of async calls, in typical use. For instance if there are 40 pins, we will be doing 40 calls. Do you think that would be an issue, or is it routinely done with no problems? An alternative that I was thinking about is:
What do you think? I reckon it wouldn't result in time savings as big as your suggestion, so yours would be the ideal if the number of async calls doesn't become an issue. Also there is the downside of the user having to wait a bit each time a pin is selected. Not sure what effects it will have on limited connection mode, besides. |
|
I don't think that 40 calls is that bad. I suspect that existence checking is the most expensive. And it will get even more expensive in the future. |
|
Out of curiosity, why is existence so expensive? |
|
Here is an example of what the "second query" I mentioned above could look like: https://w.wiki/3ut$ About existence: I only suspect it. I will try to perform some measures tomorrow. But the important measure is between the two queries in the original post, and it clearly shows that showing coordinates could be much much faster :-) |
|
So just to summarise what @nicolas-raoul said, we will be making 2 calls
@nicolas-raoul, please verify if I have understood it properly. |
|
@ashishkumar468 Exactly! 🙂 |
|
Please let me know if you need any help with the SPARQL syntax. |
|
Hi @nicolas-raoul @misaochan , I was trying the implementation for the way Nicolas had suggested. Sharing a sample query for my location. Not sure why, specifically for this id - wd:Q65976956, the query fails, otherwise seems to work. Sharing the exception This can also be seen by running the query link I have shared. Can anything be done about this? |
|
@ashishkumar468 |
|
@ashishkumar468 This SPARQL query works in all situations, and returns all information, please use it, thanks 😊 : |
|
Thanks for helping with the query @nicolas-raoul ! Pinging @ashishkumar468 instead, I don't think the other username works. ;) |
|
Hi @nicolas-raoul , I just talked with Ashish. There are a few things that we need help clearing up, as we aren't very familiar with SPARQL.
Thanks a lot! |
|
My new suggestion: Always running this as the first query: It is super-fast and only gives places and their coordinates and their WLM identifier if they have one, which is enough to start showing pins on the map. ... and then the second query from my comment above to get details. This second query is much slower (I now suspect it is due to my implementation of language fallback), so I would advise running it both:
(if there is no time to implement this, running the second query for all fetched items should work in most places, though) For performance, I suggest removing the By the way, I use this map to get an idea of Wikidata places density, it seems that Amsterdam and London are very crowded. If you know precise locations of the most crowded places, please share coordinates, that will be useful for testing :-) |
|
Hi @nicolas-raoul , Thanks for sharing this, one of the other problems we realised that my PR was making separate calls for nearby and monuments while @misaochan suggests that this can be done in a single call with client side filtering. I am not really comfortable very much with SPARQL queries, would it be possible for you too help with that too (without the split query implementation - for now). What we are planning is to get that in place first and take this one as an improvement. |
|
Yeah, as mentioned above I just realized that we are making two separate queries for Nearby and Monuments, and I'm wondering if that was part of the issue, since we are getting a lot of points and their details twice, and both queries are done sequentially and block map load in the meantime. If so, I think it would make sense to try combining the two first, as that would be a much simpler implementation that could get the WLM release out the door in time for the start date. Even for this split query, I think we should sort out the combining before implementing it, otherwise I'm not sure how it would work. What do you think @nicolas-raoul ? Do you figure we can combine https://github.com/commons-app/apps-android-commons/blob/master/app/src/main/resources/queries/nearby_query.rq with https://github.com/commons-app/apps-android-commons/blob/master/app/src/main/resources/queries/monuments_query.rq ? It looks to me like nearby_query.rq is already getting all the points we need, we just need to filter for the monument properties client-side. Or if a modification is needed to nearby_query to filter for monuments, what should we add to the query? I tried running both queries in https://query.wikidata.org/ for Brisbane City, and every point that exists in monuments_query, also exists in nearby_query. So for that area we were sending duplicate requests for 105 out of 227 points, at a 0.7km radius. Pinging @neslihanturan for input as well. |
|
Sure, here is a query that gives everything, coordinates and details and whether it is a WLM monument: Obviously it times out in crowded places. The example above took 30 seconds even with a tiny radius of 0.1. |
|
Thanks a lot Nicolas! |
|
To be handled after WLM. |
|
Hey @nicolas-raoul, Can I work on this issue 🙂 |
|
@nicolas-raoul I think this Issue should be Locked for GSOC, as it is mentioned on Phabricator too, and other tasks are left for GSOC which are mentioned on Phabricator. Since The other Optional Task is Also Completed and the one main task, which is - Modernize the app by replacing Kotlin Android Extensions and Butterknife with ViewBinding: #4664 is also completed, only this task is left to replace the main completed task #4664. Since it was mentioned on Phabricator with tasks that are already locked I have added (and other contributors too) this task in the proposal and made the proposal according to that so I think it's better to be left for GSOC since less than a week is left for application submission. |
|
Oh sorry @shashankiitbhu, started working already. Will be sharing pr asap 🙂. Maybe @nicolas-raoul we can have a different issue in GSoC. |
|
@kanahia1 I mean it has not been much time since you were assigned this issue, and honestly, I have also done a a lot of starting work on this for the GSOC proposal that I have submitted since I had to make the timeline for it. So all of us are even till this point, good that you have seen this, it'll save you a lot time working on this. The thing is this Issue is already mentioned and proposals are already built on that , so it'll be really counter productive to change now. However, I am waiting for @nicolas-raoul suggestions on what should be done here , so it's best to wait for confirmation, thanks. |
|
Sure, No Worries @shashankiitbhu. Sorry for bothering you 😞 |
|
@kanahia1 no problem, and you didn't bother me at all and thanks for understanding what I was trying to explain here. I am sure there are many other Issues you can work on , in this project and help improve this project. |
|
Sorry for the delay, I was bit out of station. I have already restructured the queries given above so that anyone who works on this issue during GSoC can use these code without any hassle. Thanks to Nicolas Raoul, Neslihanturan, Josephine Lim, Ashish for amazing inputs.
// Note for contributors |
Nearby is too slow to use in Paris:
I suggest splitting the current big SPARQL requests into:
I did some tests (both with 1km radius):
Due to radius stepping, the request that fetches coordinates is often performed several times, making it even more important to fine-tune.
Fetching details afterwards means that some pins will disappear when
Existsis enabled, but I don't think it is problematic as it is disabled by default, users who willingly enabled it will understand why the pins disappear.Latest master.
The text was updated successfully, but these errors were encountered: