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

Add interface API for geocoders #38169

Merged
merged 3 commits into from
Oct 29, 2020
Merged

Add interface API for geocoders #38169

merged 3 commits into from
Oct 29, 2020

Conversation

nyalldawson
Copy link
Collaborator

@nyalldawson nyalldawson commented Aug 6, 2020

This PR adds a basic interface for geocoder classes and operations, to allow a standard API for these using services.

Currently we have tons of geocoder-ish plugins, all of which use their own approaches and interface. Some have updated to use the unified locator bar as their UI, but others have retained custom UI, and to date few (maybe none?) are exposed through processing for bulk geocoding operations.

Accordingly, this interface is designed to remedy this situation! The intention here is that a follow up PR will add adapter classes for easily exposing a QgsGeocoderInterface implementation as a Processing algorithm or locator bar filter, e.g. via

alg = QgsGeocoderAlgorithm(GoogleGeocoder(api_key='xxxxx'))
filter = QgsLocatorFilter(EsriLocationGeocoder(url='xxxxx'))

Possibly we code include some services out-of-the-box as processing algorithms!

Here's an example Google Maps geocoder which follows the interface:

class GoogleGeocoder(QgsGeocoderInterface):
   
    def __init__(self, api_key):
        super().__init__()
        self.api_key = api_key
        self.endpoint = "https://maps.googleapis.com/maps/api/geocode/json?"
        
    def flags(self):
        return QgsGeocoderInterface.Flag.GeocodesStrings
        
    def get_request_url(self, address):
        return self.endpoint + "sensor=false&address={}&key={}".format(urllib.parse.quote(address),self.api_key)
        
    def geocodeString(self, string, context, feedback=None):
        all_results = []
        
        request = QgsBlockingNetworkRequest()
        error_code = request.get(QNetworkRequest(QUrl(self.get_request_url(string))), feedback=feedback)
        
        if error_code != QgsBlockingNetworkRequest.NoError:            
            return [QgsGeocoderResult.errorResult(request.errorMessage())]
            
        json_array = json.loads(request.reply().content().data().decode("utf-8"))

        for result in json_array["results"]:
            latitude = float(result["geometry"]["location"]["lat"])
            longitude = float(result["geometry"]["location"]["lng"])
            
            geom = QgsGeometry.fromPointXY(QgsPointXY(longitude, latitude))
            res = QgsGeocoderResult(geom, QgsCoordinateReferenceSystem('EPSG:4326'))
            
            attributes = {}
            attributes['status'] = str(json_array["status"])
            
            if "formatted_address" in result:
                attributes['formatted_address'] = str(result["formatted_address"])

            if "place_id" in result:
                attributes['place_id'] = str(result["place_id"])
            
            if "location_type" in result["geometry"]:
                attributes['location_type'] = str(result["geometry"]["location_type"])
                
            res.setAdditionalAttributes(attributes)

            all_results.append(res)

        return all_results

coder = GoogleGeocoder(api_key='xxxxxx')

context = QgsGeocoderContext(QgsProject.instance().transformContext())
results = coder.geocodeString('some address', context)

@nyalldawson nyalldawson added the API API improvement only, no visible user interface changes label Aug 6, 2020
@nyalldawson
Copy link
Collaborator Author

Refs qgis/QGIS-Enhancement-Proposals#64

@rduivenvoorde ping, you've previously shown interest in this topic

@github-actions github-actions bot added this to the 3.16.0 milestone Aug 6, 2020
@rduivenvoorde
Copy link
Contributor

@nyalldawson Thanks! I try to understand current reasoning, most of my points in the QEP were already served by the QgsLocator stuff, with which I was very happy :-)

