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

Use libspatialjoin for geometric relations, drop Boost dependency #92

Merged
merged 32 commits into from
Oct 10, 2024

Conversation

patrickbr
Copy link
Member

@patrickbr patrickbr commented Jul 18, 2024

This PR uses libspatialjoin to compute the geometric relations. It also completely drops the Boost dependency from osm2rdf (osmium still requires boost) by replacing all geometric stuff with its functional equivalent from util. Additionally, the Bzip2-zipping is now done manually (it was done via boost filtered streams previously).

Everything compiles and seems to work. The E2E tests are still failing, mainly because the mechanism to capture writes to stdout does not work anymore.

It would be great if anyone could test this and give some feedback.

Copy link
Member

@lehmann-4178656ch lehmann-4178656ch left a comment

Choose a reason for hiding this comment

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

Thanks for the work, some minor comments from just looking at the PR. Will test later.

include/osm2rdf/osm/Generic.h Outdated Show resolved Hide resolved
src/osm/GeometryHandler.cpp Outdated Show resolved Hide resolved
src/osm/OsmiumHandler.cpp Outdated Show resolved Hide resolved
@patrickbr
Copy link
Member Author

patrickbr commented Sep 30, 2024

Three notes on the option --add-hascentroid emerged after a short discussion with Axel:

  1. I am against the option --add-hascentroid, and so is Axel. It should be the default behavior, maybe with an option --no-hascentroid to disable it.
  2. It would be elegant to re-use the geometry object of points for their centroid.
  3. A valid query would be: give me all objects whose centroid is in area X. This is not possible at the moment, because the centroids are never given to libspatialjoin for computation of the geometric triples. Problems I see with adding geometric triples for them: (1) sfCovers and sfCovers would essentially double in size, (2) all geometric relations are between OSM objects at the moment (in the triple chain osmobject hasGeometry geometry asWKT WKT, geometric triples are attached to osmobject), but for the centroids, we do not have a distinct OSM object, and the geometric relations would have to be between the geometry object and another OSM object / geometry object.

@patrickbr
Copy link
Member Author

@lehmann-4178656ch, I have now fixed all the tests. I think this branch is now ready to be merged into master. We are already using it in production for the weekly TTL extracts.

@patrickbr
Copy link
Member Author

patrickbr commented Oct 9, 2024

Well, I made the centroid writing the default as a last-minute change, and now all the tests are failing again because of the centroid output. Will fix this.

…es (#95)

The auxiliary geo files will be used in the computation of the geometric triples. The expected format is exactly the same as for spatialjoin (lines of <ID><TAB><WKT>).
Copy link
Member

@lehmann-4178656ch lehmann-4178656ch left a comment

Choose a reason for hiding this comment

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

Thank you for this. I've added some remarks regarding the use of Dx and x<double>, these are not critical.

I'm having an issue trying to build this branch as the spatialjoin submodule fails:

-- Submodule update
Submodule 'vendor/spatialjoin' (https://github.com/ad-freiburg/spatialjoin.git) registered for path 'vendor/spatialjoin'
Cloning into '/app/vendor/spatialjoin'...
fatal: remote error: upload-pack: not our ref 43d5d781931d8f73e1820de0e11321fee75fd50b
fatal: the remote end hung up unexpectedly
Fetched in submodule path 'vendor/spatialjoin', but it did not contain 43d5d781931d8f73e1820de0e11321fee75fd50b. Direct fetching of that commit failed.
CMake Error at CMakeLists.txt:79 (message):
  git submodule update --init failed with 1, please checkout submodules

Following the link in the webui pointing at https://github.com/ad-freiburg/spatialjoin/tree/43d5d781931d8f73e1820de0e11321fee75fd50b results in a "Page not found" error

include/osm2rdf/osm/Node.h Outdated Show resolved Hide resolved
include/osm2rdf/osm/GeometryHandler.h Outdated Show resolved Hide resolved
include/osm2rdf/osm/GeometryHandler.h Outdated Show resolved Hide resolved
include/osm2rdf/osm/Way.h Outdated Show resolved Hide resolved
include/osm2rdf/osm/FactHandler.h Outdated Show resolved Hide resolved
src/osm/FactHandler.cpp Show resolved Hide resolved
src/osm/GeometryHandler.cpp Outdated Show resolved Hide resolved
src/CMakeLists.txt Show resolved Hide resolved
@patrickbr
Copy link
Member Author

Sorry, my bad, I forgot to push the latest spatialjoin. It should work now.

I changed the usage of the templated geometry types to their typedefs everywhere as suggested.

@lehmann-4178656ch
Copy link
Member

I've successfully build the docker container now. Thanks again!

@lehmann-4178656ch lehmann-4178656ch merged commit 2d68b0d into master Oct 10, 2024
@patrickbr patrickbr deleted the use-libspatialjoin branch October 10, 2024 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants