-
Notifications
You must be signed in to change notification settings - Fork 8
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
Refactor predicates for way nodes and relation members #111
base: master
Are you sure you want to change the base?
Conversation
Note: tests are currently failing for the reasons explained in #110 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this refactor. It would be an improvement, though I think there’s an opportunity to streamline the design a little bit. Hope this helps.
src/osm/FactHandler.cpp
Outdated
@@ -209,7 +209,7 @@ void osm2rdf::osm::FactHandler<W>::relation( | |||
const std::string& role = member.role(); | |||
const std::string& blankNode = _writer->generateBlankNode(); | |||
_writer->writeTriple( | |||
subj, _writer->generateIRIUnsafe(NAMESPACE__OSM_RELATION, "member"), | |||
subj, _writer->generateIRIUnsafe(NAMESPACE__OSM_RELATION, "member_ref"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
“Ref” is somewhat confusing because ref=*
is a very common key on both relations and ways. If you renamed member
only for consistency with osmway:node_ref
, I’d suggest calling the latter osmway:member
instead, to avoid the need for this _ref
suffix. This is how OverpassQL refers to the concept.
If you need the predicate of a way to have a different name than the predicate of a relation, then I’d suggest osmway:vertex
. The analogy is that a vertex is to a way as a member is to a relation. The “vertex” terminology is used by editors, so mappers would be familiar with it. It also suggests a possible future enhancement for indicating the angle of the way at the vertex.
(Mappers sometimes distinguish between a node that’s part of a way and a POI that stands alone, but this is very informal and risks confusion with semantic POIs that can be mapped as ways or even relations.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the feedback! So just to make sure I understand you correctly, you would prefer the following design:
osmrel:51477 osmrel:member ?member .
?member osmrel:member_role ?member_role .
?member osmrel:member_pos ?member_pos .
?member osmrel:member_id ?memer_id .
osmway:100 osmway:member ?member .
?member osmway:member_id ?member_id .
?member osmway:member_pos ?member_pos .
Or, alternatively
osmrel:51477 osmrel:member ?member .
?member osmrel:member_role ?member_role .
?member osmrel:member_pos ?member_pos .
?member osmrel:member_id ?memer_id .
osmway:100 osmway:vertex ?vertex .
?vertex osmway:vertex_id ?vertex_id .
?vertex osmway:vertex_pos ?vertex_pos .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, correct. I don’t have a strong preference between the two designs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would then prefer osmway:member
. It would be closer to our original design and therefore introduce only minor changes.
@hannahbast, is this ok for you? I know that I argued against using osmway:member
2 days ago because a way node is not strictly a "member", but OverpassQL indeed seems to use the "member" concept also for nodes, so this is already established in the community. And it does feel natural here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now implemented in 063183c
…os, osmrel:member, osmrel:member_pos, osmrel:member_role
Currently, the predicates for way nodes and member relations look as follows:
This is inconsistent and confusing (especially the usage of
osm2rdfmember:pos
for way node positions, and the double usage ofosmway:node
to point to the?node_ref
, as well as to the?node_id
.After some discussion with @hannahbast, this PR changes the predicates to look as follows:
I also renamed
osmrel:completeGeometry
toosmrel:hasCompleteGeometry
.