Some thoughts ( I try to be helpfull, please ignore if I make stuff too complex ):

  • Good that you not try to force us in some kind of type of address :-)
  • What is it that we can do with it? A way to canalize how Geocoders should return their results isn't it? I did like the very open way the QgsLocatorFilter handled results very much. How are these two connected?
  • What is the difference between a GeocoderResult (geometry+attributes) and a Feature (more or less the same?) Or is a GeocoderResult a Feature++ (and should it be easy to create a feature from a result?). Thinking about a use-case for a processing-alg which for example read's a csv and then the output is... a layer? or a list of features?
    For other geocoders (eg cadastral plots) I think a good geocoder (compared to the QgsLocator) should be able to show the geometry (be it point, line or polygon) on some (temporal) layer? Or would that be the 'gui'-part (a Geocoding input and Geocoding-layer?)
  • What is the purpose of the context-thingies, is it really nessecary (for processing maybe)? In your example you take the QgsProject.instance().transformContext(). In a case where the geocoder for example returns our nations crs (EPGS:28992) and I want to show it on a EPSG:4326 map, should I then pass another context? We should be a little careful not to go to java-ish (what I mean: create so many abstractions and helper classes that you first need to create 12 classes before outputting a string). Ah duh, now I see QgsLocator.fetchResults actually also has a context.... apparently I do not use this very often.
  • Most geocoding services return upon 1 request a list of results, ofter ordered by some kind of score-number. Would I still have this ordering in the results? Ordering would maybe also be helpfull for routing services?
  • What about Reverse Geocoding (https://github.com/PDOK/locatieserver/wiki/API-Reverse-Geocoder)? You give it a feature (eg a box or an xy, without attributes?) and you get returned a set of (mixed geometry) features. Would that idea fit in this?
  • Should the result also have the 'questioned address'? Thinking about geocoding a list of addresses; should it be part of the interface to return for example a list in which every every 'address' has a list of results? Or do you think that is out of scope for this?
  • Looking at some excisting geocoder interfaces, it is often possible to add some 'filtering' as in: only results from this bbox or results of this type(s).
  • Is QGIS aware of the Geocoders it has on board (aka are they registred) (see below about the input widget)?

Brainwaving about the gui part now:

  • would be could to have some 'result table'-widget which you could just feed the result-list and upon clicking on it zoom/show that geometry (and show all attributes, not depending on any number of them)
  • some input widget which shows all (registred?) Geocoders so you can choose which Geocoder is used when you hit enter (looking that the browser search widget in which you can choose different search machines)

Ok, HTIH :-)

@nyalldawson
Copy link
Collaborator Author

What is it that we can do with it? A way to canalize how Geocoders should return their results isn't it? I did like the very open way the QgsLocatorFilter handled results very much. How are these two connected?

Right - so the locator is definitely not going anywhere, and in my view should remain as the UI interface for "one-off" searches. For those, this interface is designed to assist plugin developers with easily exposing their geocoder to the locator and allow them to skip all the "copy and paste" required to create a locator filter.

If the plugin is exposing a geocoder which follows a standard (such as an ArcGIS Server locator), then it could be as simple as the plugin creating a filter by:

my_geocoder= QgsEsriLocationGeocoder(url='xxxxx')
my_filter = QgsGeocoderLocatorFilter(my_geocoder)
# done!

What is the difference between a GeocoderResult (geometry+attributes) and a Feature (more or less the same?) Or is a GeocoderResult a Feature++ (and should it be easy to create a feature from a result?). Thinking about a use-case for a processing-alg which for example read's a csv and then the output is... a layer? or a list of features?

Again, the intention is to create a simple adaptor class which exposes a geocoder as a ready-made bulk processing algorithm. In that case, the algorithm adapter would be responsible for transforming the geocoder result into a feature (and making the decision about whether to just take the first result, or ...? and whether to add additional "geocode metadata" attributes as fields, etc)
But for a one-off geocode (locator filter), the results would instead be transformed into locator results, so they would never need to go through a QgsFeature.

For other geocoders (eg cadastral plots) I think a good geocoder (compared to the QgsLocator) should be able to show the geometry (be it point, line or polygon) on some (temporal) layer? Or would that be the 'gui'-part (a Geocoding input and Geocoding-layer?)

I'd say we should address this by adding some "preview geometry" functionality to the locator, so that you can hover over results in the list and see a temporary highlight of the result geometry.

What is the purpose of the context-thingies, is it really nessecary (for processing maybe)? In your example you take the QgsProject.instance().transformContext(). In a case where the geocoder for example returns our nations crs (EPGS:28992) and I want to show it on a EPSG:4326 map, should I then pass another context? We should be a little careful not to go to java-ish (what I mean: create so many abstractions and helper classes that you first need to create 12 classes before outputting a string). Ah duh, now I see QgsLocator.fetchResults actually also has a context.... apparently I do not use this very often.

It's for lots of things :), but mostly to allow us flexibility to add other useful things to the context in future without breaking api. If we add all these as extra arguments now, then we can't possibly add more later without breaking api. Currently the context contains useful things like a bounding box geometry which can optionally be used by the geocoder to prioritise results based on their proximity to this area. It's up to the individual geocoder if this is useful, but the more information we provide through the context the more useful the geocoder results will be.

Most geocoding services return upon 1 request a list of results, ofter ordered by some kind of score-number. Would I still have this ordering in the results? Ordering would maybe also be helpfull for routing services?

Yes, that's why the methods return a list of results. Depending on the task we may only want to use the first results and discard the rest (e.g. bulk geocoding), but elsewhere we'd want to expose them all and let the user pick (locator filter)

What about Reverse Geocoding (https://github.com/PDOK/locatieserver/wiki/API-Reverse-Geocoder)? You give it a feature (eg a box or an xy, without attributes?) and you get returned a set of (mixed geometry) features. Would that idea fit in this?

That's a good point. Let me think on that one. Maybe another capability flag to indicate that a geocoder supports this and a new virtual method.

Should the result also have the 'questioned address'? Thinking about geocoding a list of addresses; should it be part of the interface to return for example a list in which every every 'address' has a list of results? Or do you think that is out of scope for this?

The proposed API should be ok for this use case. Incidentally, back a couple of lifetimes ago I used to work in a field where we spent a lot of effort geocoding records. One tool I made there was an interactive geocoder, which would bulk geocode and prompt users for choices whenever the results where ambiguous. They'd get a nice dialog showing all identified "close" matches along with a map canvas they could use to manually place the record. Maybe one day we'll have a tool like this in QGIS too! (or a plugin for it)

Looking at some excisting geocoder interfaces, it is often possible to add some 'filtering' as in: only results from this bbox or results of this type(s).

See above note about the context. For the locator filter, we'd use the current map canvas extent as this geographic hint.

Is QGIS aware of the Geocoders it has on board (aka are they registred) (see below about the input widget)?
would be could to have some 'result table'-widget which you could just feed the result-list and upon clicking on it zoom/show that geometry (and show all attributes, not depending on any number of them)
some input widget which shows all (registred?) Geocoders so you can choose which Geocoder is used when you hit enter (looking that the browser search widget in which you can choose different search machines)

As above, we'll still utilise the locator bar for one-off searches 😄

@nirvn
Copy link
Contributor

nirvn commented Aug 7, 2020

For the locator filter, we'd use the current map canvas extent as this geographic hint.

Great idea. I'd say though we should have a max scale, so that if someone is zoomed onto a very small area, the geocoder doesn't limit search to that narrow area.

@stale
Copy link

stale bot commented Aug 22, 2020

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular

    • link to any issues which this pull request fixes

    • add a description of workflows which this pull request fixes

    • add screenshots if applicable

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in a week.

@stale stale bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Aug 22, 2020
@stale
Copy link

stale bot commented Aug 29, 2020

While we hate to see this happen, this PR has been automatically closed because it has not had any activity in the last 21 days. If this pull request should be reconsidered, please follow the guidelines in the previous comment and reopen this pull request. Or, if you have any further questions, just ask! We love to help, and if there's anything the QGIS project can do to help push this PR forward please let us know how we can assist.

@stale stale bot closed this Aug 29, 2020
@nyalldawson nyalldawson removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Oct 29, 2020
@nyalldawson nyalldawson reopened this Oct 29, 2020
@nyalldawson
Copy link
Collaborator Author

I'd like to merge this now, unless anyone has objections?

@3nids @nirvn @rduivenvoorde ?

@nirvn
Copy link
Contributor

nirvn commented Oct 29, 2020

Hey, I'd love for this to be merged :)

@3nids
Copy link
Member

3nids commented Oct 29, 2020

sounds good to me.
registry and tests are coming in a second step?

@nyalldawson
Copy link
Collaborator Author

registry

Registry no -- but algorithm and filter adapters will be.

and tests are coming in a second step?

I'll include tests of the algorithm and filter... but this PR is just an interface, so there's no really anything I can usefully test...

@rduivenvoorde
Copy link
Contributor

Thanks. Yes please do, I will try to implement both our national geocoder AND reverse geocoder in it then :-)

One practical thing is there is often a round trip to retrieve the actual object, so you get 'results' back within it feature id's which you have to fetch separately.

One other thing I wonder, this seems to return a 'list' of GeocoderResults (being mostly features).
So showing them in some table like structure like the locator is easy to build.
But what about showing these features in the map (or an indication where they are). The structure of the features is not known (attributes I mean). The (geocoder) case below results in both addresses, cities, provinces, etc etc
So the implementer has to create a memory layer (with the right attributes) itself? Or are you/we also thinking about a layer which then can hold that pluriform set of features (just to SEE the results)?

Geocoder then is via geocodeString (example input is zipcode plus building number)

https://geodata.nationaalgeoregister.nl/locatieserver/v3/suggest?q=2022zj 23&rows=20&fq=+type:gemeente+type:woonplaats+type:weg+type:postcode+type:adres+type:perceel+type:hectometerpaal

And ReverseGeocoding is via geocodeFeature

http://geodata.nationaalgeoregister.nl/locatieserver/revgeo?lat=52.39725667&lon=4.64828213&distance=5

@nyalldawson
Copy link
Collaborator Author

@rduivenvoorde

But what about showing these features in the map (or an indication where they are). The structure of the features is not known (attributes I mean). The (geocoder) case below results in both addresses, cities, provinces, etc etc
So the implementer has to create a memory layer (with the right attributes) itself? Or are you/we also thinking about a layer which then can hold that pluriform set of features (just to SEE the results)?

That's handled by the "adapter" classes. I think it'll become clearer when I open the next PR. I'll merge this one for now and will revisit this discussion in the follow up PRs.

@nyalldawson nyalldawson merged commit d484beb into qgis:master Oct 29, 2020
@nyalldawson nyalldawson deleted the geocoder branch October 29, 2020 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API API improvement only, no visible user interface changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants